Implement reusable Schedule model - #234
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
jgeluk
left a comment
There was a problem hiding this comment.
Automated review (Claude Code), findings verified by executing the SHACL shapes with pyshacl. Nine inline comments below; one finding has no diff anchor:
CI never SHACL-validates the shipped example files (.github/workflows/build-spec.yml, pre-existing gap deepened by this PR): no workflow step invokes dprod-contracts/validate.py or pyshacl-validates the examples — the tests only do substring checks on them via read_text — even though validate.py's docstring says it is "safe to wire into CI". A future typo in an example schedule IRI (e.g. ex:daily-0700-londn) or a malformed expression would merge green despite the fail-fast validation story this PR ships.
Also noted but not commented inline (cleanups, not bugs): DutyShape/ScheduleReferenceShape constraint duplication (double violations with different messages), duplicated IANA-timezone charset regex, triplicated pyshacl invocation config across test files/validate.py, and per-format grammars hardcoded in the shared ScheduleShape despite the "pluggable formats" docs.
| $this dct:conformsTo dprod:PosixCrontabScheduleFormat ; | ||
| dprod:scheduleExpression ?expression . | ||
| FILTER ( | ||
| !REGEX(STR(?expression), "^([^ ]+ +){4}[^ ]+$") || |
There was a problem hiding this comment.
[CONFIRMED] POSIX crontab five-field check treats only spaces as separators, so it both accepts expressions with tab/CR-smuggled extra content and rejects valid tab-separated crontab entries.
Reproduced by running the shapes: "0 20 * * *\t/usr/bin/reboot" and "0 6 * * *\rcommand" both conform (the [^ ] field atom matches tabs/CR and only \n is checked by the CONTAINS branch), while the valid POSIX entry "0\t6\t*\t*\t*" (POSIX defines field separators as blanks = space or tab, per the very spec the format IRI dct:conformsTo cites) is rejected with the misleading message "must contain exactly five time fields".
A \S-based regex with a whitespace-class separator (e.g. ^(\\S+[ \t]+){4}\\S+$) closes both holes and makes the CONTAINS("\n") patch unnecessary.
| dprod:scheduleExpression ?expression . | ||
| FILTER (!REGEX( | ||
| STR(?expression), | ||
| "^DTSTART;TZID=[A-Za-z0-9._+-]+(/[A-Za-z0-9._+-]+)*:[0-9]{8}T[0-9]{6}\\r?\\nRRULE:FREQ=(SECONDLY|MINUTELY|HOURLY|DAILY|WEEKLY|MONTHLY|YEARLY)(;[^;\\r\\n=]+=[^;\\r\\n]+)*$" |
There was a problem hiding this comment.
[CONFIRMED] The RFC 5545 regex requires exactly one DTSTART line plus one RRULE line, rejecting valid iCalendar content the ontology and docs explicitly permit, while accepting invalid RRULEs.
Reproduced by running the shapes: "DTSTART;...\nRRULE:FREQ=DAILY\nEXDATE;TZID=Europe/London:20261225T060000" is rejected even though dprod-contracts.ttl describes the format as "RFC 5545 iCalendar content containing DTSTART and RRULE" (containing, not consisting of), so schedules with exception dates cannot be expressed at all. Meanwhile "RRULE:FREQ=DAILY;FREQ=WEEKLY" (invalid per RFC 5545) passes. Unanchored-but-valid forms like VALUE=DATE DTSTART are also excluded.
The shape's claimed grammar and the documented format diverge in both directions.
| sh:message "Schedule must have exactly one authoritative expression as an xsd:string." | ||
| ] ; | ||
| sh:property [ | ||
| sh:path dprod:scheduleTimeZone ; |
There was a problem hiding this comment.
[CONFIRMED] Two independent timezone sources can conflict and still validate, and the "timezone required when the format carries none" rule is hardcoded to the POSIX IRI so extension formats get no timezone enforcement.
Reproduced by running the shapes: a Schedule with dct:conformsTo dprod:Rfc5545ScheduleFormat, TZID=Europe/London in the expression, AND dprod:scheduleTimeZone "America/New_York" conforms. No shape or prose defines precedence, so two conformant processors legally disagree by 5 hours on every occurrence — the exact nondeterminism this PR was written to remove.
Conversely, an extension format lacking an embedded timezone (Quartz/AWS cron) triggers no timezone requirement, because the check at line 338 is keyed on dprod:PosixCrontabScheduleFormat only rather than driven by metadata (e.g. a carriesTimeZone flag) on the ScheduleFormat individual.
| let times = expand(duty.recurrence, Σ.clock) | ||
| in { t ∈ times | duty.condition = ⊥ ∨ ⟦duty.condition⟧(Σ.env) } | ||
| case expand(duty.schedule, Σ.clock) of | ||
| Error(e) → Error(e) |
There was a problem hiding this comment.
[CONFIRMED] occurrences(duty, Σ) now propagates ScheduleError, but no lifecycle, updateDutyStates, or Eval rule consumes the error, leaving a SHACL-valid policy with an unprocessable schedule with an undefined evaluation outcome — while README still claims "Total functions, no undefined states".
§5.1–§5.2 (lines 325–394) and Eval (§7.2, lines 693–733) have no Error branch — updateDutyStates only branches on Pending/Active — so two conformant processors evaluating a contract whose schedule uses an extension format with no registered processor may diverge (one marks the duty Violated, another halts or skips it). §9's note restricts the theorems to "processable schedules", but §9.1 line 809 still asserts totality for all well-formed inputs and README.md lines 13/146 remain unqualified; the formal-verification story (Dafny/Coq) cannot state the theorem as written.
Also expand's signature (Schedule × Time, line 451) omits the processor registry its own definition depends on.
|
|
||
| {schedule} | ||
| """, | ||
| format="turtle", |
There was a problem hiding this comment.
[CONFIRMED] Validating with ont_graph + inference='rdfs' lets rdfs:range inference type every dprod:schedule object as dprod:Schedule, making the sh:class constraints unfalsifiable in the test harness and causing the same data to pass here but fail the documented CLI invocation.
Reproduced by running both configurations: a duty referencing ex:sched that has dct:conformsTo and dprod:scheduleExpression but no a dprod:Schedule conforms under the test/validate.py config (type inferred from rdfs:range) yet fails under the docs' recommended shacl validate --shapes ... --data ... invocation.
The suite therefore never exercises sh:class on either ScheduleReferenceShape or DutyShape (deleting both leaves all 32 tests green), and test_bare_dcat_frequency_is_not_an_executable_schedule passes via inferred-ScheduleShape minCounts, not via the reference check its name implies.
| dprod:subjectOfDuty ex:dataTeam ; | ||
| odrl:action ex:deliver ; | ||
| dprod:recurrence "FREQ=DAILY;BYHOUR=7;BYMINUTE=0" ; | ||
| dprod:schedule ex:daily-0700-london ; |
There was a problem hiding this comment.
[CONFIRMED] The "Daily Delivery" and "high-frequency" complete examples reference ex:daily-0700-london and ex:every-minute-london Schedules that are never defined anywhere in the document, so the examples fail the guide's own validation checklist.
Reproduced by validating the guide's example with its own documented command: the shapes report "Duty schedule must be a single IRI identifying a dprod:Schedule" and "Schedule references must be IRIs identifying dprod:Schedule resources" because the referenced Schedule resources don't exist (the guide only ever defines ex:daily-0600-london). A reader copying the lines-334–341 or 373–381 examples verbatim ships data that violates checklist item 10 ("each Schedule has one exact format and one expression") added by this same PR.
| rdfs:label "schedule shape" ; | ||
| sh:targetClass dprod:Schedule ; | ||
| sh:nodeKind sh:IRI ; | ||
| sh:message "A Schedule must have an IRI identifier." ; |
There was a problem hiding this comment.
[CONFIRMED] The node-shape-level sh:message "A Schedule must have an IRI identifier." attaches to every violation ScheduleShape produces, so unrelated failures (expression cardinality, format, POSIX field count) all also report the misleading IRI-identifier message.
Reproduced by running the shapes: a Schedule with a proper IRI but a six-field crontab expression yields two messages — "POSIX crontab schedules must contain exactly five time fields." and "A Schedule must have an IRI identifier." — sending users hunting for a nonexistent identifier problem.
The message should live on a dedicated constraint (e.g. move sh:nodeKind+sh:message into a sub-shape, or rely on per-constraint messages) so each violation carries only its own diagnostic.
| own SHACL and processor profile. Unknown or malformed formats fail validation | ||
| or processing; they never silently produce an empty schedule. | ||
|
|
||
| ### Common RFC 5545 RRULE Components |
There was a problem hiding this comment.
[CONFIRMED] The "Common RFC 5545 RRULE Components" table kept the old model's bare RRULE strings (FREQ=DAILY;BYHOUR=6;BYMINUTE=0) with no text explaining how to compose them into a valid expression, and its BYHOUR/BYMINUTE idiom contradicts every shipped example, which anchors time-of-day in DTSTART.
Any table value pasted into dprod:scheduleExpression fails the ScheduleShape regex (needs DTSTART;TZID=...\nRRULE:...), and BYHOUR/BYMINUTE appears nowhere else in dprod-contracts/ — the example directly above the table (ex:daily-0600-london) encodes 06:00 in DTSTART with a bare RRULE:FREQ=DAILY, so a reader combining the two conventions produces a redundant or self-contradictory schedule (DTSTART at 06:00 plus BYHOUR=7 validates but is ambiguous across processors).
| *sorted( | ||
| path | ||
| for path in (CONTRACTS_DIR / "docs").glob("*.md") | ||
| if path.name != "specification.md" |
There was a problem hiding this comment.
[PLAUSIBLE] The obsolete-recurrence guard test excludes the entire specification.md from its sweep (if path.name != "specification.md") although only one table row there legitimately mentions dprod:recurrence, leaving the primary normative document unguarded.
A future edit (e.g. a merge resolved against pre-PR text) could reintroduce a dprod:recurrence usage example into specification.md; the suite stays green and the normative spec contradicts the shapes that hard-reject the property. The sanctioned mention is exactly one line (the RejectObsoleteRecurrenceShape row at specification.md:447), so the test could assert count <= 1 or match usage patterns instead of a whole-file carve-out.
Relatedly, the parallel lifecycle test (test_lifecycle_status_model.py:151-162) maintains a broader diverging copy of this authored-files list, so the two sweeps miss different files.
Shapes: accept tab-separated POSIX crontab fields and reject smuggled tab/CR/LF content; allow EXDATE/RDATE lines in RFC 5545 expressions while rejecting non-standard or duplicate RRULE parts; reject a dprod:scheduleTimeZone that conflicts with the embedded TZID; move the IRI-identifier message onto its own shape so it no longer annotates unrelated violations. Ontology: every dprod:ScheduleFormat now declares dprod:carriesTimeZone, and the timezone-required rule keys on it so extension formats get the same enforcement as POSIX crontab. Semantics: schedule errors freeze the affected duty (Rule D-FREEZE), surface in the result's errors field, and the processor registry is part of Σ, so the totality and determinism theorems hold for unprocessable schedules too. Tests and CI: every schedule case is validated both with the ontology graph and in the bare documented CLI configuration, making the sh:class constraints falsifiable; the obsolete-recurrence sweep covers specification.md; validate.py runs in the build workflow. Docs: define the previously missing example schedules, rewrite the RRULE components table as complete expressions, and sync the guide checklist and specification with the new rules. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The GitHub Pages site is offline and every branch is now published via ekgf.org/dprod, so remaining references to the old URL would 404 forever. Each occurrence is mapped by role rather than blindly replaced: - Term/ontology namespaces -> https://www.omg.org/spec/DPROD/dprod/ - Shapes namespace -> https://www.omg.org/spec/DPROD/shapes/ - Data-individual namespace -> https://www.omg.org/spec/DPROD/data/ - JSON-LD context and artifact links -> the permanently served archive copies under https://ekgf.org/dprod/spec/archive/1.0/ - Self-links inside the archived 1.0 spec page -> relative links - Site links (README, ReSpec metadata) -> https://ekgf.org/dprod The archived 1.0 snapshot is rewritten under the same rules so the spec served at /spec/main contains no dead URLs; the marketing-site plan is updated to record that. The GitHub Pages deploy job is removed from the build workflow so the retired site cannot be resurrected by a push to main. The still-live ekgf.github.io/respec script URL (a different repository) is untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Addresses #239. This is intentionally stacked on #234 because the acceptance criteria require the Schedule model introduced there. Once #234 is merged, this branch can be rebased onto `develop` and the PR base changed. ## Changes - add regression tests for Permission and Prohibition misuse of every surviving duty-only property - remove OWL domains that silently inferred an additional `odrl:Duty` type - enforce Duty subject scope through fail-closed SHACL - retain global rejection of obsolete `dprod:recurrence` - verify generic `dprod:schedule` remains valid on non-Duty resources - document SHACL as the enforceable source of Duty-only scope ## Verification - `python -m unittest discover -s dprod-contracts/tests` (48 tests) - `python -m unittest discover -s tests` (2 tests) - `python dprod-contracts/validate.py` - `python spec-generator/main.py` - `git diff --check`
Summary
dprod:recurrencestring with reusable, identifieddprod:Scheduleresourcesdprod:ScheduleFormat, exact RFC 5545 and POSIX crontab format identifiers,dprod:scheduleExpression, anddprod:scheduleTimeZonedprod:recurrenceRoot cause
The previous model coupled scheduling to duties and encoded only an unanchored RRULE string. It could not represent exact alternative formats, reuse schedules across different business concepts, or distinguish invalid schedules from schedules with no occurrences.
Breaking change
dprod:recurrenceis removed from the ontology. Existing data must create an identifieddprod:Scheduleand reference it throughdprod:schedule. The SHACL profile rejects the obsolete property with a migration message.Validation
.venv/bin/python -m unittest discover -s tests -v.venv/bin/python -m unittest discover -s dprod-contracts/tests -v.venv/bin/python dprod-contracts/validate.pybash build.shgit diff --checkCloses #184