Skip to content

emrg: fix GUI task save no-response — wire type field collision - #793

Merged
argszero merged 2 commits into
masterfrom
feature/gui-task-wire-type-fix
Aug 14, 2026
Merged

emrg: fix GUI task save no-response — wire type field collision#793
argszero merged 2 commits into
masterfrom
feature/gui-task-wire-type-fix

Conversation

@argszero

Copy link
Copy Markdown
Owner

Fixes rant 2026-08-14T21:48:00: GUI task management save appears to do nothing (form stays open, no list refresh, no visible error) in v0.2.37.

Root cause

daemon_client.js sendCommand built wire frames as { type, ...params }. The task CRUD payload carries a type field (the task type, e.g. "evolution"), which spread over the wire message type — the daemon then routed "evolution" as the message type and replied unknown message type. The error went to the chat area, which is hidden while the tasks panel is open (view exclusivity) → appears to be no response.

Changes

  1. emrg/gui/daemon_client.jssendCommand: { type, ...params }{ ...params, type } (wire message type last, never overridden by a payload field; generic defense for this whole bug class).
  2. emrg/gui/main.jsemrg:taskCreate and emrg:taskUpdate send the task type as task_type (never occupying the message-type field name).
  3. emrg/server/daemon.pytask_create reads task_type (never msg["type"], which is the wire message type after fix 1); task_update maps task_typetype for scheduler.task_update (its internal name is unchanged).
  4. Tests — +1 daemon_client frame-shape regression (payload type must not override wire type); +3 wire e2e tests (task_create reads task_type / missing task_type never falls back to the wire type / task_update maps task_type to type). Agent.md counts 806→809, GUI 246→247.

Verification

  • pytest 809 passed (incl. 3 new wire e2e)
  • GUI npm test 247 passed (incl. 1 new frame-shape test)
  • import check + CLI check green

Rant 2026-08-14T21:48:00: GUI task management save appeared to do
nothing (form stays open, no list refresh). Root cause: daemon_client.js
sendCommand built frames as { type, ...params } — the task CRUD payload
carries a "type" field (the task type, e.g. "evolution") which spread
over the wire message type, so the daemon routed "evolution" as the
message type and replied "unknown message type". The error was written
to the chat area while the tasks panel is open (view exclusivity), so it
looked like no response.

Fixes:
- daemon_client.js sendCommand: { ...params, type } — wire message type
  last, never overridden by payload fields (generic defense)
- main.js taskCreate/taskUpdate: send the task type as "task_type"
  (never occupying the message-type field name)
- daemon.py task_create reads task_type; task_update maps task_type to
  type for scheduler.task_update (internal name unchanged)
- tests: +1 daemon_client frame-shape regression (payload type must not
  override wire type); +3 wire e2e tests (task_create reads task_type /
  missing task_type never falls back to wire type / task_update maps
  task_type to type); Agent.md counts 806->809, GUI 246->247

@argszero argszero 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.

✅ LGTM — cycle: sendCommand frame order fix verified ({ ...params, type } keeps wire type authoritative), taskCreate/taskUpdate send task_type, daemon task_create reads task_type (negative e2e proves no fallback to wire type) and task_update maps task_type→type for scheduler. Local pytest 809 + GUI 247 + import + CLI green; CI test + test-windows pass.

@pm25coder

Copy link
Copy Markdown
Contributor

I tested this PR end-to-end at head 3fb4f94 and the fix holds up. Findings:

Verification (first-hand, Windows host)

  • uv run pytest tests/ -q: 809 collected — 749 passed, 60 skipped, 0 failed (matches the Agent.md 809 update)
  • cd emrg/gui && npm test: 247/247 pass, 0 fail (matches the 246→247 + daemon_client 44→45 update)
  • Call-site coverage grep: only main.js:628 (taskCreate) and main.js:641 (taskUpdate) send task CRUD; both are updated to task_type. No other sendCommand caller in emrg/gui passes a payload field named type, so the reorder cannot silently drop data anywhere else.

Code review

  • Root cause and fix are correct: JSON.stringify({ ...params, type }) guarantees the wire message type always wins via duplicate-key-last-wins, and renaming the payload field to task_type prevents the payload value from being lost in that collision. The daemon reads task_type on create and maps task_typetype for the scheduler's internal field on update — consistent with scheduler.py's create_task/update_task signatures.
  • The e2e tests are well-designed: test_task_create_reads_task_type (positive), test_task_create_missing_task_type_not_read_from_wire_type (negative — discriminative against the exact "fall back to reading the wire type" bug class), and test_task_update_maps_task_type_to_type (asserts task_type not leaked into scheduler fields).

Non-blocking notes

  1. With the new ordering, any future payload field literally named type is silently dropped (duplicate-key semantics). The task_type rename pattern is now the required convention for commands with a type-like payload field — the added comment at sendCommand covers this, worth keeping in mind for future GUI commands.
  2. daemon_client.test.js:720 still passes type: "evolution" as a payload to sendCommandAndWait("task_create", ...) — harmless in the mocked harness (responses are mocked), and it documents the old payload shape; optional to migrate to task_type for consistency.
  3. Backward compatibility: a pre-fix GUI client sending type for task_create now yields task_type="" → validation error rather than silent no-response. Acceptable, since the old frame never routed to task_create at all (that was the bug) — no working path regresses.

CI was pending (run 31806929663) at test time — local suites are green; watching the checks complete.

@argszero argszero 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.

✅ LGTM — cycle (2nd ✅)

Full review this cycle (head 3fb4f94, diff audited):

  1. Root cause sound: sendCommand built frames as { type, ...params } — a payload type field (task type, e.g. "evolution") spread over the wire message type → daemon routed "evolution" → "unknown message type" → error went to hidden chat area while tasks panel open → "no response".
  2. Fix is defense-in-depth across the whole class: { ...params, type } guarantees the wire message type always wins (generic); task_create/parsed as task_type; task_update maps task_typetype for scheduler (whose internal field name is type, verified at scheduler.py:1427-1435); GUI main.js sends task_type for both create + update.
  3. No collateral: grep of all sendCommand/sendCommandAndWait callers — none pass type: in payload (only main→renderer event frames use type:, different direction, not via sendCommand). Scheduler task_update reads fields.get("type") — mapping correct.
  4. Tests discriminative: +1 GUI frame-shape test (wire type preserved, KeyError-style regression if reverted); +3 wire e2e (reads task_type / missing task_type never falls back to wire type / task_type mapped to type). 809 pytest + 247 npm pass locally; import + CLI OK; CI test+test-windows green; Agent.md counts synced (806→809, 246→247).

MERGEABLE/CLEAN.

@argszero argszero 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.

✅ LGTM — cycle (3rd consecutive ✅, R1781)

Conflict resolved this cycle (Agent.md only): master advanced with #792 (which added 1 test) while #793 was open — both touched the test-count line. Merged master into the branch (9371a65) and corrected the count to the true collected value: 810 pytest (806 base + 1 from #792 + 3 from #793) + 247 GUI. Verified on the merged tree:

  • Full suite 810 passed locally (13.97s)
  • Import check + emrg --help OK
  • No conflict markers remain; GUI 247 pass re-confirmed
  • PR CI re-running on the merged head (test + test-windows)

Original R1780 review stands: root cause sound ({ type, ...params } spread allowed payload type to override wire message type), fix defense-in-depth ({ ...params, type } wire-type-last + task_type wire contract + daemon task_type→type mapping verified vs scheduler.py:1427-1435), no collateral (grep clean), tests discriminative.

3 consecutive ✅ from different cycles, no ❌ between — MERGEABLE.

@argszero
argszero merged commit 70d2209 into master Aug 14, 2026
2 checks passed
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.

2 participants