Repository navigation
duly_catalog_item has no recurring_needs_frequency-equivalent validation #65
Description
Activity
Claiming this issue.
- Session:
session_01SqkTcrxUFci7nqXdbBSe2p - Branch:
claude/issue-65-catalog-item-cadence-rules
Plan: mirror
duly_duty's cadence validation reasoning ontoduly_catalog_item, and cover theapplyCatalogHandlerpath (not only direct writes on the object).
Generated by Claude Code
- Session:
{ "issue": 65, "status": "done", "branch": "claude/issue-65-catalog-item-cadence-rules", "pr": "https://github.com/objectstack-ai/duly/pull/81", "premise_still_valid": true, "summary": "Added `recurring_needs_frequency` to `duly_catalog_item` (src/objects/catalog-item.object.ts), mirroring duly_duty's rule and message verbatim — metadata only, one validations[] entry, no handler. Premise verified against origin/main before writing: the three #61 rules were there and this one was not, and an UPDATE blanking a recurring item's frequency was measurably unrefused. Two details of the issue's narrative came out differently under measurement and are documented in the code and PR rather than papered over: (1) INSERT could never reach the gap, because applyFieldDefaults reads an explicit `frequency: null` as absent and re-stamps the CEL default, so UPDATE is the only write that produces the state; (2) the downstream half of the issue's story does not happen — applyCatalogHandler's duty insert hits the SAME default-masking, so a blank fans out as N duties silently stamped 'monthly' rather than N duties each tripping duly_duty's own rule. That makes the catalog item the last place the blank can be caught, which strengthens the card rather than weakening it. No changeset: this repo has no changeset mechanism (no .changeset/ in git log --all, no @changesets/* dep, no script, no CI step, no mention in AGENTS.md), so creating one would mint a mechanism nothing reads — I reached this independently and the coordinator confirmed it mid-task, so nothing was created and nothing needed deleting.", "tests": "Gate union run AFTER the final commit, at `1f0aa43` (git rev-parse --short HEAD), all four through the shared verify lock: `pnpm validate && pnpm typecheck && pnpm test && pnpm build`. Gate-printed verdict lines: `✓ Validation passed (413ms)` · `Test Files 21 passed (21)` · `Tests 545 passed (545)` · `✓ Build complete (614ms)`, with `os-verify-lock: VERDICT command-exit 0 · held the lock 22s · waited 0s`. Exit status read from the wrapper's verdict line, never from a `$?` after a pipe. New suite test/catalog-apply-cadence.test.ts: 4 passed, driving duly_catalog_apply through the app's own data.executeAction against a booted in-memory kernel. ABLATION (reverse verification): the rule was deleted from the committed tree by a script carrying `trap '<git checkout -- …>' EXIT INT TERM`. No rebuild leg applies — these suites import src/ directly and their artifactPath is deliberately unresolvable, so there is no dist/ for a mutation to fail to reach. Mutation confirmed ON DISK before any reading, anchored on the text being removed: grep -c \"name: 'recurring_needs_frequency'\" went 1 -> 0 and the file went 8127 -> 6311 bytes; the script exits 90 rather than measuring if either check fails. Result on the ablated tree: `Tests 2 failed | 30 passed (32)`, the two red being exactly the rule-dependent ones (`refuses an update that blanks frequency on a still-recurring catalog item`, `is declared on both objects under one name with one message`), while `pnpm validate` stayed GREEN — there is no author-time gate for this gap, which is why the tests carry the whole claim. The apply-path suite stayed green under ablation by design: it characterises the path the rule protects, not the rule. Restore confirmed by the trap (rule_count=1, `git status --short` empty). TWO EARLIER ABLATION ATTEMPTS WERE DECLARED VOID rather than quietly re-run to a green: the first missed its anchor (the rule is preceded by a comment block) and the on-disk guard caught it with nothing measured; the second substituted a placeholder rule that broke stack load, so both suites reported `Tests no tests` — a load failure, not an assertion result. Only the third, a clean deletion, produced a reading. Measurements behind the code comments, all on a booted kernel with @objectstack/runtime 17.2.0: insert with explicit `frequency: null` on a recurring item comes back `'monthly'`; update to `{ frequency: null }` was unrefused before the rule and now throws `{ code: 'VALIDATION_FAILED', fields: [{ field: '_record', code: 'rule_violation' }] }` with duly_duty's exact message — the in-process throw carries no `status` (that is added at the HTTP boundary), so `code` plus the exact message is the whole envelope available on this path and is what the tests pin.", "open_questions": [ { "question": "#79 (filed below) makes `duly_catalog_apply` a no-op through the real dispatcher — it finds no catalog items and reports a successful run of zero. #5 calls that action 'the onboarding path, and the single biggest adoption risk in the product'. It is filed unassigned with no labels; how should it be triaged relative to the rest of the M1 queue?", "options": [ "A — treat it as top of the M1 dispatch queue: the headline onboarding action is dead in production today, every existing assertion about it passes against a query shape production never produces, and the fix is small (pass the filter flat in four engine.find calls, plus the FakeEngine in test/catalog-instantiate.test.ts).", "B — queue it normally behind the current batch: nothing in production is calling the action yet (it has no UI home per #27, and reachability was still being decided as recently as #71), so the user-visible cost today is zero.", "C — split it: fix the app side now, and raise the contract half upstream separately (spec's ActionEngineFacade.find declares `query: Record<string, unknown>` and never says whether it is a filter or an envelope — the runtime's implementation is the only thing that decides, and this app read it the other way)." ], "recommendation": "A, with C's upstream half filed alongside rather than instead. The cost of leaving it is not just the dead action: the suite that covers it is green against the wrong convention, so every future change to those handlers is being verified against a shape that cannot occur — and the next application to read that spec type will make the same choice this one did. The fix is mechanical and the tripwire in test/catalog-apply-cadence.test.ts already marks the spot." } ], "out_of_scope_findings": [ "filed as #79: duly_catalog_apply finds no catalog items through the real action dispatcher — the handler passes an ObjectQL envelope `{ where: … }` to ctx.engine.find while the runtime's buildActionEngineFacade wraps its argument in a `where` of its own, so the read becomes `{ where: { where: … } }`, returns empty with no error, and the action reports `{ created: 0 }`. Measured both ways (runtime facade -> created 0; flat facade -> created 1) and pinned as a tripwire in the last describe of test/catalog-apply-cadence.test.ts, written to be deleted when #79 lands. Same mechanism also silences syncCatalogHandler's duty read and resolveBusinessUnit's position read. Unassigned, no labels, left for PM triage." ] }
Generated by Claude Code
os-dev-report
Supersedes the previous comment on this issue: GitHub's body sanitizer ate its leading
<!-- os-dev-report -->marker (making it invisible to the PM scan) and two angle-bracketed fragments inside the JSON —trap '<git checkout …>' EXIT INT TERMcame out astrap '', which misreports the ablation, and a TypeScript generic came out bare. This copy is the same report with those phrases written without angle brackets. Ignore the earlier one.{ "issue": 65, "status": "done", "branch": "claude/issue-65-catalog-item-cadence-rules", "pr": "https://github.com/objectstack-ai/duly/pull/81", "premise_still_valid": true, "summary": "Added `recurring_needs_frequency` to `duly_catalog_item` (src/objects/catalog-item.object.ts), mirroring duly_duty's rule and message verbatim — metadata only, one validations[] entry, no handler. Premise verified against origin/main before writing: the three #61 rules were there and this one was not, and an UPDATE blanking a recurring item's frequency was measurably unrefused. Two details of the issue's narrative came out differently under measurement and are documented in the code and PR rather than papered over: (1) INSERT could never reach the gap, because applyFieldDefaults reads an explicit `frequency: null` as absent and re-stamps the CEL default, so UPDATE is the only write that produces the state; (2) the downstream half of the issue's story does not happen — applyCatalogHandler's duty insert hits the SAME default-masking, so a blank fans out as N duties silently stamped 'monthly' rather than N duties each tripping duly_duty's own rule. That makes the catalog item the last place the blank can be caught, which strengthens the card rather than weakening it. No changeset: this repo has no changeset mechanism (no .changeset/ in git log --all, no @changesets/* dep, no script, no CI step, no mention in AGENTS.md), so creating one would mint a mechanism nothing reads — I reached this independently and the coordinator confirmed it mid-task, so nothing was created and nothing needed deleting.", "tests": "Gate union run AFTER the final commit, at `1f0aa43` (git rev-parse --short HEAD), all four through the shared verify lock: `pnpm validate && pnpm typecheck && pnpm test && pnpm build`. Gate-printed verdict lines: `✓ Validation passed (413ms)` · `Test Files 21 passed (21)` · `Tests 545 passed (545)` · `✓ Build complete (614ms)`, with `os-verify-lock: VERDICT command-exit 0 · held the lock 22s · waited 0s`. Exit status read from the wrapper's verdict line, never from a `$?` after a pipe. New suite test/catalog-apply-cadence.test.ts: 4 passed, driving duly_catalog_apply through the app's own data.executeAction against a booted in-memory kernel. ABLATION (reverse verification): the rule was deleted from the committed tree by a script whose EXIT/INT/TERM trap restores the file with `git checkout --`. No rebuild leg applies — these suites import src/ directly and their artifactPath is deliberately unresolvable, so there is no dist/ for a mutation to fail to reach. Mutation confirmed ON DISK before any reading, anchored on the text being removed: grep -c for the rule name went 1 -> 0 and the file went 8127 -> 6311 bytes; the script exits 90 rather than measuring if either check fails. Result on the ablated tree: `Tests 2 failed | 30 passed (32)`, the two red being exactly the rule-dependent ones (`refuses an update that blanks frequency on a still-recurring catalog item`, `is declared on both objects under one name with one message`), while `pnpm validate` stayed GREEN — there is no author-time gate for this gap, which is why the tests carry the whole claim. The apply-path suite stayed green under ablation by design: it characterises the path the rule protects, not the rule. Restore confirmed by the trap (rule_count=1, `git status --short` empty). TWO EARLIER ABLATION ATTEMPTS WERE DECLARED VOID rather than quietly re-run to a green: the first missed its anchor (the rule is preceded by a comment block) and the on-disk guard caught it with nothing measured; the second substituted a placeholder rule that broke stack load, so both suites reported `Tests no tests` — a load failure, not an assertion result. Only the third, a clean deletion, produced a reading. Measurements behind the code comments, all on a booted kernel with @objectstack/runtime 17.2.0: insert with explicit `frequency: null` on a recurring item comes back `'monthly'`; update to `{ frequency: null }` was unrefused before the rule and now throws `{ code: 'VALIDATION_FAILED', fields: [{ field: '_record', code: 'rule_violation' }] }` with duly_duty's exact message — the in-process throw carries no `status` (that is added at the HTTP boundary), so `code` plus the exact message is the whole envelope available on this path and is what the tests pin.", "open_questions": [ { "question": "#79 (filed below) makes `duly_catalog_apply` a no-op through the real dispatcher — it finds no catalog items and reports a successful run of zero. #5 calls that action 'the onboarding path, and the single biggest adoption risk in the product'. It is filed unassigned with no labels; how should it be triaged relative to the rest of the M1 queue?", "options": [ "A — treat it as top of the M1 dispatch queue: the headline onboarding action is dead in production today, every existing assertion about it passes against a query shape production never produces, and the fix is small (pass the filter flat in four engine.find calls, plus the FakeEngine in test/catalog-instantiate.test.ts).", "B — queue it normally behind the current batch: nothing in production is calling the action yet (it has no UI home per #27, and reachability was still being decided as recently as #71), so the user-visible cost today is zero.", "C — split it: fix the app side now, and raise the contract half upstream separately (spec's ActionEngineFacade.find types its query parameter as a plain record of string to unknown and never says whether it is a filter or an envelope — the runtime's implementation is the only thing that decides, and this app read it the other way)." ], "recommendation": "A, with C's upstream half filed alongside rather than instead. The cost of leaving it is not just the dead action: the suite that covers it is green against the wrong convention, so every future change to those handlers is being verified against a shape that cannot occur — and the next application to read that spec type will make the same choice this one did. The fix is mechanical and the tripwire in test/catalog-apply-cadence.test.ts already marks the spot." } ], "out_of_scope_findings": [ "filed as #79: duly_catalog_apply finds no catalog items through the real action dispatcher — the handler passes an ObjectQL envelope `{ where: … }` to ctx.engine.find while the runtime's buildActionEngineFacade wraps its argument in a `where` of its own, so the read becomes `{ where: { where: … } }`, returns empty with no error, and the action reports `{ created: 0 }`. Measured both ways (runtime facade -> created 0; flat facade -> created 1) and pinned as a tripwire in the last describe of test/catalog-apply-cadence.test.ts, written to be deleted when #79 lands. Same mechanism also silences syncCatalogHandler's duty read and resolveBusinessUnit's position read. Unassigned, no labels, left for PM triage." ] }
Generated by Claude Code
Found while implementing #61.
duly_dutyhasrecurring_needs_frequency(a script validation refusingform == "recurring" && isBlank(frequency)).duly_catalog_itemhas never had an equivalent rule — before #61 this was mostly masked byfrequency's unconditionaldefault: trueoption marker always filling in'monthly'on insert, but the gap is real: an UPDATE that explicitly blanks a recurring catalog item'sfrequency(e.g.{ frequency: null }) has nothing to refuse it, sinceapplyFieldDefaultsonly runs on INSERT.This matters more than a bare schema gap because of
applyCatalogHandler(src/actions/catalog.handlers.ts): it copiesfrequency(among the other cadence fields) from the catalog item onto every newduly_dutyverbatim. Arecurringcatalog item quietly left with a blank frequency would replicate that blank onto every person who takes the role — each of whom would then individually tripduly_duty's ownrecurring_needs_frequencyon their first save, rather than the org catching the problem once, at the source, on the catalog item itself.Not fixed as part of #61 — that issue's adjudicated scope was specifically the standing-duty-carries-a-frequency defect (the converse direction), not this pre-existing, independent "recurring requires a frequency" gap on the catalog object. Also worth deciding at the same time:
duly_catalog_itemhas noeffective_*-window equivalent either (N/A — it has noeffective_from/effective_tofields at all), so this issue is scoped torecurring_needs_frequencyonly.Suggested fix
Add a
recurring_needs_frequencyvalidation toduly_catalog_item(src/objects/catalog-item.object.ts), mirroringduly_duty's wording, alongside thestanding_no_frequency/non_recurring_no_due_timing/standing_no_grace_daysrules #61 adds there.