Uh oh!
There was an error while loading. Please reload this page.
Forbid a frequency (and the neighbouring cadence fields) on a standing duty - #66
Merged
Merged
Conversation
…g duty A standing duty never dispatches, so a stamped frequency/due_anchor/ due_offset_days/lead_days/grace_days reads as though it runs on a schedule it never will. Makes each default conditional on `form` (CEL null-guard idiom) and adds the validation rules that refuse the meaningless combinations outright, on both duly_duty and duly_catalog_item so a wrong value on the catalog side is never replicated onto an instantiated duty. Fixes#61 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p
os-warren
marked this pull request as ready for review
September 1, 2026 08:19
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Sep 1, 2026
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes#61
What changed
frequency— and the four fields the issue asked me to check alongside it (due_anchor,due_offset_days,lead_days,grace_days) — now carry conditional defaults on bothduly_dutyandduly_catalog_item, plus validation rules that refuse the meaningless combinations outright (Option A from the issue; B — hiding it in the UI — was rejected there because it leaves the wrong value in the data).The scoping is not uniform across all five fields, because it follows what
dispatch.plan.ts#planForDutyactually reads, not a blanket "non-recurring" rule:frequencystandingrecurring,one_offrecurring_needs_frequency's converse, standing only). One-off's equally-meaningless frequency is a separate, narrower case this issue didn't ask me to touch.due_anchor,due_offset_days,lead_daysstanding,one_offrecurringonlyplanForDutyreturns before reading any of the three for the other two forms).grace_daysstandingrecurring,one_offstanding, which never has a task at all, loses it.Mechanism: each default is now a CEL
defaultValueusing the blessed null-guard idiom (cond ? value : null, objectstack#3306 — the same evaluatorField.formulashares), e.g.formis declared above every cadence field, so by the time each of these runs,record.formis already resolved. Four new validation rules pair with the defaults (two onduly_dutymirrorrecurring_needs_frequency's converse, two apply the same treatment toduly_catalog_item; see the object files for the exact set —standing_no_frequency,non_recurring_no_due_timing,standing_no_grace_dayson each object).Applied to both
duly_dutyandduly_catalog_item— #5's instantiation (applyCatalogHandler) copies these five fields verbatim onto every duty made from a catalog item, so a wrong value on the catalog side would otherwise be replicated onto every person who takes the role.Why a real-engine test suite, not a schema-structural pin
pnpm validatedoes not check a CELdefaultValue's syntax or behaviour at all —@objectstack/spec's authoring gate discriminates the shape and, for an expression envelope, returns unconditionally ("a CEL result type is unknowable at parse time"). So the only thing that proves the conditional default actually works is inserting a row through a booted engine and reading it back, which is whattest/cadence-conditional-defaults.test.tsdoes (23 new tests,duly_dutyandduly_catalog_itemboth covered, plus a#5-shaped instantiation check and a negative control — see below).test/dispatch.test.ts's old "the cadence fallbacks are the object schema" block used to pindue_anchor/due_offset_days/lead_daysstructurally (Duty.fields.x.defaultValue === literal); that shape no longer applies once the default is a CEL expression, so it's replaced with a comment pointing at the new file, which checks the sameDEFAULT_DUE_ANCHOR/DEFAULT_DUE_OFFSET_DAYS/DEFAULT_LEAD_DAYSconstants against a real inserted recurring row instead — a stronger pin, not a weaker one.Ablation
Removed the
standing_no_frequencyvalidation block fromduty.object.ts(confirmed on disk:grep -cdropped from 2 occurrences to 1, the remaining one being the field description's own mention of the rule name), rantest/cadence-conditional-defaults.test.ts: exactly the targeted test ("refuses a frequency on a standing duty") went red, all 22 others stayed green. Restored viagit checkout -- src/objects/duty.object.tsfrom a prior commit (script used atrap ... EXIT INT TERMso this happens even on a kill), confirmed back to 2 occurrences and a cleangit diffagainst HEAD, re-ran the file: 23/23 green again.Negative control:
test/cadence-conditional-defaults.test.tsalso assertsrecurring_needs_frequencystill fires — an UPDATE that blanksfrequencyon a duty that staysform: 'recurring'is refused with that rule's own exact message, and the row is left untouched. A second test inserts arecurringduty with an explicit non-default frequency and astandingduty with none, to confirm neither new rule accidentally fires on the other form.Gates (all at
78ee086)Neighbouring fields not touched
effective_from/effective_toand theireffective_window_orderedrule are untouched — a standing duty legitimately has an effective window (when it started/stopped being someone's responsibility), unrelated to dispatch cadence.Filed separately (out of scope for this issue)
duly_catalog_itemhas norecurring_needs_frequency-equivalent rule at all (it never had one, unrelated to standing) — arecurringcatalog item'sfrequencycan be blanked via an UPDATE with nothing to refuse it, and that blank would propagate onto every duty instantiated from it afterward. Not fixed here (different validation, not a mechanical extension of this fix).Generated by Claude Code