Skip to content

feat: ask before handing a requirement to an SOP - #157

Open
crispinhu27 wants to merge 5 commits into
mainfrom
feature/chat-dispatch-proposal
Open

crispinhu27 wants to merge 5 commits into
mainfrom
feature/chat-dispatch-proposal

Conversation

@crispinhu27

Copy link
Copy Markdown
Collaborator

What changed

The assistant could start an SOP the user never asked for, and a timeout could run the same draft twice. A requirement the assistant chooses to hand to an SOP is now a proposal unless the user @ mentioned an SOP or named one. Roles are still dispatched directly. Full access starts the SOP immediately; confirm and assisted approval ask first. A start proposal waits three minutes, then the assistant does the work itself. Accepting is recorded before the draft runs, so a timeout that already handed the work back cannot run it again.

Settings for this live on a new AI assistant page, after Bot management. Proactive role dispatch and proactive SOP use are off by default. Custom rules are stored per user and added to every turn. They do not override approval mode.

Scope

  • Platform core / API / runner
  • Public adapter / deployment
  • Classic
  • Portable
  • Shared behavior contract
  • Installer / CLI
  • Documentation only

Compatibility

New table assistant_preferences is created by AutoMigrate. No manual SQL. New routes: GET/PUT /api/assistant/preferences and POST /api/assistant/sessions/{id}/proposals. Existing conversations and role dispatch are unchanged. Full access still starts an SOP without a card.

Validation

go test -count=1 -timeout 180s -run 'TestSOPProposal|TestVetProposal|TestDecideProposal|TestUnansweredStart|TestSOPDraftsNeed|TestAssistantPreferences|TestAssistantProgressSubmission' ./internal/httpapi/
go test -count=1 -timeout 120s -run TestAssistantSettings ./web
npx vitest run tests/components.test.jsx tests/hooks.test.jsx tests/conversation-navigation.test.mjs
python3 scripts/check-public.py
AGENTFLOW_E2E_LOCAL_PORT=18117 AGENTFLOW_E2E_LOCAL_MYSQL_PORT=33417 AGENTFLOW_E2E_LOCAL_MYSQL_CONTAINER=loopforge-e2e-dispatch ./e2e/local.sh api/70-assistant-proposals.test.mjs

The Go tests, the web settings test, and 43 chatbot tests passed. The public-content check passed. The e2e file ran against MySQL 8 in Docker: preferences persisted, an oversized rule was rejected, decline returned the follow-up and a second decision returned 409, accept recorded the decision without a follow-up, and a plain draft was rejected. Signed in on the local platform and saved the AI assistant settings; a reload kept the switch and the rule.

Not run: go test ./..., go vet ./..., bash local/scripts/validate.sh, bash local/scripts/smoke-install.sh, bash local/scripts/e2e.sh, bash scripts/scan-secrets.sh. No live model turn was used to generate a proposal card.

Safety

  • I did not include credentials, internal endpoints, production data, or organization-specific infrastructure.
  • I preserved user files and unrelated changes.
  • I updated generated Classic host bundles when applicable.
  • I documented third-party content and licensing changes when applicable.

The assistant could start an SOP the user never asked for, and a timeout could run the same draft twice.
The view tour treats a nav entry without a title as a broken page.

@Robert0622 Robert0622 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 57be7b4. I reproduced a blocking proposal-decision race in chatbot/src/useChat.js (applyDraft):

[P1] A stale proposal card can execute after the stored decision is declined, muted or expired. applyDraft fetches the fresh session, but only calls the /proposals acceptance endpoint when !draft.decision. If the server now returns decision: "declined", "muted" or "expired", it skips acceptance and proceeds directly to /mcp. For example, one tab lets the proposal expire (or another participant declines it), while a second tab still displays the old executable card. Clicking that card starts the SOP even though the decision already rejected it, potentially alongside the timeout's Main Bot continuation.

Local reproduction: I added three temporary hook regression cases to the existing Vitest fixture. Each returned a freshly fetched draft with one of those decisions, called applyDraft, and asserted zero /mcp calls. All three failed: each made one /mcp call. The existing test only covers the different race where /proposals itself returns 409.

Please reject all terminal negative decisions before executing any write, refresh the displayed receipt, and add these regression cases. Ideally validate the proposal decision and consume its execution atomically on the server as well; accepting an already accepted but unsaved card in two clients currently has no single execution claim. I did not exercise a paid-model turn for this finding: the reproduction uses the real React hook with mocked HTTP responses and no external writes.

@Robert0622

Copy link
Copy Markdown
Collaborator

LoopForge PR Review — head 57be7b4d5f42130d800717a8d2dd09918f17c6d6

Verdict: ✅ Pass

  • Checked out the head SHA in an isolated sandbox (no secrets/no prod access) and read the full diff against base ffaed1574, focusing on internal/httpapi/assistant_proposals.go (new), assistant_preferences.go (new), internal/domain/assistant_proposal.go, internal/repo/assistant_proposals.go, and the chatbot proposal-card UI.
  • Concurrency check: decideProposal rejects a second decision on the same draft (draft.Decision != "" → 409), and expireProposal only fires when decision == "" && saved != true and the stamped expiresAt still equals the deadline it was scheduled against — so a timeout that lost the race to a user decision becomes a no-op instead of double-running the handoff. This matches the PR's claimed guarantee (accept recorded before the draft runs; a second decision on an already-decided proposal 409s).
  • Approval-mode gating (vetProposalDrafts/delegationContext) only requires proposal={reason,timing} for SOP-handoff drafts (issues_dispatch, runs_start, or issues_save with no existing requirement id) outside full-access mode, and correctly respects per-conversation mute/decline history (proposalHistory).

What I ran (built web/native-chat first — go test ./internal/httpapi needs the embedded chatbot assets):

  • go test -count=1 -timeout 180s -run 'TestSOPProposal|TestVetProposal|TestDecideProposal|TestUnansweredStart|TestSOPDraftsNeed|TestAssistantPreferences|TestAssistantProgressSubmission' ./internal/httpapi/ → ok
  • go test -count=1 -timeout 120s -run TestAssistantSettings ./web → ok
  • npx vitest run in chatbot/ (full suite, superset of the PR's listed files) → 46 files / 294 tests passed
  • python3 scripts/check-public.py → passed

Not run here: the Docker/MySQL e2e (e2e/local.sh api/70-assistant-proposals.test.mjs) — no Docker available in this review sandbox. The PR description reports it green with specific scenarios (preferences persisted, oversized rule rejected, decline returns follow-up + second decision 409s, accept records decision without follow-up). No blocking issues found from the code/unit-test review.

@Robert0622 Robert0622 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

复审当前 head 57be7b4:此前的 P1 尚未修复。本次再次运行真实 useChat hook 的回归复现,declined、muted、expired 三个用例全部失败,每种状态都仍发送一次 /mcp 写请求。

旧标签页保留可执行的建议卡片,另一个标签页拒绝建议或等待超时后,旧标签页点击执行,applyDraft 虽读取最新草稿,但在已有负面 decision 时跳过 /proposals 的确认,直接执行写入。这可能同时启动 SOP 和超时后的助手续作。

请在写入前拒绝这些终态、刷新卡片并补上三项回归,最好由服务端原子校验并认领执行。当前保留 Changes requested,不应因后续 Pass 概述而视为已解决。本次复现使用模拟 HTTP 和真实 React hook,没有启动真实 SOP。

@fx-kurumi

Copy link
Copy Markdown
Collaborator

✅ 结论:Approve(通过)

复审 head 76c8cbe

上次 Changes requested 针对的是 57be7b4。之后新增了 bf8996f fix(chatbot): block execution of rejected SOP proposals,以及一个合并 origin/main 的 merge commit 76c8cbe。

此前 P1(旧卡片在 declined/muted/expired 后仍执行)已修复

  • chatbot/src/useChat.js:952-955:applyDraft 先读取最新会话,没有决定时先 accept。之后只要 draft.proposal && draft.decision !== "accepted",就刷新会话、抛出「这条建议已经处理过,不能执行。」,并且不会发送 /mcp。这个错误会走外层 catch 中的 setError 分支,executed=false。
  • chatbot/tests/hooks.test.jsx 新增了 declined/muted/expired 三个用例,断言没有 /mcp、没有写入,并且卡片被刷新。我在本地临时去掉这 4 行修复代码后,这三个用例全部失败;恢复代码后全部通过。也就是说,这组回归测试确实能锁住这个问题。

服务端认领情况

  • 决定的写入是原子的:decideProposal(internal/httpapi/assistant_proposals.go:346-432)和超时 expireProposal(:508-541)都通过 PutAssistantSessionIfUnchanged / PutSharedAssistantReceipt(SELECT ... FOR UPDATE + 比对 UpdatedAt 和消息内容 + 检查是否有 running generation)写入。所以 accept 和 expire、或两个 accept 之间只会有一方成功,另一方返回 409 或什么都不做。
  • 执行本身没有和这次认领绑定(非阻塞,建议后续跟进):/mcp 不检查 proposal 的 decision。决定变成 accepted 以后,在 saved 写回之前,另一个标签页或另一位共享会话成员点击旧卡片时,读到的最新状态是 accepted,会跳过 accept 直接再执行一次 /mcp,可能派发出两个 SOP 运行。这个窗口很窄,而且 main 上的普通确认草稿也有同样的「多端各自执行」问题,所以不作为阻塞项。根治办法是把 decision 记录成「谁、哪一次认领」(例如 claimId),由 /mcp 的会话头校验后再在服务端执行;或者由 /proposals 的 accept 直接在服务端执行并写回 saved。

AGENTS.md 检查:新表 assistant_preferences 只靠 AutoMigrate 建表,没有数据迁移和 seed,符合规则;repo 层和 handler 层的分层没问题。

验证

  • npx vitest run(chatbot/ 全量):47 个文件,306 个测试全部通过
  • 临时回退 useChat.js:952-955 后运行 npx vitest run tests/hooks.test.jsx -t "proposal decision":3 个失败;恢复后通过
  • go test -count=1 ./internal/httpapi -run 'Proposal|SOPDrafts|AssistantPreferences|AssistantProgressSubmission|UnansweredStart':ok
  • go test -count=1 ./web -run TestAssistantSettings:ok
  • go build ./...、go vet ./internal/httpapi ./internal/repo:通过
  • 本地起了一个临时平台(一次性 MariaDB、虚拟账号),截了两张图:「AI 助手」设置页,以及一张注入的 timing=start 建议卡片(倒计时、走 SOP / 让 AI 助手自己做 / 本会话不再建议)
  • 新 head 的 CI:core、browser、first-run、macOS e2e 在审查时还在运行,其余已通过。建议合并前确认这几项转绿。

Robert0622
Robert0622 previously approved these changes Nov 11, 2026

@Robert0622 Robert0622 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

复审 76c8cbe(含修复 bf8996f):上次的 P1 已修复,approve。合入前需要先解决与 main 的冲突,见文末。

修复确认

  • applyDraft 在执行写入前会拒绝 decision 不是 accepted 的建议,并用最新会话刷新卡片;新增的 declined / muted / expired 三项 hook 回归覆盖了这个路径。
  • 真实 Chromium 实测(临时平台 + MySQL):打开带建议卡片的会话后,另一端通过 API 把建议改成 declined 或 muted,再点旧卡片的「走 SOP」:
    • /mcp 写请求数为 0
    • 页面提示「这条建议已经处理过,不能执行。」
    • 卡片刷新为「未采纳」/「未采纳,本会话不再建议」
    • 无页面错误
  • expired 只能由服务端超时产生,没法通过 API 构造,由单测覆盖。

其他验证

  • chatbot vitest 306/306、npm run build、go vet ./internal/...、go test internal/httpapi、repo、domain 与 ./web/... 全部通过。
  • e2e/api/70-assistant-proposals.test.mjs 4/4 通过。
  • CodeBuddy IOA 真实模型用例 api/40-model.test.mjs 8/8 通过,包括 AI 助手问答、SSE、草稿确认后续作不重复执行。
  • CI 16/16 通过。

非阻塞说明
如果另一个标签页已经 accept、正在执行,旧卡片读到 accepted 且尚未 saved,仍会再执行一次(实测 /mcp 调用 1 次)。普通草稿在 main 上也存在同样的双标签页竞态,不是本 PR 引入的。后续可以考虑由服务端原子认领执行。

合入前
#169 已合入 main,重构了设置导航和资产页。本 PR 在 web/app.js、web/index.html、web/i18n-en.js、e2e/ui/01-views.spec.mjs 有冲突。解决时请把「AI 助手设置」接到新的设置侧边栏,并重跑 ui/01-views。

Keep the AI assistant settings page inside the consolidated settings navigation.

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants