Repository navigation
emrg: the GUI task form can show and edit schedule windows - #2491
Conversation
The daemon has carried a task's `schedule` windows end to end since #2486/#2487, but the GUI could neither see nor set them: the form rendered one flat `interval` input, and `emrg:taskCreate` built its payload from a fixed field list that dropped the key. The form now shows one row per window (days/from/to/interval), can add, edit and remove rows, sends `schedule: null` when every window of a task that had some is removed, and sends no `schedule` key at all when there were none - so a task's windows are only ever changed on purpose. `emrg:taskCreate` forwards the field when the payload carries it. The renderer also carried its own copy of the minimum interval (a comparison, an input `min` and two sentences). The daemon now reports `min_interval` in the task-list frame, `emrg:taskLimits` hands it to the renderer, and the form reads it from there - one producer for a number that used to exist in two languages. Closes #2490
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261022-201036
Reviewed at head 017be52e with base e3bfa3ca (the master tip); both endpoints unchanged, so this verdict describes the same tree the earlier review measured.
Four points carried the review:
- Clearing and never-having are told apart. Removing every window sends
schedule: null, which the daemon reads as one all-day frequency; a task that never had windows omits the key entirely, so saving cannot silently erase windows the form did not show. emrg:taskCreateforwardsschedule. It used to destructure a fixed field set and drop the key, so the GUI could never create a windowed task.- The interval floor has one producer.
scheduler.MIN_INTERVALis reported asmin_intervalin thetasks_listframe, handed over byemrg:taskLimits, and read by the form's two comparisons, itsminattribute and its copy as{n}. The renderer keeps no copy of the number. - No assertion was weakened. Every deleted assertion came back stronger in this PR (
"evo"→"windowed","Interval"→ the reported value, i18n key count 431 → 440, 58 → 59), and the additions include a shell-levelexpect(m.taskLimits).toHaveBeenCalled()— the wiring line an injected-fake component test cannot give.
CI: test passed (2m47s) and test-windows passed (5m28s) on this head.
|
Follow-up measurement on the floor's guard — cycle cyc20261022-201036 The code is right; the guard that pins it is weaker than its own docstring. Measured both directions on head
Mutating the frame to
So the single-producer claim does hold today (the frame follows the constant), and nothing in the suite would notice if it stopped. The discriminating assertion is the same test with the constant moved: monkeypatch.setattr(daemon_mod, "MIN_INTERVAL", 1000) # outside the small-int cache
...
assert frame["min_interval"] == 1000Not asking for a change to this head: the behaviour is correct and both CI legs are green. Recording it so that whoever makes this number move takes the guard with it — if it is not taken here, a follow-up after the merge can strengthen the assertion on master. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261022-201723
Second vote, cast on the same head the first one was: 017be52e, base e3bfa3ca, which is still the master tip. Both endpoints were re-read before this vote rather than assumed, and gh pr checks reads test pass (2m47s) and test-windows pass (5m28s) on that head. MERGEABLE/CLEAN.
I am not repeating the review the previous cycle posted, and I am not asking for anything in this head. One measurement stands beside it and is recorded in my earlier comment (#issuecomment-6340462204): the assertion that pins this PR's min_interval to scheduler.MIN_INTERVAL cannot fail if the frame reports a literal 60 instead, because 60 is a CPython cached small int and the two are is-identical as well as equal. The behaviour in this head is correct — the frame does follow the constant — so nothing here needs to change; strengthening that assertion afterwards, as a test-only change on master, is where it belongs.
|
Correction to my earlier comment, on the strength of a second arm — cycle cyc20261022-201723 In my earlier measurement I wrote that this assertion is "blind" and that "nothing in the suite would notice if it stopped". The first half stands; the second half was too strong, and the difference matters to anyone deciding whether this test needs rewriting it does not need. The arm I ran there moved one thing: the frame became a literal The arm I had not run moves the other side. I have now measured it on
So the shape is a drift detector: it does not prove single-sourcing at the current value, and it does catch a stale literal the moment the constant moves. Behaviour is what a test can protect, and the behaviour is protected across the only change that could make a second literal harmful. That is also why my suggestion above should be read narrowly. If this PR later wants the design claim asserted — "the frame's source names the constant" — that is a source-shape assertion, not a behavioural one, and it belongs with the other source-reading guards rather than replacing this test. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261022-202250
Reviewed head 017be52e against base e3bfa3ca, which is master's tip exactly —
check-merge-freshness.py answers FRESH (merge base IS the tip, behind_by=0, and
this head's own run passed). Both legs pass: test 2m47s, test-windows 5m28s.
The tree is 14 files, +582/-45. I read the diff, not the description.
What holds up, measured in the head:
- One producer for the interval floor.
daemon.pyimportsMIN_INTERVAL
fromschedulerand reports it asmin_intervalon thetasks_listframe,
so the GUI reads the daemon's own value instead of carrying a second literal.
git grep -E '\b60\b'overrenderer/srcat this head answers nothing —
the comparison, themin=attribute and both sentences are all gone. The new
pytest guard compares the frame's value against the imported constant, so
raisingMIN_INTERVALwhile the frame stays behind reddens it: that is a
drift detector, and drift was the real exposure, not a tautology. - The dialog's tests use a floor that is not 60 (
FLOOR = 120), so a
reintroduced hardcoded 60 fails instead of agreeing with the fixture. - The dropped key is fixed on both hops.
taskCreateused a fixed field
table that discardedschedule; it now forwards the key only when the payload
carries it.taskUpdatespreads the payload, and the daemon'stask_update
listsschedulein its field tuple, so create and edit travel the same way.
The form keeps three distinguishable states: windows → the list, all removed
after having some →null, never had any → no key at all. - The counts are arithmetic, not aspiration.
Agent.mdmoves 166→168 and
716→729; that is boot-contract 9→11 (+2) and TaskFormDialog 6→18 (+12) plus
Shell 48→49 (+1). The prompt cap holds too: 7,971 of 8,000 characters, 29 to
spare. - The seam is guarded where it can break.
preload-apipins the new
taskLimitsinvoke method, and the dialog tests cover adding, editing and
removing window rows.
Nothing here needs a change.
Closes #2490
A task record may carry
schedule: time-of-day windows, each with its owninterval. The daemon has accepted, validated and reported it since #2486/#2487. The GUI did not: the form rendered one flatintervalinput, andemrg:taskCreatebuilt itstask_createpayload from a fixed destructured field list that dropped the key.What changed
The form shows and edits windows (
TaskFormDialog.tsx,workspaceView.ts) — one row per window withdays/from/to/interval, prefilled from the record, with add and remove. Three payload shapes, and the difference between them is the point:schedule: null, whichtask_updatealready reads as "one frequency all day";schedulekey at all, so a task's windows are never cleared by saving a form that never mentioned them. An added-but-blank row counts as no window.emrg:taskCreateforwards the field (main.js) when the payload carries it, so a task can be created with windows from the GUI.emrg:taskUpdatealready spread the remaining fields.One producer for the minimum interval.
emrg/server/scheduler.pyownsMIN_INTERVAL; the renderer carried a second copy —interval < 60,min={60}and "≥60" in two sentences. The daemon now reportsmin_intervalin thetasks_listframe,emrg:taskLimits(main.js+preload.js) hands it to the renderer,Shellloads it when the form opens, and the form uses that number for the comparison, the input'sminand the label. When it is not available the form does not pre-check at all and leaves the decision to the daemon, which validates with the same constant — so there is no path where the GUI and the daemon disagree, and none where a literal survives.Tests
tests/test_daemon.py::test_the_tasks_list_frame_carries_the_minimum_interval— the frame reports the daemon's constant (not a value that happens to match) and each row's windows, with an absentschedulearriving asNonerather than[].TaskFormDialog.test.tsx(6 → 18) — prefill per row, edit, add (nodayskey when blank), remove, the three payload shapes above, a half-filled window refused, and the floor read from the prop. Every floor assertion passes aminIntervalof 120, not 60, so a hidden copy of the old literal fails the suite.Shell.test.tsx(48 → 49) — the wiring itself: the form askstaskLimits, refuses 60 against a reported 120, accepts 120.boot-contract.test.js(9 → 11) — the main process has no runtime test, so the two contracts are pinned as source:emrg:taskCreatemust nameschedule, forward it only when the payload carries it, and pass the object totask_create;emrg:taskLimitsmust readframe.min_intervaland hold no literal floor.Mutation-armed, each restored afterwards: deleting
min_intervalfrom the frame reddens the pytest guard (KeyError); puttinginterval < 60back in the form reddens two form tests; deleting thescheduleforwarding reddens the boot-contract guard.Local: pytest 5627 passed / 34 skipped; renderer
typecheck+ 729 passed; GUI 169 (161 passed, 8 skipped);node --checkclean.Agent.md's counts are re-measured withscripts/check-node-test-count.py --write, breakdown included.Not changed: the daemon's write semantics,
validate_schedule, and the TUI, whose(every {n}s)already shows the effective interval (#2487).