Skip to content

feat(core,pi): add EventNotice for transient retry hints (fixes #1684) - #1685

Open
chenhg5 wants to merge 2 commits into
mainfrom
agent/cc-connect/t-20260815-1rcubl-pi-retry-notice
Open

feat(core,pi): add EventNotice for transient retry hints (fixes #1684)#1685
chenhg5 wants to merge 2 commits into
mainfrom
agent/cc-connect/t-20260815-1rcubl-pi-retry-notice

Conversation

@chenhg5

@chenhg5 chenhg5 commented Aug 15, 2026

Copy link
Copy Markdown
Owner

Summary

Implements engine-level EventNotice for transient, non-fatal progress hints, and wires the Pi adapter to surface retry backoff hints on the platform progress card.

Fixes #1684.

Background

Pi auto-retries on transient provider failures (HTTP 429 / 5xx / overloaded). agent_end.willRetry=true and Pi 0.84.0+ session events auto_retry_start/auto_retry_end keep the turn open per #1597/#1677, but during the exponential backoff (up to minutes) the progress card stays silent. Users on rate-limited providers (Kimi etc.) cannot tell "Pi is backing off before retry 3/8" from "the agent hung".

Approach

Engine (this PR adds the channel)

  • core.EventNotice — new event type. Non-fatal, non-terminal: engine appends a progress entry but never finalizes the turn. Subsequent EventToolUse/EventText/EventResult/EventError flow through the normal lifecycle.
  • core.ProgressEntryNotice — new structured progress card entry kind. inferLegacyEntryKind recognizes / ⚠️ prefixes so older pi builds emitting plain "retrying…" text also get the right kind.
  • core.ToolStepKindInfo — corresponding rich-card tool-step kind for platforms that use the rich-card rendering path.

Pi adapter (first consumer)

  • agent/pi/session.go — translates agent_end.willRetry=true and auto_retry_start into EventNotice. Carries structured metadata (attempt, maxAttempts, delayMs, error) so platforms that want richer UX ("retry 3/8 in 8s — 429") can use it; the fallback Notice text is good enough for any platform that ignores metadata.
  • Falls back to a minimal ⏳ provider is rate-limited, retrying… hint when pi omits the structured fields.
  • auto_retry_end deliberately emits no notice — the next event (agent_start success or EventError failure) drives the visible state, no need to flood the card.

Constraint

This round ships engine EventNotice + pi adapter only. Other agents (claudecode/codex/opencode/qoder/dsh) can opt in by translating their own retry signals into EventNotice — no core change required.

Tests

  • agent/pi/pi_test.go — five unit tests covering agent_end.willRetry=true, agent_end without willRetry, auto_retry_start, auto_retry_end (no-op), and the empty-fields fallback. Plus formatDelayMs formatting.
  • core/engine_test.go — three integration tests covering the engine lifecycle:
    • TestProcessInteractiveEvents_NoticeDoesNotFinalizeTurn: notice followed by EventResult ends in ProgressCardStateCompleted, not failed.
    • TestProcessInteractiveEvents_NoticeEmptyIsNoOp: empty notice is silently dropped, no empty entry on the card.
    • TestProgressEntryNotice_InferLegacyKind: legacy / ⚠️ prefixes get the notice kind.

All tests pass locally with go test -race for both ./core/ and ./agent/pi/, plus the dependent ./platform/feishu/, ./agent/claudecode/, and ./agent/codex/ packages.

Files Changed

  • core/message.goEventNotice type, Notice / NoticeMetadata fields on Event.
  • core/progress_compact.goProgressEntryNotice kind, inferLegacyEntryKind recognition.
  • core/streaming.goToolStepKindInfo kind.
  • core/engine.goEventNotice switch case; appends notice entry, never finalizes or returns.
  • core/engine_test.go — integration tests.
  • agent/pi/session.goemitRetryNotice, buildRetryHint, formatRetryHint, formatDelayMs, retryMetadata, getInt helpers; new event types auto_retry_start / auto_retry_end.
  • agent/pi/pi_test.go — adapter unit tests.

Risk / Compatibility

  • Purely additive. No existing event type changes; other agents' behavior is unchanged.
  • Platforms without MessageUpdater see the notice as a regular chat message — same fallback path as EventThinking and EventToolResult.
  • Empty Notice is silently dropped, so misconfigured adapters cannot accidentally flood the card.

Not In Scope

  • Localization (MsgAgentRetryHint etc.) — kept English-only this round; platform teams can localize on their side if needed.
  • Adapter-level wiring for claudecode / codex / opencode / qoder / dsh — follow-up issue(s) as maintainer decides.
  • Richer UX variants ("retry N/M in Ds" formatting in the card renderer) — left to the platform teams; structured metadata is available for any of them to opt in.

Pi auto-retries on provider rate-limit / 5xx / overload, and agent_end.willRetry=true
plus Pi 0.84.0+ session events (auto_retry_start/auto_retry_end) keep the turn open
per #1597. But the backoff can run for minutes, leaving the platform progress card
silent — users cannot tell retry from hang.

Add a generic engine-level transient-notice channel:

- core.EventNotice — non-fatal, non-terminal event. Engine appends a
  ProgressEntryNotice to the structured progress card but never finalizes
  the turn. Subsequent EventToolUse / EventText / EventResult / EventError
  drive the normal lifecycle.
- core.ProgressEntryNotice — new typed progress card entry; legacy fallback
  recognizes ⏳ / ⚠️ prefixes via inferLegacyEntryKind so older pi builds
  emitting plain 'retrying…' text also get the right kind.
- core.ToolStepKindInfo — corresponding rich-card tool-step kind.
- agent/pi — translate agent_end.willRetry and auto_retry_start into
  EventNotice, with structured metadata (attempt/maxAttempts/delayMs/error)
  for platforms that want richer rendering. Falls back to a plain 'retrying…'
  hint when pi omits the structured fields.

Constraint: this round ships engine EventNotice + pi adapter only. Other
agents (claudecode/codex/opencode/qoder/dsh) can opt in by translating
their own retry signals into EventNotice — no core change needed.

Closes: #1684

@chenhg5 chenhg5 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

QA Review — PR #1685

结论: Comment (建议 maintainer merge;CI 全绿、diff 隔离良好、纯 additive。)

注: chenhg5 是 PR 作者 (OWNER),gh CLI 会拒绝 self approve/request-changes,降级为 comment。结构化输出 review_decision 字段保留我建议的结论。

总体判断

PR #1685 实现 engine 通用 transient notice 通道 + Pi adapter 第一个接入,fixes #1684。改动隔离在 7 文件 581 行 (+581/-0),纯 additive 不破坏任何现有 API。CI 5/5 全绿,本地测试覆盖 engine 集成 + pi adapter 全套 + 依赖包回归。

✅ 做得好的地方

  1. Engine-generic schema,pi-first 接入:完全按 maintainer 在 PM 分流时拍的方向 (Issue #1684 决策报告 doc-20260815-pbtl9q)。core.EventNotice 一次性建立通用通道,后续 claudecode/codex/opencode/qoder/dsh 接入只需 adapter 翻译一次,无需 core 改动。一次扩展长期受益。

  2. Engine 不终结 turn 的关键不变量:新 case 用 break 而非 return,且没有 cardMessageID = nil 或类似的状态重置。TestProcessInteractiveEvents_NoticeDoesNotFinalizeTurn 显式断言 notice → EventResult 后状态为 ProgressCardStateCompleted,这是 #1597/#1677 路径不回归的回归测试。

  3. 三层渲染 fallback:rich card (有 PreviewStarter/MessageUpdater) → compact progress (cp.AppendEvent) → plain text (sendWorkspace)。所有 path 都覆盖,平台实现差异不影响 notice 路径。

  4. Backward compat:inferLegacyEntryKind 识别 /⚠️ prefix,老 pi build 走 raw text 路径仍能正确识别为 notice kind。这是迁移期的友好设计。

  5. 测试质量高:

    • 3 个 engine 集成测试 + 6 个 pi adapter unit test,共 9 个新测试
    • 覆盖 happy path、空字段 fallback、empty notice no-op、legacy prefix、delay 格式化边界 (0/250/1500/60000/65000 ms)
    • 显式断言 metadata round-trip (attempt=3, maxAttempts=8, delayMs=8000)
    • 显式断言 notice 不影响 terminal state (ProgressCardStateCompleted)
    • 显式断言 auto_retry_end no-op (避免 flooding)
  6. 代码组织清晰:小函数 (emitRetryNotice / buildRetryHint / formatRetryHint / formatDelayMs / retryMetadata / getInt),每个单一职责。retryHint struct 把 text + metadata 打包传递。

  7. 不破坏 #1597/#1677 行为:在 RPC mode,s.emitRetryNotice(raw)s.handleAgentEnd(raw) 之后调用,但在 EventResult 之前 (per comment),保证 notice 先于 terminal event 进入 events channel。

🟠 P2 Should improve (建议 follow-up,不阻塞)

  1. formatDelayMs 1500ms → "2s" 截断精度:pi 实际 delay 1500ms 时 UI 显示 "2s",1500-2499ms 都是 "Ns" (round-half-up)。test 已覆盖,代码正确,但建议在 i18n 阶段把精度提到 500ms 或 100ms 步进。可在 follow-up issue 提出。

  2. auto_retry_start 不携带 eventType 区分:engine 不知道这是 retry-specific notice 还是 generic info notice。如果后续有别的 agent 用 EventNotice 表达 "正在思考" / "等待用户输入" 之类的 hint,需要区分。可考虑未来加 NoticeMetadata["kind"] 已经有 kind: "retry",这是对的;只是建议在 docs 里说明 kind 字段是公共 API,platform 应基于 kind 决定 rendering。

  3. cp.AppendEvent(ProgressEntryNotice, notice, "", notice) 第四参重复:第二个 notice 是 metadata(也是 notice text)。看起来不像有意为之,但 PR description 没解释为什么 metadata 是 text 本身。建议在 follow-up 确认 metadata 实际使用方,如果是给 platform 直接渲染的,可以简化为 nil。

  4. English-only 文案硬编码:"⏳ provider is rate-limited, retrying…""⏳ provider is rate-limited: retry N/M in Ds — err" 都是英文硬编码。PR 描述里写明"i18n not in scope, kept English-only this round",这是合理决定。建议在 follow-up 把 i18n key 抽出来 (MsgAgentRetryHint 等),让 platform 团队可以本地化。

  5. buildRetryHint 默认 attempt=1, maxAttempts=0 不显示 N/M:实际逻辑是 if maxAttempts <= 0 { omit N/M fraction }。这条 fallback 路径会让 progress card 显示 "retrying in 8s — 429",而不是 "retry 1/? in 8s — 429"。这是 design choice,合理但不直观,建议在 doc-comment 里加一行解释。

🔵 P3 Nice to have (可选)

  1. getInt helper 只支持 float64/int/int64,没有 uint / json.Number。如果 pi 之后用其他 numeric type 发送 retry metadata,可能 silent fallback 到 0。可以 follow-up 加 warning log。

🚨/🔴 Blocker

未发现。CI 5/5 全绿,mergeable_state=clean,mergeable=true,无 conflict。

🧪 测试与 CI

check conclusion job
lint success 94964345176
unit-test success (4m23s) 94964512582
smoke-test success (33s) 94944891367
regression-test success (34s) 94944965028
performance-test success (52s) 94945033801

本地验证(PR description):

  • go build -tags no_web ./... PASS
  • go vet -tags no_web ./... PASS
  • go test -tags no_web -race ./core/ ./agent/pi/ PASS
  • go test -tags no_web ./platform/feishu/ ./agent/claudecode/ ./agent/codex/ PASS

📦 改动内容

  • core/message.go (+14):新增 EventNotice type + Event.Notice + Event.NoticeMetadata 字段,纯 additive。
  • core/progress_compact.go (+6):新增 ProgressEntryNotice kind + inferLegacyEntryKind 识别 /⚠️ prefix。
  • core/streaming.go (+3):新增 ToolStepKindInfo kind 用于 rich card。
  • core/engine.go (+40):processInteractiveEvents switch 新增 EventNotice case;三层 fallback (rich card / compact progress / plain text);空 Notice 防御性 skip;break 不 finalize。
  • core/engine_test.go (+165):3 个 integration 测试覆盖 notice-not-finalize / empty-noop / legacy-prefix。
  • agent/pi/session.go (+173):emitRetryNotice + buildRetryHint + formatRetryHint + formatDelayMs + retryMetadata + getInt;agent_end.willRetry=trueauto_retry_start 翻译为 EventNotice;auto_retry_end 故意 no-op。
  • agent/pi/pi_test.go (+180):6 个 unit 测试覆盖 willRetry / no-willRetry / auto_retry_start / auto_retry_end no-op / missing-fields-fallback / delay-format。

🎯 QA 推荐动作

merge。理由:

  1. CI 5/5 全绿,mergeable_state=clean
  2. 纯 additive,无 breaking change,旧 pi build + 旧 cc-connect binary 走 fallback 路径行为不变
  3. 9 个新测试覆盖 happy path + edge cases + 回归保护
  4. 与 maintainer 在 PM triage 阶段拍的方向 (#1684 决策报告 推荐 B) 完全一致
  5. Schema-generic 通道为后续 agent 接入零核心改动,长期 ROI 高
  6. P2 建议都是 follow-up 改进,无阻塞合并的理由
  7. 与已 merged 的 #1677 (willRetry keeps turn open) 路径对齐,不破坏 #1597/#1677 行为

Next step

Maintainer 决定合并后,建议:

  1. 在 release notes / changelog 标注:feat(core,pi): engine-level EventNotice + Pi auto-retry transient hint (#1684)
  2. 创建 follow-up issue 跟踪 i18n (English-only 文案本地化)
  3. 创建 follow-up issue 给 claudecode/codex/opencode 接入 EventNotice 通道

QA 报告: 见 PR #1685 QA 决策报告 (cc-connect/qa/pr-1685/)

…notice

Resolve conflicts caused by #1597 (willRetry turn-open) and #1693 (v1.5.0-beta.3 P1 stability)
landing on main after #1685 was branched.

Conflict in agent/pi/session.go (agent_end case):
  - PR #1597 (origin/main): keep turn open on willRetry=true, surface
    deferred pendingErr on real close, emit EventResult on rpc mode.
  - PR #1685 (HEAD): emit a transient EventNotice via emitRetryNotice
    before falling through so the progress card stays informative.

Resolution: call emitRetryNotice first (it's a no-op when willRetry=false
because buildRetryHint returns ok=false for normal agent_end), then keep
#1597's willRetry break + pendingErr flush + rpc EventResult path. This
preserves #1597's turn-open invariant while adding #1684's progress hint.

@chenhg5 chenhg5 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

QA Re-Review — PR #1685 (post merge conflict fix)

结论: Approve (建议 maintainer merge;merge conflict 已正确解决,CI 5/5 全绿,turn-open 与 EventNotice 两套不变量都被显式保护。)

注: chenhg5 是 PR 作者 (OWNER),gh CLI 会拒绝 self approve/request-changes,降级为 comment。结构化输出 review_decision 字段保留 QA 建议(merge)。

总体判断

PR #1685 与 main 的合并冲突已解决 (merge commit 2316c46d89cf)。dev-claudecode 选择 git merge origin/main 而非 rebase,保留 PR #1685 的原始 commit (504504b),便于审查追溯"EventNotice 引入意图"与"willRetry 兼容点"两个独立改动。冲突解决策略干净,3 个 P1 改动 (#1684 EventNotice / #1597 willRetry turn-open / #1693 v1.5.0-beta.3 stability) 共存,无相互破坏。

✅ 做得好的地方

  1. 冲突解决策略正确:git merge origin/main 保留 PR #1685 的原始 commit (504504b),不 squash 不 rebase。后续 review 仍可追溯 "EventNotice 引入意图" (504504b) 与 "willRetry 兼容点" (2316c46 merge commit) 的独立改动。如果用 rebase 会把 EventNotice + willRetry 兼容点混在一起,review 不友好。

  2. session.go agent_end case 顺序最关键: emitRetryNotice 在 willRetry 检查之前调用

    • willRetry=false (普通终态) 时: emitRetryNotice 是 no-op (buildRetryHint 返回 ok=false),然后走 pendingErr flush + rpc EventResult 路径
    • willRetry=true (重试中) 时: emitRetryNotice 先发 EventNotice,再 break 保持 turn open
    • 两套不变量都不被破坏
  3. pi_test.go 两个 willRetry test 显式更新(这是关键):

    • TestHandleEvent_AgentEndWillRetryKeepsTurnOpen: 从原 len(evts) != 0 改为 for _, e := range evts { if e.Type == EventResult || EventError { t.Fatalf } }。保留 turn-open 不变量,且承认 EventNotice 是新引入的合法事件(见 inline comment)。
    • TestHandleEvent_AgentEndRetrySuccessDropsPendingError: 改为统计 results/notices/errs 三类,results==1, errs==0, notices>=1。保留原"pendingErr dropped after successful retry" 不变量,显式断言 notices>=1 保护 #1684 行为不被回归。这是非常好的 regression guard。
  4. CI 5/5 全绿 (run 32044368384): lint / unit-test / smoke-test / regression-test / performance-test 全部 success。regression-test 首次失败是 GitHub infrastructure 503/429 (setup-go 下载失败),rerun 后通过,非代码问题。

  5. mergeable=true, mergeable_state=clean: GitHub 报 PR 现在可合并。无 conflict,无 resolution 遗留。

  6. 测试本地全部通过(per dev_rework summary):

    • go test -tags no_web -race ./agent/pi/ (含 #1597 willRetry tests + #1685 emitRetryNotice tests) all pass
    • go test -tags no_web -race -run 'Notice|EventNotice' ./core/ all pass
    • go test -tags no_web -race ./core/ ./agent/pi/ ./platform/feishu/ all pass
    • go build -tags no_web ./... clean
    • gofmt -l agent/pi/session.go agent/pi/pi_test.go clean

🟠 P2 Should improve (建议 follow-up,不阻塞)

  1. merge commit message 提到 file-level diff 时建议列出每个文件的影响: 当前 message 只详细解释了 session.go 的冲突,pi_test.go 用 "git 自动合并,人工复核" 简略描述。建议 follow-up 改进 merge commit 模板,显式列出每个被冲突影响文件的 resolution 选择。

    实际方案不需要重做,只是 message 写作规范。

  2. 回归测试断言 notices>=1 是软约束: notices < 1 会 fail,但 notices>=2 也会 pass(虽然现实不会)。如果未来 emitRetryNotice 改成发 2 个 notice (e.g., 一个 retry-start 一个 retry-end),这个 test 不会捕获。建议改为 notices == 1 显式锁定数量。

  3. 测试 drainEvents 后没断言顺序: TestHandleEvent_AgentEndRetrySuccessDropsPendingError 期望结果是 [EventNotice (from willRetry), EventResult (from retry success)],但只统计计数。建议 follow-up 加顺序断言,确保 EventNotice 在 EventResult 之前发出 (避免未来 refactor 颠倒顺序)。

  4. PR 标题仍是单 PR 标题,merge commit 的 message 与 PR title 没有衔接。GitHub 默认会用 PR title 作为 merge commit title。可以接受,但建议 follow-up 标准化 merge commit 模板。

🔵 P3 Nice to have (可选)

  1. merge commit message 里 #1597 / #1693 没有 commit hash,如果 reviewer 想看具体改动需要手动查找。建议 follow-up 在 merge commit message 里加 commit <full-sha> 链接。

🚨/🔴 Blocker

未发现。merge conflict 已解决,CI 5/5 全绿,无冲突,无新增风险。

🧪 测试与 CI

check conclusion 时长 job
lint success 1m58s run 32044368384
unit-test success 4m53s run 32044368384
smoke-test success 1m2s run 32044368384
regression-test success (rerun) 1m0s run 32044368384
performance-test success - run 32044368384

5/5 SUCCESS, mergeable_state=clean, mergeable=true

本地验证(per dev_rework summary):

  • go test -tags no_web -race ./agent/pi/ -count=1 (7.0s, all pass)
  • go test -tags no_web -race -run 'Notice|EventNotice' ./core/ (1.0s, all pass)
  • go test -tags no_web -race ./core/ ./agent/pi/ ./platform/feishu/ all pass
  • go build -tags no_web ./... clean
  • gofmt -l clean

📦 关键 merge resolution 改动

agent/pi/session.go agent_end case (新顺序):

case "agent_end":
    s.handleAgentEnd(raw)
    // Issue #1684: emitRetryNotice runs first; when willRetry=false,
    // buildRetryHint returns ok=false → no-op.
    s.emitRetryNotice(raw)
    if willRetry, _ := raw["willRetry"].(bool); willRetry {
        // #1597: keep turn open
        break
    }
    if s.pendingErr != "" {
        // #1597: surface deferred error
        evt := core.Event{Type: core.EventError, Error: fmt.Errorf("%s", s.pendingErr)}
        s.pendingErr = ""
        select { case s.events <- evt: case <-s.ctx.Done(): }
    }
    if s.rpc {
        // #1597: emit EventResult on RPC mode
        sid := s.CurrentSessionID()
        evt := core.Event{Type: core.EventResult, SessionID: sid, Done: true}
        select { case s.events <- evt: case <-s.ctx.Done(): }
    }

agent/pi/pi_test.go 关键更新:

  • TestHandleEvent_AgentEndWillRetryKeepsTurnOpen: 改为枚举 EventResult/EventError 断言 turn-open 不变量,显式承认 EventNotice 是合法事件
  • TestHandleEvent_AgentEndRetrySuccessDropsPendingError: 改为统计 results/notices/errs 三类,显式断言 notices>=1 保护 #1684 行为

🎯 QA 推荐动作

merge。理由:

  1. merge conflict 已干净解决 (策略: git merge 而非 rebase,保留原始 commit)
  2. session.go 顺序正确: emitRetryNotice 在 willRetry 检查之前,既保证 #1684 progress hint 又保留 #1597 turn-open invariant
  3. pi_test.go 两个 test 显式更新 + 添加 notices>=1 regression guard
  4. CI 5/5 全绿,mergeable_state=clean
  5. 与 main 已合入的 #1597/#1693 共存,无相互破坏
  6. P2/P3 都是 follow-up 改进(merge commit 模板 / 测试断言精度),无阻塞理由

Next step

Maintainer 决定合并后建议:

  1. 同步 release notes 标注:feat(core,pi): engine-level EventNotice + Pi auto-retry transient hint (#1684) + willRetry turn-open 兼容 (#1597) + v1.5.0-beta.3 stability 共存 (#1693)
  2. 创建 follow-up issue 跟踪 P2 改进 (merge commit message 模板 / notices==1 精度 / 顺序断言)
  3. v1.5.0 stable release gate 可继续推进 (与 #1693 同节奏)

QA 报告: 见 PR #1685 QA Re-Review 决策报告 (post-merge conflict fix)

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.

feat(pi): surface a transient "retrying…" hint when Pi auto-retries (agent_end.willRetry)

1 participant