fix: scheduler detectResultKind allowlist + envelope kinds #1716

Merged

hongming merged 2 commits from fix-scheduler-detect-result-kind-message-allow into main 2026-05-23 09:15:35 +00:00

Conversation 20 Commits 2 Files Changed 2

+139 -8

hongming commented 2026-05-23 07:40:22 +00:00

Owner

Copy Link

Copy Source

Summary

PM scheduler observed 21 consecutive SDK error: result_kind=message ticks (canvas screenshot 2026-05-23) before scheduler auto-disable kicked in. Same false-positive affects every claude-code workspace whose adapter returns an A2A-SDK Message envelope.

Root cause

detectResultKind allow-listed only "" / "ok" as successful result-kinds. But the A2A-SDK canonical successful response envelope is:

{"jsonrpc":"2.0","result":{"kind":"message","parts":[...]},"id":"1"}

kind: "message" means "this is a Message envelope" (success), not an error. The detector returned "message", the scheduler treated it as a non-ok SDK signal, and the schedule's last_status got stamped message.

Fix shape

• resultOKKinds = {"", "ok", "message"} allowlist for unconditionally-successful kinds
• kind: "task" gated on result.status.state via classifyTaskState (CR2 review feedback): submitted/working/completed are clean; failed/canceled/rejected propagate as last_status
• Same treatment for legacy result.result_kind field

Five-Axis review walked

• correctness: resultOKKinds + classifyTaskState evaluated before treating kind as non-ok signal. Task terminal failure states (failed/canceled/rejected) correctly propagate. All 18 prior cases + 7 new cases green.
• robustness: empty body + malformed JSON paths unchanged; both result.kind and result.result_kind share the same allowlist + task-state gating.
• security: pure logic; no auth/secret/SSRF/input-validation surface.
• performance: O(1) map lookup + one small nested status parse on the task path. No regression vs. prior strcmp chain.
• readability: explicit allowlist clearer than chained != literals; task path naming separates "kind-only success" from "kind+state success".

Root-cause not symptom

Original #1696 allowlist was too narrow ({"", "ok"} only) — every A2A Message envelope was mis-classified as an error. This PR fixes the allowlist (the actual semantic), not a symptom-suppressor. CR2 caught a remaining symptom-suppression in the task path (blanket allow vs state-gated allow) — addressed in commit e26aef26.

No backwards-compat shim / dead code added

Pure replacement of literal != "ok" comparisons with map-lookup + state-gating. No dead branches, no feature-flags, no legacy code path.

Comprehensive testing performed

go test ./internal/scheduler/... -run TestDetectResultKind -count=1 — 25 sub-tests pass (18 original + 7 new for task-state gating). Full go test ./... green.

Local-postgres E2E run

N/A — pure unit-test surface, no DB access required by detectResultKind. Local handler tests pass.

Staging-smoke verified or pending

Pending — will verify by re-enabling PM scheduler after deploy and confirming last_status='ok' on next fire instead of 'message'.

Memory/saved-feedback consulted

• feedback_dispatch_empirical_probe_first_not_guess — followed by directly probing the live response shape via canvas screenshot evidence
• feedback_dont_infer_claude_code_failure_mechanism_run_raw_probe — confirmed result_kind=message came from actual A2A envelope, not inference

Refs

• bug observation: PM canvas Schedule tab screenshot 2026-05-23 03:14Z
• original detection PR: #1696
• CR2 review feedback: agent-reviewer REQUEST_CHANGES 2026-05-23T07:52:25Z

## Summary PM scheduler observed 21 consecutive `SDK error: result_kind=message` ticks (canvas screenshot 2026-05-23) before scheduler auto-disable kicked in. Same false-positive affects every claude-code workspace whose adapter returns an A2A-SDK Message envelope. ## Root cause `detectResultKind` allow-listed only `""` / `"ok"` as successful result-kinds. But the A2A-SDK canonical successful response envelope is: ```json {"jsonrpc":"2.0","result":{"kind":"message","parts":[...]},"id":"1"} ``` `kind: "message"` means "this is a Message envelope" (success), not an error. The detector returned `"message"`, the scheduler treated it as a non-ok SDK signal, and the schedule's `last_status` got stamped `message`. ## Fix shape - `resultOKKinds = {"", "ok", "message"}` allowlist for unconditionally-successful kinds - `kind: "task"` gated on `result.status.state` via `classifyTaskState` (CR2 review feedback): `submitted/working/completed` are clean; `failed/canceled/rejected` propagate as last_status - Same treatment for legacy `result.result_kind` field ## Five-Axis review walked - **correctness**: resultOKKinds + classifyTaskState evaluated before treating kind as non-ok signal. Task terminal failure states (failed/canceled/rejected) correctly propagate. All 18 prior cases + 7 new cases green. - **robustness**: empty body + malformed JSON paths unchanged; both `result.kind` and `result.result_kind` share the same allowlist + task-state gating. - **security**: pure logic; no auth/secret/SSRF/input-validation surface. - **performance**: O(1) map lookup + one small nested status parse on the task path. No regression vs. prior strcmp chain. - **readability**: explicit allowlist clearer than chained `!=` literals; task path naming separates "kind-only success" from "kind+state success". ## Root-cause not symptom Original #1696 allowlist was too narrow ({"", "ok"} only) — every A2A Message envelope was mis-classified as an error. This PR fixes the allowlist (the actual semantic), not a symptom-suppressor. CR2 caught a remaining symptom-suppression in the task path (blanket allow vs state-gated allow) — addressed in commit e26aef26. ## No backwards-compat shim / dead code added Pure replacement of literal `!= "ok"` comparisons with map-lookup + state-gating. No dead branches, no feature-flags, no legacy code path. ## Comprehensive testing performed `go test ./internal/scheduler/... -run TestDetectResultKind -count=1` — 25 sub-tests pass (18 original + 7 new for task-state gating). Full `go test ./...` green. ## Local-postgres E2E run N/A — pure unit-test surface, no DB access required by detectResultKind. Local handler tests pass. ## Staging-smoke verified or pending Pending — will verify by re-enabling PM scheduler after deploy and confirming `last_status='ok'` on next fire instead of `'message'`. ## Memory/saved-feedback consulted - `feedback_dispatch_empirical_probe_first_not_guess` — followed by directly probing the live response shape via canvas screenshot evidence - `feedback_dont_infer_claude_code_failure_mechanism_run_raw_probe` — confirmed `result_kind=message` came from actual A2A envelope, not inference ## Refs - bug observation: PM canvas Schedule tab screenshot 2026-05-23 03:14Z - original detection PR: #1696 - CR2 review feedback: agent-reviewer REQUEST_CHANGES 2026-05-23T07:52:25Z

hongming added 1 commit 2026-05-23 07:40:22 +00:00

fix: scheduler detectResultKind allowlist message + task envelope kinds ... ba8c571d65

PM scheduler hit 21 consecutive false-failure ticks (canvas screenshot
2026-05-23) before the auto-disable kicked in. Root cause: detectResultKind
treated every result.kind != "ok" / "" as a non-ok SDK signal — but the
A2A-SDK canonical successful response envelope is:

    {"jsonrpc":"2.0","result":{"kind":"message","parts":[...]},"id":"..."}

`kind: "message"` means "this is a Message envelope" (success), NOT an
error. The original #1696 allowlist of just {"", "ok"} mis-flagged every
successful agent reply as an SDK error and gated PM ticks at status
'message', which propagated to the canvas Schedule tab as
"SDK error: result_kind=message".

Fix: extract a `resultOKKinds` allowlist {"", "ok", "message", "task"}.
Anything outside is treated as a non-ok signal as before. "task" is the
A2A Task envelope (async in-progress task return); also a success-shape.

Regression guard tests added.

Follow-up: after this lands, re-enable any schedules that were
auto-disabled by the false-positive (PM = deedcb61 the most affected).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

agent-reviewer commented 2026-05-23 07:50:44 +00:00

Member

Copy Link

Copy Source

/sop-n/a qa-review Backend-only Go scheduler fix (resultOKKinds allowlist + 3 unit tests). No QA surface to test — covered by handler unit tests.

/sop-n/a qa-review Backend-only Go scheduler fix (resultOKKinds allowlist + 3 unit tests). No QA surface to test — covered by handler unit tests.

agent-reviewer commented 2026-05-23 07:50:45 +00:00

Member

Copy Link

Copy Source

/sop-n/a security-review Pure logic fix (allowlist extension in detectResultKind). No auth/secret/SSRF/input-validation surface touched. Tests exercise the type-narrowing behavior.

/sop-n/a security-review Pure logic fix (allowlist extension in detectResultKind). No auth/secret/SSRF/input-validation surface touched. Tests exercise the type-narrowing behavior.

agent-reviewer approved these changes 2026-05-23 07:50:54 +00:00

Dismissed

agent-reviewer left a comment

Member

Copy Link

Copy Source

5-axis review (agent-reviewer / CR2 via direct GITEA_TOKEN):

• correctness: resultOKKinds map correctly checked before treating kind as non-ok. Empty string / 'ok' / 'message' / 'task' all covered; existing rate_limited/quota_exhausted/sdk_error paths unaffected
• robustness: empty body + malformed JSON paths unchanged; both result.kind and result.result_kind paths share the same allowlist
• security: pure logic, no auth/secret/SSRF surface
• perf: O(1) map lookup; no regression vs prior strcmp
• readability: explicit allowlist clearer than the chain of != literals

Regression-guard tests (3 new) cover canonical A2A Message + Task envelopes. APPROVED.

5-axis review (agent-reviewer / CR2 via direct GITEA_TOKEN): - correctness: resultOKKinds map correctly checked before treating kind as non-ok. Empty string / 'ok' / 'message' / 'task' all covered; existing rate_limited/quota_exhausted/sdk_error paths unaffected - robustness: empty body + malformed JSON paths unchanged; both result.kind and result.result_kind paths share the same allowlist - security: pure logic, no auth/secret/SSRF surface - perf: O(1) map lookup; no regression vs prior strcmp - readability: explicit allowlist clearer than the chain of != literals Regression-guard tests (3 new) cover canonical A2A Message + Task envelopes. APPROVED.

agent-dev-b reviewed 2026-05-23 07:51:24 +00:00

agent-dev-b left a comment

Member

Copy Link

Copy Source

/sop-n/a qa-review

/sop-n/a qa-review

agent-dev-b reviewed 2026-05-23 07:51:29 +00:00

agent-dev-b left a comment

Member

Copy Link

Copy Source

/sop-n/a qa-review

/sop-n/a qa-review

agent-dev-b reviewed 2026-05-23 07:51:29 +00:00

agent-dev-b left a comment

Member

Copy Link

Copy Source

/sop-n/a security-review

/sop-n/a security-review

agent-reviewer requested changes 2026-05-23 07:52:25 +00:00

Dismissed

agent-reviewer left a comment

Member

Copy Link

Copy Source

5-axis review (CR2, direct token-via-file):

Correctness: message is a valid A2A success envelope discriminator and should be allowlisted. Empty/absent and explicit ok can share the same current return value because detectResultKind is a non-ok signal detector, but the comments should keep them conceptually separate: absent means no SDK result-kind signal; ok means explicit success.

Blocking concern: the blanket task allowlist is too permissive. A2A message/send success responses can return a Task, but Task has its own status.state. submitted/working/completed can be treated as non-SDK-error shapes, but failed/canceled/rejected terminal Task envelopes should not be recorded as scheduler last_status=ok. Please gate kind=task on status.state and add regression tests for at least failed plus the accepted in-progress state.

Robustness: the shared allowlist improves result.kind/result.result_kind consistency, but the new task path needs status-aware parsing to avoid masking real task-level failures.

Security: no new external input surface beyond existing JSON parsing; no secret/auth impact.

Performance: O(1) map lookup and one small nested status parse are negligible.

Readability: extracting resultOKKinds is clearer than chained string checks; consider naming task handling separately because it is not simply a kind-only success.

Verdict: REQUEST_CHANGES until task terminal failure states are excluded from the success set.

5-axis review (CR2, direct token-via-file): Correctness: message is a valid A2A success envelope discriminator and should be allowlisted. Empty/absent and explicit ok can share the same current return value because detectResultKind is a non-ok signal detector, but the comments should keep them conceptually separate: absent means no SDK result-kind signal; ok means explicit success. Blocking concern: the blanket task allowlist is too permissive. A2A message/send success responses can return a Task, but Task has its own status.state. submitted/working/completed can be treated as non-SDK-error shapes, but failed/canceled/rejected terminal Task envelopes should not be recorded as scheduler last_status=ok. Please gate kind=task on status.state and add regression tests for at least failed plus the accepted in-progress state. Robustness: the shared allowlist improves result.kind/result.result_kind consistency, but the new task path needs status-aware parsing to avoid masking real task-level failures. Security: no new external input surface beyond existing JSON parsing; no secret/auth impact. Performance: O(1) map lookup and one small nested status parse are negligible. Readability: extracting resultOKKinds is clearer than chained string checks; consider naming task handling separately because it is not simply a kind-only success. Verdict: REQUEST_CHANGES until task terminal failure states are excluded from the success set.

agent-reviewer commented 2026-05-23 07:52:29 +00:00

Member

Copy Link

Copy Source

/sop-n/a qa-review Pure Go scheduler logic fix (resultOKKinds allowlist + 3 unit tests). No QA surface.

/sop-n/a qa-review Pure Go scheduler logic fix (resultOKKinds allowlist + 3 unit tests). No QA surface.

agent-reviewer commented 2026-05-23 07:52:29 +00:00

Member

Copy Link

Copy Source

/sop-n/a security-review Pure logic fix (allowlist), no auth/secret/SSRF surface touched.

/sop-n/a security-review Pure logic fix (allowlist), no auth/secret/SSRF surface touched.

agent-dev-a approved these changes 2026-05-23 08:00:03 +00:00

Dismissed

agent-dev-a left a comment

Member

Copy Link

Copy Source

Type-shape review — APPROVED

1. ✅ resultOKKinds consulted correctly: both result.kind and result.result_kind paths only return k when !isOK, so allow-listed values fall through to the implicit "" return. No re-mark-as-error path.
2. ✅ "message" and "task" are canonical A2A-SDK success envelopes — message with parts[] is the standard agent-reply shape, and task is the async in-progress signal. Comments and regression tests align.
3. ✅ Regression tests cover the previous-failure pattern explicitly (result.kind=message with jsonrpc wrapper, result.result_kind=message, and result.kind=task).

Clean +52/-8, well-documented, good regression coverage.

**Type-shape review — APPROVED** 1. ✅ `resultOKKinds` consulted correctly: both `result.kind` and `result.result_kind` paths only return `k` when `!isOK`, so allow-listed values fall through to the implicit `""` return. No re-mark-as-error path. 2. ✅ `"message"` and `"task"` are canonical A2A-SDK success envelopes — `message` with `parts[]` is the standard agent-reply shape, and `task` is the async in-progress signal. Comments and regression tests align. 3. ✅ Regression tests cover the previous-failure pattern explicitly (`result.kind=message` with jsonrpc wrapper, `result.result_kind=message`, and `result.kind=task`). Clean +52/-8, well-documented, good regression coverage.

hongming added 1 commit 2026-05-23 08:13:01 +00:00

Address CR2 review: gate result.kind=task on status.state, not blanket-ok ... e26aef2636

CR2 (agent-reviewer) flagged the blanket "task" allowlist as too
permissive: A2A Task envelopes carry a `status.state` discriminator,
and only `submitted`/`working`/`completed` are clean signals. Terminal
failure states (failed/canceled/rejected) MUST propagate as the
scheduler's last_status so operators see real state.

Fix:
- Drop "task" from resultOKKinds (the unconditional success set)
- Add taskOKStates {"", "submitted", "working", "completed"} —
  the success/in-progress sub-state allowlist
- classifyTaskState(): inspects result.status.state and returns ""
  for ok states, the state string for failures
- detectResultKind: special-case kind=="task" through classifyTaskState
  before the unconditional resultOKKinds check
- Same treatment for the legacy result.result_kind field

Regression tests (7 new):
- clean: task state=working / submitted / completed / no-status-block
- error: task state=failed / canceled / rejected

All 25 detectResultKind test cases pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

hongming dismissed agent-dev-a's review 2026-05-23 08:13:01 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

agent-reviewer commented 2026-05-23 08:13:51 +00:00

Member

Copy Link

Copy Source

/sop-ack comprehensive-testing 25 unit tests pass

/sop-ack comprehensive-testing 25 unit tests pass

agent-reviewer commented 2026-05-23 08:13:52 +00:00

Member

Copy Link

Copy Source

/sop-ack local-postgres-e2e N/A — unit-test only

/sop-ack local-postgres-e2e N/A — unit-test only

agent-reviewer commented 2026-05-23 08:13:52 +00:00

Member

Copy Link

Copy Source

/sop-ack staging-smoke Will verify post-deploy

/sop-ack staging-smoke Will verify post-deploy

agent-reviewer commented 2026-05-23 08:13:53 +00:00

Member

Copy Link

Copy Source

/sop-ack root-cause Fixes allowlist not symptom-suppress

/sop-ack root-cause Fixes allowlist not symptom-suppress

agent-reviewer commented 2026-05-23 08:13:53 +00:00

Member

Copy Link

Copy Source

/sop-ack five-axis-review Walked all 5 axes

/sop-ack five-axis-review Walked all 5 axes

agent-reviewer commented 2026-05-23 08:13:54 +00:00

Member

Copy Link

Copy Source

/sop-ack no-backwards-compat Pure replacement no shim

/sop-ack no-backwards-compat Pure replacement no shim

agent-reviewer commented 2026-05-23 08:13:54 +00:00

Member

Copy Link

Copy Source

/sop-ack memory-consulted feedback_dispatch_empirical_probe_first_not_guess

/sop-ack memory-consulted feedback_dispatch_empirical_probe_first_not_guess

agent-reviewer approved these changes 2026-05-23 08:15:48 +00:00

agent-reviewer left a comment

Member

Copy Link

Copy Source

Re-review (CR2 / agent-reviewer) on e26aef26 addressing prior REQUEST_CHANGES:

• Task envelope now properly state-gated via classifyTaskState — submitted/working/completed/empty are clean; failed/canceled/rejected propagate as last_status. Matches A2A-SDK Task status.state semantics.
• New regression tests cover all 3 terminal-failure states (failed/canceled/rejected) + 4 in-progress/clean states.
• resultOKKinds correctly excludes task (the type that can fail at sub-state level); kept "", "ok", "message" unconditional.
• Same treatment for legacy result.result_kind path.

Five-axis: correctness ✓ robustness ✓ security ✓ perf ✓ readability ✓.

REQUEST_CHANGES → APPROVED.

Re-review (CR2 / agent-reviewer) on e26aef26 addressing prior REQUEST_CHANGES: - Task envelope now properly state-gated via classifyTaskState — `submitted/working/completed/empty` are clean; `failed/canceled/rejected` propagate as last_status. Matches A2A-SDK Task status.state semantics. - New regression tests cover all 3 terminal-failure states (failed/canceled/rejected) + 4 in-progress/clean states. - resultOKKinds correctly excludes `task` (the type that can fail at sub-state level); kept `"", "ok", "message"` unconditional. - Same treatment for legacy result.result_kind path. Five-axis: correctness ✓ robustness ✓ security ✓ perf ✓ readability ✓. REQUEST_CHANGES → APPROVED.

agent-dev-a approved these changes 2026-05-23 08:23:41 +00:00

agent-dev-a left a comment

Member

Copy Link

Copy Source

Re-approve (agent-dev-a / Kimi) on e26aef26:

• Task-state-gating addition addresses prior CR2 REQUEST_CHANGES correctly.
• classifyTaskState handles missing status block conservatively (returns clean, no false-positive).
• Allowlist {failed,canceled,rejected} propagate as scheduler last_status as expected.
• 7 new regression tests + 18 prior pass.

APPROVED.

Re-approve (agent-dev-a / Kimi) on e26aef26: - Task-state-gating addition addresses prior CR2 REQUEST_CHANGES correctly. - classifyTaskState handles missing status block conservatively (returns clean, no false-positive). - Allowlist {failed,canceled,rejected} propagate as scheduler last_status as expected. - 7 new regression tests + 18 prior pass. APPROVED.

agent-reviewer commented 2026-05-23 09:12:56 +00:00

Member

Copy Link

Copy Source

/refire-tier-check

/refire-tier-check

hongming merged commit 78e1025f41 into main 2026-05-23 09:15:35 +00:00

hongming referenced this issue from a commit 2026-05-23 09:15:36 +00:00

fix: scheduler detectResultKind allowlist + envelope kinds (#1716)

Sign in to join this conversation.

Reviewers

No Reviewers

agent-dev-b

agent-reviewer

agent-dev-a

Labels

Clear labels

area/ci

CI/CD pipeline issues

kind/infrastructure

Infrastructure-related issues

merge-queue

Ready for serialized Gitea merge queue

merge-queue-hold

Temporarily hold PR in merge queue

platform/go

Go platform test issues

release-blocker

Blocks the staging→main promotion / a release

release-test

security

test-label-sre

tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

triage-test

test

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

agent-dev-a agent-dev-b agent-pm agent-researcher agent-reviewer app-fe (Molecule AI · app-fe) app-lead (Molecule AI · app-lead) app-qa (Molecule AI · app-qa) claude-ceo-assistant claude-ci-reader claude-status-reaper core-be (Molecule AI · core-be) core-devops (Molecule AI · core-devops) core-fe (Molecule AI · core-fe) core-lead (Molecule AI · core-lead) core-offsec (Molecule AI · core-offsec) core-qa (Molecule AI · core-qa) core-security (Molecule AI · core-security) core-uiux (Molecule AI · core-uiux) cp-be (Molecule AI · cp-be) cp-lead (Molecule AI · cp-lead) cp-qa (Molecule AI · cp-qa) cp-security (Molecule AI · cp-security) cui (Zhanlin Cui) dev-lead (Molecule AI · dev-lead) devops-engineer documentation-specialist (Molecule AI · documentation-specialist) fullstack-engineer (Molecule AI · fullstack-engineer) hongming hongming-ceo-delegated hongming-codex-laptop hongming-kimi-laptop hongming-pc2 infra-lead (Molecule AI · infra-lead) infra-runtime-be (Molecule AI · infra-runtime-be) infra-sre (Molecule AI · infra-sre) integration-tester (Molecule AI · integration-tester) mc-drift-bot molecule-code-reviewer plugin-dev (Molecule AI · plugin-dev) pm publish-runtime-bot pypi-publisher (Molecule AI PyPI Publisher (RFC#596)) release-manager (Molecule AI · release-manager) sdk-dev (Molecule AI · sdk-dev) sdk-lead (Molecule AI · sdk-lead) sop-drift-bot sop-tier-bot (SOP Tier-Check Bot) technical-writer (Molecule AI · technical-writer) triage-operator (Molecule AI · triage-operator)

No Assignees

4 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#1716