Task data model and TaskManager queue #84

Merged

liamjd merged 3 commits from 66-task-model-and-manager into main

2026-09-11 07:49:34 +01:00

liamjd commented

2026-09-11 07:44:00 +01:00

Owner

The generic task data model and the queue that selects from it. Pure logic with no node and no rendering, which is what lets the whole thing be proven headless long before a pawn can walk.

Closes #66
Closes #67

What is here

Task extends Entity is an abstract RefCounted record: id, priority, creation sequence, status, parent and children links, target cell, assignee and work remaining. Four abstract hooks (can_start, work, on_complete, on_cancel) are the only thing a new task type has to implement, which is #66's promise that adding a type needs no change to the manager.

transition_to() is concrete on the base class rather than abstract, and it validates against a fixed table. A ConstructionTask and a HaulTask differ in what work means, never in whether COMPLETE can return to PENDING, so the rules must not vary by subclass.

TaskManager owns the queue, ids, selection and the lifecycle calls. next_for(pawn_id) orders by priority and then by age, skips tasks whose can_start() is false and marks them BLOCKED, and re-evaluates blocked tasks on the next pass so the queue does not silt up.

Decisions worth recording

claim() lands on CLAIMED, not IN_PROGRESS. The two are separate states because a claim can be cancelled before work ever begins, and because a pawn spends the walk owning a task it is not yet working. start(id) is the step for "the pawn arrived". Issue #75's work loop needs to tell those apart.

The manager takes pawn ids, not Pawn objects. Keeps it independent of the scene tree, which matters because #77 renames that node to PawnView. The same reasoning puts an int in Task.assignee.

Priority resolution goes through one comparator. The design docs require an Emergency Protocols override that reprioritises all labour in one action. That should become a swap of the ordering policy rather than a rewrite of selection.

Lower priority number wins. Issue #67's body says the opposite and is wrong; task.gd is the source of truth.

claim(), start() and release() return Error. They previously returned Task.Status, where the failure value was a real, plausible status and indistinguishable from a legitimate result. release() was worst: PENDING meant both success and unknown id. Godot's Error enum is the native idiom and carries a reason. Adding an INVALID member to Status was rejected because that enum describes what a task is, and a member describing what a call did would be assignable to task.status.

Bugs found by review, not by the tests

Both were fixed, and both now have a test.

An in-progress task was offered to a second pawn. The selection loop skipped CANCELLED and CLAIMED but not IN_PROGRESS, so a task already being worked was offered to the next idle pawn, and the loop overwrote its status on the way past, silently undoing the first pawn's start().

The age tiebreak was never applied. The comparator read priority alone. Array.sort_custom() is an introsort and is not stable, so equal-priority tasks came back in whatever order the partitioning left them. The original two-task test passed and proved nothing: introsort drops to insertion sort below sixteen elements, and insertion sort leaves an already-ordered run alone. The replacement queues twenty-four tasks, which is past that threshold.

Both status writes in the selection loop now go through transition_to() rather than assigning the field, so the validated lifecycle has no bypass.

Verification

Full GUT suite run headless, three consecutive times, deterministic:

Scripts              11
Tests               236
Passing Tests       236

Of those, 18 cover the lifecycle table and 33 cover the manager. No orphans, no leaked nodes.

Tests were written before the implementation. Worth recording what that taught, because it is not the same as in a statically compiled language: a call to a method that does not exist is a runtime error in GDScript, so the script still loads and you get honest failures. A call to a method that does exist with a different signature is a parse error, so GUT discards the entire script and reports zero tests. Stubbing a method with the wrong signature is worse than not writing it at all.

Known and deliberately left

Task.parent and Task.children form a reference cycle. Both are RefCounted and Godot reference-counts without cycle detection, so no task in a parent-child tree is ever freed. Nothing builds trees yet, so no test sees it, and orphan counting only tracks Nodes. Issue #68 is where it starts to matter. The fix is the choice already made for assignee: store parent_id and resolve through the manager.

Sibling tasks get the same creation sequence. add() derives a child's sequence from its parent plus one, so two children of one parent tie with each other and can collide with an unrelated task. Marked with a TODO; #68 owns it.

by_status() always reports an empty CANCELLED bucket, since cancel() erases the record. Harmless, but #76's debug panel will show a permanently empty group.

next_for() ignores its pawn_id. Specialisation weighting is #69.

The context object is still a bare Dictionary. #66 left its shape open; it wants a real type once there is a resource system to put in it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KqCxgzSMS7oexpR6tsK5Wc

The generic task data model and the queue that selects from it. Pure logic with no node and no rendering, which is what lets the whole thing be proven headless long before a pawn can walk. Closes #66 Closes #67 ## What is here `Task extends Entity` is an abstract RefCounted record: id, priority, creation sequence, status, parent and children links, target cell, assignee and work remaining. Four abstract hooks (`can_start`, `work`, `on_complete`, `on_cancel`) are the only thing a new task type has to implement, which is #66's promise that adding a type needs no change to the manager. `transition_to()` is concrete on the base class rather than abstract, and it validates against a fixed table. A `ConstructionTask` and a `HaulTask` differ in what work means, never in whether COMPLETE can return to PENDING, so the rules must not vary by subclass. `TaskManager` owns the queue, ids, selection and the lifecycle calls. `next_for(pawn_id)` orders by priority and then by age, skips tasks whose `can_start()` is false and marks them BLOCKED, and re-evaluates blocked tasks on the next pass so the queue does not silt up. ## Decisions worth recording **`claim()` lands on CLAIMED, not IN_PROGRESS.** The two are separate states because a claim can be cancelled before work ever begins, and because a pawn spends the walk owning a task it is not yet working. `start(id)` is the step for "the pawn arrived". Issue #75's work loop needs to tell those apart. **The manager takes pawn ids, not `Pawn` objects.** Keeps it independent of the scene tree, which matters because #77 renames that node to `PawnView`. The same reasoning puts an int in `Task.assignee`. **Priority resolution goes through one comparator.** The design docs require an Emergency Protocols override that reprioritises all labour in one action. That should become a swap of the ordering policy rather than a rewrite of selection. **Lower priority number wins.** Issue #67's body says the opposite and is wrong; `task.gd` is the source of truth. **`claim()`, `start()` and `release()` return `Error`.** They previously returned `Task.Status`, where the failure value was a real, plausible status and indistinguishable from a legitimate result. `release()` was worst: PENDING meant both success and unknown id. Godot's `Error` enum is the native idiom and carries a reason. Adding an INVALID member to `Status` was rejected because that enum describes what a task *is*, and a member describing what a *call did* would be assignable to `task.status`. ## Bugs found by review, not by the tests Both were fixed, and both now have a test. **An in-progress task was offered to a second pawn.** The selection loop skipped CANCELLED and CLAIMED but not IN_PROGRESS, so a task already being worked was offered to the next idle pawn, and the loop overwrote its status on the way past, silently undoing the first pawn's `start()`. **The age tiebreak was never applied.** The comparator read `priority` alone. `Array.sort_custom()` is an introsort and is not stable, so equal-priority tasks came back in whatever order the partitioning left them. The original two-task test passed and proved nothing: introsort drops to insertion sort below sixteen elements, and insertion sort leaves an already-ordered run alone. The replacement queues twenty-four tasks, which is past that threshold. Both status writes in the selection loop now go through `transition_to()` rather than assigning the field, so the validated lifecycle has no bypass. ## Verification Full GUT suite run headless, three consecutive times, deterministic: ``` Scripts 11 Tests 236 Passing Tests 236 ``` Of those, 18 cover the lifecycle table and 33 cover the manager. No orphans, no leaked nodes. Tests were written before the implementation. Worth recording what that taught, because it is not the same as in a statically compiled language: a call to a method that **does not exist** is a runtime error in GDScript, so the script still loads and you get honest failures. A call to a method that **does exist with a different signature** is a parse error, so GUT discards the entire script and reports zero tests. Stubbing a method with the wrong signature is worse than not writing it at all. ## Known and deliberately left **`Task.parent` and `Task.children` form a reference cycle.** Both are `RefCounted` and Godot reference-counts without cycle detection, so no task in a parent-child tree is ever freed. Nothing builds trees yet, so no test sees it, and orphan counting only tracks `Node`s. Issue #68 is where it starts to matter. The fix is the choice already made for `assignee`: store `parent_id` and resolve through the manager. **Sibling tasks get the same creation sequence.** `add()` derives a child's sequence from its parent plus one, so two children of one parent tie with each other and can collide with an unrelated task. Marked with a TODO; #68 owns it. **`by_status()` always reports an empty CANCELLED bucket**, since `cancel()` erases the record. Harmless, but #76's debug panel will show a permanently empty group. **`next_for()` ignores its `pawn_id`.** Specialisation weighting is #69. **The context object is still a bare Dictionary.** #66 left its shape open; it wants a real type once there is a resource system to put in it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01KqCxgzSMS7oexpR6tsK5Wc

liamjd added 3 commits

2026-09-11 07:44:00 +01:00

Started implementation of the base Task class and outlined the TaskManager class. Claude wrote unit tests for both. 09fa0f1607

Implemented the Task.transition_to method, validating the plausibility of a status transition (e.g. can't go from PENDING to COMPLETE).

Implemented most of the basic task manager functions, except those which need to refer to pawns or the task context. 7 failing tests remain. b2c3c0a98c

Complete the TaskManager queue and fix task selection ordering 2b0983b633

Finishes the remaining seven failing tests and the issues found reviewing
them. All 236 tests in the suite pass.

Selection:
- next_for() sorts through a named comparator that orders by priority and
  then by creation_seq. Array.sort_custom() is an introsort and is not
  stable, so equal-priority tasks came back in partition order and the age
  rule was never actually applied. A two-task test could not catch this:
  introsort drops to insertion sort below sixteen elements, which leaves an
  ordered run alone. The new test queues twenty-four.
- next_for() now skips IN_PROGRESS. A task being worked was previously
  offered to the next idle pawn, and the loop overwrote its status on the
  way past, undoing the first pawn's start().
- Both status writes in the loop go through transition_to() rather than
  assigning the field, so the validated lifecycle has no bypass.

Lifecycle and storage:
- claim() records the assignee; release() and cancel() clear it.
- cancel() erases the task; complete() archives it in _completed and erases
  it from _tasks. Neither dictionary grew without bound before, but _tasks
  was a record of every task ever created and was rescanned on every
  selection pass.
- start() and release() return Error alongside claim(), replacing a
  Task.Status return whose failure value was indistinguishable from a
  legitimate PENDING result.

Task gains _to_string() so GUT failure messages name the task rather than
printing a RefCounted id.

Known and deliberately left: Task.parent and Task.children form a
reference cycle between RefCounted objects, which Godot does not collect,
so no task in a parent-child tree is freed. Nothing builds trees yet; #68
is where it starts to matter.

Closes #66
Closes #67

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KqCxgzSMS7oexpR6tsK5Wc