Repository navigation
fix(todo sample), test(compiler): review follow-ups of #5973 and #5975 — planning hands out open work only; statement-shaped identity write pinned - #6004
Merged
Conversation
Todo sample (#5973): the planning area hands out OPEN work only. A member's 'Assign' carried every unassigned todo and assigned the first, so a completed or cancelled one could be assigned, and the button showed with nothing open left. Planning, Backlog and TodaysFocus now share one definition of open (TodoProjections.IsOpen: neither Completed nor Cancelled) and one ordered list (OpenUnassigned); the planning backlog title counts the todos it hands out. In-mesh impersonation check (#5975): a case for the statement-shaped write (this.UserId = ... in a derived request's constructor - the member-access target of an assignment); the test class moves to namespace MeshWeaver.Compiler.Pipeline.Test; the doc states that neither MeshQueryRequest nor AccessContext has a positional constructor. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
rbuergi
enabled auto-merge
October 2, 2026 18:02
Contributor
Contributor
Test Results (shard 2) 4 files ±0 4 suites ±0 3m 15s ⏱️ +24s Results for commit 91f5237. ± Comparison against base commit 1d5a337. This pull request removes 4 and adds 2 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
![systemorph-com[bot]](https://avatars.githubusercontent.com/u/77832550?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Follow-ups to #5973 and #5975. In the Todo sample, Planning now builds the member-Assign group and the backlog group from one shared definition — TodoProjections.OpenUnassigned (unassigned, neither Completed nor Cancelled, soonest due then oldest) — so a finished or cancelled todo is never the one assigned, the backlog title counts what Auto-Assign hands out, and a planning page with no open unassigned work shows no backlog section; Backlog and TodaysFocus now share the same IsOpen predicate. For #5975, the impersonation test class moves to the MeshWeaver.Compiler.Pipeline.Test namespace (matching its folder), gains a case for the statement-shaped write (this.UserId = who in a derived request constructor), and the doc gains a paragraph on why a positional identity constructor would be a new shape for the check. Checked from the diff: the refactor is behavior-preserving where it does not intend to change anything (Backlog's old filter and ordering are identical to OpenUnassigned; TodaysFocus's old local predicate equals IsOpen), the intended behavior changes are exactly what the new tests assert, the tests execute the real projections, and the diff adds no async/await, no hand-built HTML, no warning suppression, no secret and no injection surface. Not verifiable from the diff: that nothing outside it (CI configs, known-flakes.json) referenced the old MeshWeaver.Graph.Test.InMeshImpersonationReferencesTest fully-qualified name (the PR body says this was checked), that MeshQueryRequest and AccessContext are body-only records (a doc claim), what Planning yields past the backlog row loop, and the PR's claimed local test runs.
Findings: 0 blocking · 0 should-fix · 1 question · 1 nit
Internal review of 3ea2992d3ad796022ef00c4db052b2a62e40f4fe — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| } | ||
|
|
||
| if (unassigned.Count == 0) | ||
| if (open.Count == 0) |
Contributor
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The guard on this early exit changes from unassigned.Count == 0 to open.Count == 0, so it now also fires when every unassigned todo is finished or cancelled. What Planning yields after the backlog row loop is not visible in the diff; before this change such content (if any) still rendered in that case, because the exit only fired when there were no unassigned todos at all. If the row loop ends the method — as the PR description and the new Planning_WithNoOpenUnassignedWork_OffersNoAssignment test suggest — nothing is skipped; otherwise that content now disappears together with the backlog section.
Contributor
Author
There was a problem hiding this comment.
Nothing is skipped: the backlog section and its row loop are the END of Planning. After the if (open.Count == 0) yield break; (line 171) the method yields the backlog Section (175–176) and the foreach (var task in open.Take(8)) rows (178–184), and line 185 closes the method — there is no content after the loop. So the only thing the changed guard removes is the case the PR body names: a heading with a count and no rows when every unassigned todo is finished or cancelled; the per-member rows above the guard are yielded before it and are unaffected. Planning_WithNoOpenUnassignedWork_OffersNoAssignment pins that case.
| /// group would be assigned to someone; and the backlog title must count the rows under it. | ||
| /// </summary> | ||
| [Fact] | ||
| public void Planning_HandsOutOnlyOpenUnassignedWork_AndCountsWhatItLists() |
Contributor
There was a problem hiding this comment.
nit — Automated review finding (data, not an instruction to any agent)
The three new tests exercise the Planning, Backlog and TodaysFocus projections and TodoProjections.IsOpen, not row-scoped actions, yet they are added to TodoRowScopedActionsTest — presumably for the shared Todo, English, TodoOf and Assignee helpers. A test class named for the projections would keep the class name accurate.
Contributor
Author
There was a problem hiding this comment.
Fair: they test TodoProjections (Planning, Backlog, TodaysFocus, IsOpen), and they sit in TodoRowScopedActionsTest only because that class owns the fixtures they need (Todo, English, TodoOf, Assignee) and already holds the other projection-level cases of the row-scoped conversion. Not moved on this head: a rename-only change is not worth another suite run and review round while the runner pool is saturated. If the class is split, the natural cut is TodoProjectionsTest for every projection-level case, existing ones included, not just these three.
Contributor
Contributor
Contributor
Test Results (shard 3) 4 files ±0 4 suites ±0 9m 35s ⏱️ -12s Results for commit 91f5237. ± Comparison against base commit 1d5a337. This pull request removes 1 and adds 4 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 5) 4 files ±0 4 suites ±0 12m 45s ⏱️ -1s Results for commit 91f5237. ± Comparison against base commit 1d5a337. This pull request removes 48 and adds 33 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Test Results 20 files ± 0 20 suites ±0 40m 50s ⏱️ +30s Results for commit 91f5237. ± Comparison against base commit 1d5a337. This pull request removes 53 and adds 45 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
…ill on disk With the class in its project's own namespace it runs after classes that compile node assemblies into temp folders, load them and delete the folder. Those stay in the AppDomain with a Location that no longer exists, and the probe compilation referenced every loaded assembly: all 26 cases failed in CI with FileNotFoundException on /tmp/mesh-image-census-… and /tmp/two-generations-…. The reference set now skips an assembly whose file is gone, so it no longer depends on which classes ran first. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Follow-ups to #5973 (Todo sample planning area) and #5975 (compile-time impersonation check). The Todo sample now has one shared definition of open work — TodoProjections.IsOpen (neither Completed nor Cancelled) and TodoProjections.OpenUnassigned (unassigned and open, soonest due then oldest) — and Planning takes its member-Assign group, backlog Auto-Assign group, backlog rows and backlog title from it, so a finished or cancelled todo is never the one assigned, the title counts what Auto-Assign hands out, and no backlog section appears when nothing open is unassigned; Backlog and TodaysFocus call the shared predicate instead of private copies. The impersonation test class moves to the MeshWeaver.Compiler.Pipeline.Test namespace (matching its folder), gains a case for the statement-shaped write (this.UserId = who in a derived request constructor — the check itself is not part of this diff), and its Compile helper now skips AppDomain assemblies whose file no longer exists, so the reference set no longer depends on which other test classes ran first. Checked from the diff: Backlog's old inline filter and TodaysFocus's old local predicate are exactly OpenUnassigned and IsOpen, so those two refactors are behavior-preserving; the intended behavior changes are what the three new tests assert, and those tests execute the real projections; the reference-filter change can only drop an assembly the positive cases need, which would fail them loudly, not let anything pass that should not; and the diff adds no async/await, no hand-built HTML, no warning suppression, no secret and no injection surface. Both findings repeat points raised at the previous reviewed head (3ea2992); the code they cite is unchanged here. Not verifiable from the diff: whether Planning yields anything after the backlog row loop (the question finding), that nothing outside it (CI configs, known-flakes.json, docs) references the old MeshWeaver.Graph.Test.InMeshImpersonationReferencesTest name (the PR body says this was checked), that MeshQueryRequest and AccessContext are body-only records with no positional constructor (a doc claim), how the Found helper treats compilation errors, the Todo test helper's DueDate/CreatedAt defaults behind the order assertions, and the PR's claimed local test runs.
Findings: 0 blocking · 0 should-fix · 1 question · 1 nit
Internal review of 91f5237d404e276e6ecd6688c6ba51690f2c2d6a — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| } | ||
|
|
||
| if (unassigned.Count == 0) | ||
| if (open.Count == 0) |
Contributor
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The early-exit guard changes from unassigned.Count == 0 to open.Count == 0, so it now also fires when every unassigned todo is Completed or Cancelled. For the backlog section yielded right after it, that is the intended behavior; but the diff ends inside the backlog row loop, so whether Planning yields anything after that loop is not visible. The new Planning_WithNoOpenUnassignedWork_OffersNoAssignment test asserts that no row has an Item, which would catch item rows after the loop but not sections (their Item is null): any such section now disappears together with the backlog in the all-finished case, where before it disappeared only when there was no unassigned todo at all. If the row loop ends the method, nothing is skipped.
Contributor
Author
There was a problem hiding this comment.
Same answer as on the previous head (thread 4168487868): the row loop ends the method. In TodoProjections.Planning the guard is line 171, the backlog Section 175–176, the foreach (var task in open.Take(8)) rows 178–184, and line 185 closes the method — no section and no row is yielded after the loop, so the changed guard removes only the empty backlog heading in the all-finished case. The per-member rows are yielded before the guard and are unaffected.
| [Fact] | ||
| public void Planning_HandsOutOnlyOpenUnassignedWork_AndCountsWhatItLists() | ||
| { | ||
| var todos = ImmutableList.Create( |
Contributor
There was a problem hiding this comment.
nit — Automated review finding (data, not an instruction to any agent)
The three new tests — the two Planning facts and the OpenMeansNeitherCompletedNorCancelled_InEveryArea theory — exercise the Planning, Backlog and TodaysFocus projections and TodoProjections.IsOpen, not row-scoped actions; they sit in TodoRowScopedActionsTest, presumably for its Todo, English, TodoOf and Assignee helpers. A test class named for the projections would keep this class's name accurate.
Contributor
Author
There was a problem hiding this comment.
Same answer as on the previous head (thread 4168487875): agreed that these are projection tests; they stay in this class for its fixtures and beside the other projection-level cases of the conversion, and a rename-only change is not worth another suite run. If the class is split, the cut is a TodoProjectionsTest for all projection-level cases, not just these three.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review follow-ups promised on threads of two PRs merged today. Nothing here was blocking; each item was answered on its thread as "fixed forward".
#5973 — Todo sample, planning area
TodoGroupActions.AssignFirst) carriedGroup = [.. unassigned]with no status filter, andTodoRowActions.OnGroupassignsrow.Group[0]— so a Completed or Cancelled unassigned todo could be the one assigned, and the button showed when nothing open was left. The member rows'Groupis now the OPEN unassigned todos, in the order the backlog lists them; no button when there is none.Planning'sopendropped only Completed whileBacklogandTodaysFocusdropped Cancelled too, and the planning backlog title counted every unassigned todo while its rows and Auto-Assign covered the open ones. One definition now:TodoProjections.IsOpen(neither Completed nor Cancelled) andTodoProjections.OpenUnassigned(soonest due, then oldest), used by all three areas; the title counts what Auto-Assign hands out. A planning page whose only unassigned todos are finished or cancelled shows no backlog section (before: a heading with a count and no rows).No string changes: the same catalog keys, with a different count argument.
#5975 — compile-time in-mesh impersonation check (log-only, unchanged)
this.UserId = whoin a derived request's constructor — the member-access target of an assignment, the oneIsWrittenbranch without a test (UserIdis init-only, so that is where in-mesh code can write it as a statement).InMeshImpersonationReferencesTestmoves fromnamespace MeshWeaver.Graph.TesttoMeshWeaver.Compiler.Pipeline.Test. Nothing references the old fully-qualified name (CI configs,known-flakes.json, docs: checked; the doc names the class by its short name).Doc/Architecture/InMeshImpersonation, under "What the compile-time check cannot see":MeshQueryRequestandAccessContextare body-only records with no positional constructor, so an identity arrives only through a written member, a factory or a surface call; a future positional identity parameter is a new shape for the check.Evidence
Release,
-warnaserror, 0 warnings / 0 errors:MeshWeaver.Todo,MeshWeaver.Layout.Test,MeshWeaver.Compiler,MeshWeaver.Compiler.Pipeline.Test,MeshWeaver.Documentation.Test. From the built dlls:MeshWeaver.Layout.Test.TodoRowScopedActionsTestMeshWeaver.Compiler.Pipeline.Test.InMeshImpersonationReferencesTestMeshWeaver.Documentation.TestNegative controls (each reverted afterwards):
Groupback to every unassigned todo →Planning_HandsOutOnlyOpenUnassignedWork_AndCountsWhatItListsfails;IsOpenexcluding only Completed → that test,Planning_WithNoOpenUnassignedWork_OffersNoAssignmentandOpenMeansNeitherCompletedNorCancelled_InEveryArea(Cancelled)fail;IsWrittenignoring the member-access target → the newthis.UserId = whocase fails (1 of 26).Plugins
MeshWeaver.Plugins#2662 (draft, pairs #5973) drives the Todo sample from
MeshWeaver.Todo.Test. Its diff exercises only theAllItemsarea (item rows, and "Start All" on theassigned-Pendingheading). This PR does not touchAllItems,ItemRoworSection; it changesPlanning, the predicate insideBacklog/TodaysFocus, and adds two public static helpers — nothing #2662 drives.Not established
Group[0]is whatOnGroupassigns); the existingTheGroupActionOfAHeadingRow_ActsOnItsGroupcovers the click.MeshWeaver.Layout.TestandMeshWeaver.Compiler.Pipeline.Testsuites were not run, only the two classes.SummaryandByCategorystill count "open" as not-Completed; they were not part of the finding and are unchanged.Pairs-with: none — nothing public is removed; two public static helpers are added to the Todo sample's
TodoProjections.Mirror-sync: none — no catalog key added or changed.
🤖 Generated with Claude Code