Let a plan name one task twice, and number its nodes - #738
Conversation
A customer plan may want one catalogue task under two scenes. `EvalPlan` refused it: `_each_task_appears_once` held the task list to unique ids, which is a rule about our report rather than about what the platform can run. The rule goes. In its place `_number_the_nodes` stamps each node with its index, which is what identifies a node when `task_id` cannot. `TaskNode.position` is excluded from the wire: a caller states the order by writing the list, and the index overwrites anything sent. The coordinator keys its tables on that position already, so the two agree on what a node is. Client 0.7.0 -> 0.8.0, with the root pin and the lock. Ticket: none — client-side plan model; the coordinator half is its own PR.
Cut the four-line field comment to the one fact a reader acts on, and drop the two clefts and the rationale the PR body already carries. Ticket: none — a prose pass on the commit before it.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 289520d9b8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # identifies the node, so a plan naming one task twice keys its two nodes apart; `task_id` alone | ||
| # cannot. Excluded from the wire: a caller states the order by writing the list, and a value it | ||
| # sent would be overwritten by the index anyway. | ||
| position: int = Field(default=0, ge=0, exclude=True) |
There was a problem hiding this comment.
Rule earn-its-place violated:
TaskNode.position duplicates the index already encoded by EvalPlan.tasks; because these Pydantic models and their task lists remain mutable, reversing, inserting into, or appending to tasks after validation leaves the stored positions stale and can key coordinator records to the wrong nodes. Derive the position with enumerate() when filing nodes, or encapsulate all list mutation so numbering is recomputed.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| asking for two scenes of one task is asking for something the platform runs. | ||
| """ | ||
| for position, task in enumerate(self.tasks): | ||
| task.position = position |
There was a problem hiding this comment.
Rule hidden-dependency violated:
_number_the_nodes assumes every list element is an independently owned TaskNode, but Pydantic retains already-constructed model instances: EvalPlan(tasks=[node, node], ...) therefore assigns through the same object twice and leaves both entries at position 1, while using a node in a later plan can silently rewrite its position in the earlier plan. Number distinct copies or keep the index outside the mutable node.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
1 participant
A customer plan may want one catalogue task under two scenes — the same task bare, and again with
clutter on the table.
EvalPlanrefused that:_each_task_appears_onceheld the task list tounique ids.
That rule was about our report rather than about what the platform can run, so it goes. Nothing
replaces it: a caller asking for two nodes of one task is asking for something the rig executes
fine, and the platform is not the place to tell them they meant something else.
The position is what identifies a node
With ids no longer unique,
task_idcannot say which node a row belongs to._number_the_nodesstamps each node with its index in the plan's list, and that index is the identity everywhere a
node is filed.
TaskNode.positionis excluded from the wire. A caller states the order by writing the list, sothere is nothing for them to send; a value that arrived anyway is overwritten by the index, which is
the only answer that can be consistent with where the node actually sits. A test pins both halves.
Why here rather than at the coordinator
The coordinator keys its own tables on a node's position already (
endpoints,cascade,episodes,runs), and it composes this class rather than defining a parallel one. Numbering thenodes where they are declared gives the two one answer to "which node is this"; numbering them at
the far end would give a client whose plan validates a request the coordinator then reads
differently.
Verification
client/— 414 passed, including three new tests: one task named twice keeps both nodes, threenodes number 0/1/2, and a position a caller states is replaced by the list order and never
serialised.
Client
0.7.0->0.8.0, with the root pin and the lock.