Repository navigation
Catalog instantiation — apply a position's duty catalog to a person #5
Description
Activity
Claim: PM loop round 1
Session:session_01SqkTcrxUFci7nqXdbBSe2p
Branch:claude/issue-5-catalog-instantiate
Worktree:duly-issue-5
Domain: n/a (single-package app repo; nodomain:*lanes in this repo)
File surface:src/actions/(incl. the existingregister-handlers.ts),test/catalog-instantiate.test.ts(stop on breach; explain in the report)
Container & model: M,mode:subagent,model: opus
Clause-②: no — application metadata only; no protocol contract surface touched.
Serial constraints cleared: batch 1 is #1 / #3 / #5 / #6, file surfaces disjoint. This card is the sole owner ofsrc/actions/this round — #4 (the othersrc/actions/card) is held atpm:blockedbehind #3 precisely so the two do not collide inregister-handlers.ts.
Generated by Claude Code
{ "issue": 5, "status": "done", "branch": "claude/issue-5-catalog-instantiate", "pr": "https://github.com/objectstack-ai/duly/pull/34", "premise_still_valid": true, "summary": "Implemented duly_catalog_apply and duly_catalog_sync as object-less script actions plus their handlers, registered inside the existing registerDulyActionHandlers (objectstack.config.ts untouched, register-handlers.ts shape preserved for #4). Apply is idempotent on (catalog_item, owner) with a same-run guard; sync writes only the five cadence fields, never touches source:'self' duties, and reports duties from deactivated catalog items instead of deleting them. Two PM mechanism assumptions were tested: the global-action input surface CAN express position_code + a multi-user picker (confirmed in the built artifact), and sys_user_position.business_unit_id IS the real BU anchor (defined in @objectstack/plugin-security, not platform-objects). The third is WRONG and is the finding that matters: sys_user carries no timezone field AND ExecutionContext.timezone is not propagated into an action handler's context, so a duty's zone has no source at all and resolves to UTC — filed as #26, and it changes what #20's deployment docs must say. Separately, a global action has no UI home in protocol 17 (global_nav retired, every surviving location object-bound), so this onboarding flow is API/MCP-only with no button — filed as #27, reported rather than redesigned around.", "tests": "All four gates green on 13feae8 (= PR head sha, clean tree, run after the final commit) via the shared verify lock: `pnpm validate && pnpm typecheck && pnpm test && pnpm build` -> os-verify-lock VERDICT command-exit 0. Verdict lines as the gates printed them: validate '✓ Validation passed (316ms)' with 'UI: 1 Apps 5 Views 2 Actions'; typecheck `tsc --noEmit` no output; test 'Test Files 2 passed (2) / Tests 36 passed (36)'; build '✓ Build complete' writing dist/objectstack.json 53.5 KB. 36 tests total (28 new + 8 pre-existing invariants), covering all five acceptance criteria: 26 items x 3 users = 78 created all source:'catalog' with catalog_item set and 78 distinct (item,owner) pairs; re-apply = 0 created / 78 skipped with engine.inserts.length asserted unchanged; editing due_offset_days then syncing updates all 3 derived duties while owner/status/timezone/effective_* are asserted untouched AND every update patch is asserted to name only cadence keys; a source:'self' duty with catalog_item set is never touched; deactivating an item reports 3 retired with zero updates and zero deletes. TWO ABLATIONS, both with a `trap ... EXIT INT TERM` restore, both confirmed on disk by grep counts of the removed and injected text (no reliance on an editor exit code), both restored byte-identically (clean `git status`). No rebuild was needed or claimed: the tests import ../src/** directly, so nothing resolves through dist/. (1) Commenting out `registerCatalogActionHandlers(ql);` (marker count 1->0, ABLATED marker 1): `pnpm validate` exit 0 '✓ Validation passed' while `pnpm test` exit 1 on '× every declared action has a registered handler' — this is the point of the wiring test, since validate provably cannot see the missing registration. (2) Removing the in-code `if (text(duty?.source) !== 'catalog') continue;` guard (count 1->0): `pnpm test` exit 1 on '× the source guard is in the code, not only in the query filter', and ONLY that test — the sibling self-duty test stayed green because the fake engine's where-clause still filtered, which is exactly why the filter-blind-driver test exists. Exit codes were captured before any pipe (redirect-then-capture), and each result is quoted from the gate's own printed line.", "open_questions": [ { "question": "Should a duty fall back to sys_user.primary_business_unit_id when the person has no anchored sys_user_position row? Today it is created with NO business unit (not null), which silently costs the rollup.", "options": [ "A. Keep the declared anchor only (shipped). sys_user_position.business_unit_id, else no business_unit. Matches the issue's mapping table exactly and keeps 'do not require a sys_user_position row' true.", "B. Add sys_user.primary_business_unit_id as a documented second rung. It is a real, declared platform field on sys_user, and day one — before positions are modelled — it is the ONLY anchor a person has, which is precisely the state the issue says must work." ], "recommendation": "B, but as a follow-up rather than in this PR. Real business need: with position_code free text by design, the no-position-row case is the normal day-one state, so option A means the first customers' duties roll up nowhere and the manager dashboard (#10) shows empty units for a reason nobody can see. Long-term soundness: two declared platform anchors with an explicit precedence is a resolver, not a tolerant fallback — the assignment-level anchor documents itself as nullable ('Null = unanchored'), so consulting the user-level one is reading the platform's own second answer, not inventing one. AI-written-metadata safety: neutral. Startup scope discipline: this is the axis that keeps it OUT of this PR — the issue names one anchor, the PM said to report a different shape rather than redesign around it, and resolveBusinessUnit() is a single named function so adding the rung is one line once you rule." }, { "question": "duly_catalog_sync takes an OPTIONAL position_code that narrows the sweep. The issue did not specify sync's input; omitting it syncs every catalog-sourced duty in the org, as the issue's text describes.", "options": [ "A. Keep the optional narrowing (shipped) — omit for the org-wide behaviour the issue describes, pass it to correct one position.", "B. Drop the param and make sync always org-wide, matching the issue's literal text with no added surface." ], "recommendation": "A. The issue itself says sync 'is destructive to authored cadence, so it must be legible after the fact', and the same reasoning applies before the fact: a one-position correction should not be able to rewrite cadence on every duty in the org because that is the only shape available. It is one optional param on the already-declared surface, it costs nothing when omitted, and it mirrors apply's input so the pair reads as one feature. Flagging it because it is an input the issue did not specify — say the word and I will drop it." }, { "question": "Apply's idempotency probe treats ANY existing duty for a (catalog_item, owner) pair as taken, without qualifying on source or status — so a source:'self' duty with a stray catalog_item, or a retired catalog duty, suppresses a fresh create.", "options": [ "A. Literal reading (shipped) — the issue's rule is the '(catalog_item, owner)' pair, unqualified; a second row for that pair is the duplicate the rule exists to prevent, whatever wrote the first.", "B. Qualify the probe on source = 'catalog' so a self-declared duty cannot mask a catalog one.", "C. Also qualify on status != 'retired' so re-applying re-creates a duty someone retired." ], "recommendation": "A, which is what shipped and what the issue's wording says. Flagging it because B and C are both defensible and the difference only shows up on messy data. C in particular is a real product question: if an admin retires a duty and then re-runs onboarding, should the duty come back? Silently resurrecting it would undo a deliberate local decision, which is the same principle that keeps sync off status — so I would leave A alone unless you want the opposite." } ], "out_of_scope_findings": [ "filed as #26: a duty's timezone has no source to resolve from — sys_user declares no timezone/locale field and ExecutionContext.timezone is not propagated into the action-handler context (buildSession carries userId/organizationId/positions/roles only), so every instantiated duty gets UTC; distinct from #24 (which is about validating the value, not producing it), and it changes what #20's deployment docs must say", "filed as #27: object-less ('global') actions have no UI home in protocol 17 — global_nav was retired from ACTION_LOCATIONS (#6888) and every surviving location is object-bound, so the product's biggest adoption path is reachable only over POST /api/v1/actions/global/... or MCP, with no button; options laid out including an object-bound twin on duly_catalog_item's list_toolbar", "filed as #30 (sub-issue of #8): both catalog actions are ungated — no requiredPermissions, and ctx.engine is the trusted RLS/FLS-bypassing facade, so the invoke-time capability gate is the only boundary; needs the duly_admin capability #8 creates, hence attached there rather than free-standing", "commented on #24 (no new issue — it corrects a claim in an existing one): duly_catalog_item does NOT carry a timezone field (grep -c timezone -> 0; its fields are name, description, position_code, form, frequency, due_anchor, due_offset_days, lead_days, grace_days, regulation_ref, active), so that validation has one home rather than two" ] }
Generated by Claude Code
os-dev-report
(Supersedes the previous comment on this issue — same report. The HTML-comment marker did not survive GitHub's sanitizer, so this one leads with the literal text instead. No comment-edit route is available to this session: the agent proxy allows only the MCP GitHub tools, which have no edit-comment method, and a direct REST
PATCHreturns 403 "GitHub access is not enabled for this session".){ "issue": 5, "status": "done", "branch": "claude/issue-5-catalog-instantiate", "pr": "https://github.com/objectstack-ai/duly/pull/34", "premise_still_valid": true, "summary": "Implemented duly_catalog_apply and duly_catalog_sync as object-less script actions plus their handlers, registered inside the existing registerDulyActionHandlers (objectstack.config.ts untouched, register-handlers.ts shape preserved for #4). Apply is idempotent on (catalog_item, owner) with a same-run guard; sync writes only the five cadence fields, never touches source:'self' duties, and reports duties from deactivated catalog items instead of deleting them. Two PM mechanism assumptions were tested and HOLD: the global-action input surface CAN express position_code + a multi-user picker (confirmed in the built artifact), and sys_user_position.business_unit_id IS the real BU anchor (defined in @objectstack/plugin-security, not platform-objects). The third is WRONG and is the finding that matters: sys_user carries no timezone field AND ExecutionContext.timezone is not propagated into an action handler's context, so a duty's zone has no source at all and resolves to UTC — filed as #26, and it changes what #20's deployment docs must say. Separately, a global action has no UI home in protocol 17 (global_nav retired, every surviving location object-bound), so this onboarding flow is API/MCP-only with no button — filed as #27, reported rather than redesigned around.", "tests": "All four gates green on 13feae8 (= PR head sha, clean tree, run after the final commit) via the shared verify lock: `pnpm validate && pnpm typecheck && pnpm test && pnpm build` -> os-verify-lock VERDICT command-exit 0. Verdict lines as the gates printed them: validate '✓ Validation passed (316ms)' with 'UI: 1 Apps 5 Views 2 Actions'; typecheck `tsc --noEmit` no output; test 'Test Files 2 passed (2) / Tests 36 passed (36)'; build '✓ Build complete' writing dist/objectstack.json 53.5 KB. 36 tests total (28 new + 8 pre-existing invariants), covering all five acceptance criteria: 26 items x 3 users = 78 created all source:'catalog' with catalog_item set and 78 distinct (item,owner) pairs; re-apply = 0 created / 78 skipped with engine.inserts.length asserted unchanged; editing due_offset_days then syncing updates all 3 derived duties while owner/status/timezone/effective_* are asserted untouched AND every update patch is asserted to name only cadence keys; a source:'self' duty with catalog_item set is never touched; deactivating an item reports 3 retired with zero updates and zero deletes. TWO ABLATIONS, both with a `trap ... EXIT INT TERM` restore, both confirmed on disk by grep counts of the removed and injected text (no reliance on an editor exit code), both restored byte-identically (clean `git status`). No rebuild was needed or claimed: the tests import ../src/** directly, so nothing resolves through dist/. (1) Commenting out `registerCatalogActionHandlers(ql);` (marker count 1->0, ABLATED marker 1): `pnpm validate` exit 0 '✓ Validation passed' while `pnpm test` exit 1 on '× every declared action has a registered handler' — this is the point of the wiring test, since validate provably cannot see the missing registration. (2) Removing the in-code `if (text(duty?.source) !== 'catalog') continue;` guard (count 1->0): `pnpm test` exit 1 on '× the source guard is in the code, not only in the query filter', and ONLY that test — the sibling self-duty test stayed green because the fake engine's where-clause still filtered, which is exactly why the filter-blind-driver test exists. Exit codes were captured before any pipe (redirect-then-capture), and each result is quoted from the gate's own printed line.", "files_changed": [ "src/actions/catalog.actions.ts (new)", "src/actions/catalog.handlers.ts (new)", "src/actions/index.ts (added both to dulyActions)", "src/actions/register-handlers.ts (one call added inside the existing function; _ql renamed to ql so it can be used — shape otherwise untouched for #4)", "test/catalog-instantiate.test.ts (new, 28 tests)" ], "deviations": [ "duly_catalog_sync takes an OPTIONAL position_code that the issue did not specify. Omitted = the org-wide behaviour the issue describes; passed = narrow the sweep. Rationale in open_questions #2.", "No requiredPermissions on either action: the app declares no permission sets yet (dulyPermissionSets is empty), so naming a capability would leave a gate that grants nothing. Filed as #30, a sub-issue of #8, which is where those permission sets get created.", "No ai.exposed on either action. Not needed for reachability (the REST action route works without it), and arming an agent to bulk-create duties for arbitrary users is a deliberate decision, not a side effect. Listed as an option in #27.", "No changeset — this repo has no .changeset/ and no changeset gate, per the dispatch brief." ], "open_questions": [ { "question": "Should a duty fall back to sys_user.primary_business_unit_id when the person has no anchored sys_user_position row? Today it is created with NO business unit (not null), which silently costs the rollup.", "options": [ "A. Keep the declared anchor only (shipped). sys_user_position.business_unit_id, else no business_unit. Matches the issue's mapping table exactly and keeps 'do not require a sys_user_position row' true.", "B. Add sys_user.primary_business_unit_id as a documented second rung. It is a real, declared platform field on sys_user, and day one — before positions are modelled — it is the ONLY anchor a person has, which is precisely the state the issue says must work." ], "recommendation": "B, but as a follow-up rather than in this PR. Real business need: with position_code free text by design, the no-position-row case is the normal day-one state, so option A means the first customers' duties roll up nowhere and the manager dashboard (#10) shows empty units for a reason nobody can see. Long-term soundness: two declared platform anchors with an explicit precedence is a resolver, not a tolerant fallback — the assignment-level anchor documents itself as nullable ('Null = unanchored'), so consulting the user-level one is reading the platform's own second answer, not inventing one. AI-written-metadata safety: neutral. Startup scope discipline: this is the axis that keeps it OUT of this PR — the issue names one anchor, the PM said to report a different shape rather than redesign around it, and resolveBusinessUnit() is a single named function so adding the rung is one line once you rule." }, { "question": "duly_catalog_sync takes an OPTIONAL position_code that narrows the sweep. The issue did not specify sync's input; omitting it syncs every catalog-sourced duty in the org, as the issue's text describes.", "options": [ "A. Keep the optional narrowing (shipped) — omit for the org-wide behaviour the issue describes, pass it to correct one position.", "B. Drop the param and make sync always org-wide, matching the issue's literal text with no added surface." ], "recommendation": "A. The issue itself says sync 'is destructive to authored cadence, so it must be legible after the fact', and the same reasoning applies before the fact: a one-position correction should not be able to rewrite cadence on every duty in the org because that is the only shape available. It is one optional param on the already-declared surface, it costs nothing when omitted, and it mirrors apply's input so the pair reads as one feature. Flagging it because it is an input the issue did not specify — say the word and I will drop it." }, { "question": "Apply's idempotency probe treats ANY existing duty for a (catalog_item, owner) pair as taken, without qualifying on source or status — so a source:'self' duty with a stray catalog_item, or a retired catalog duty, suppresses a fresh create.", "options": [ "A. Literal reading (shipped) — the issue's rule is the '(catalog_item, owner)' pair, unqualified; a second row for that pair is the duplicate the rule exists to prevent, whatever wrote the first.", "B. Qualify the probe on source = 'catalog' so a self-declared duty cannot mask a catalog one.", "C. Also qualify on status != 'retired' so re-applying re-creates a duty someone retired." ], "recommendation": "A, which is what shipped and what the issue's wording says. Flagging it because B and C are both defensible and the difference only shows up on messy data. C in particular is a real product question: if an admin retires a duty and then re-runs onboarding, should the duty come back? Silently resurrecting it would undo a deliberate local decision, which is the same principle that keeps sync off status — so I would leave A alone unless you want the opposite." } ], "out_of_scope_findings": [ "filed as #26: a duty's timezone has nothing to read it from — sys_user declares no timezone/locale field and ExecutionContext.timezone is not propagated into the action-handler context (buildSession carries userId/organizationId/positions/roles only), so every instantiated duty gets UTC; distinct from #24 (which is about validating the value, not producing it), and it changes what #20's deployment docs must say", "filed as #27: object-less ('global') actions have no UI home in protocol 17 — global_nav was retired from ACTION_LOCATIONS (#6888) and every surviving location is object-bound, so the product's biggest adoption path is reachable only over POST /api/v1/actions/global/... or MCP, with no button; options laid out including an object-bound twin on duly_catalog_item's list_toolbar", "filed as #30 (sub-issue of #8): both catalog actions are ungated — no requiredPermissions, and ctx.engine is the trusted RLS/FLS-bypassing facade, so the invoke-time capability gate is the only boundary; needs the duly_admin capability #8 creates, hence attached there rather than free-standing", "commented on #24 (no new issue — it corrects a claim in an existing one): duly_catalog_item does NOT carry a timezone field (grep -c timezone -> 0; its fields are name, description, position_code, form, frequency, due_anchor, due_offset_days, lead_days, grace_days, regulation_ref, active), so that validation has one home rather than two" ] }
Generated by Claude Code
ACCEPT — PM review of #34 after one rework round.
The NUL is gone and verified on the pushed blob, not just locally:
git cat-file -p origin/claude/issue-5-catalog-instantiate:src/actions/catalog.handlers.ts | grep -caP '\x00'→0, and a sweep of the whole tree for the wider control-char class[\x00-\x08\x0b\x0c\x0e-\x1f]comes back empty.git diff --statnow renders a real456 +++++instead ofBin 0 -> 17555 bytes— the file is reviewable again, permanently. Separator semantics unchanged, collision argument preserved in a comment.Gates re-run with
origin/mainmerged in (so against the landing state, with #1, #3 and #6 present): all fourEXIT=0, 175 tests,2 Actions. CIverifysuccesson the new head.The ablation I ran myself, because this is the failure mode with no author-time gate. With
dist/objectstack.jsonon disk at 64,251 bytes, I commented outregisterCatalogActionHandlers(ql):pnpm validate→ exit 0, green. An action that renders, is clickable, and fails at call time, and the author-time gate has nothing to say about it.pnpm test→ exit 1, exactly one failure:× every declared action has a registered handler.
That test is the most valuable thing in this PR. It is the only gate against the defect, and it is hermetic under a stale artifact.
On your two disclosures. Both were worth writing down, and the second is the more useful one: a
trap 'git checkout -- "$F"'restores from the index, so running an ablation over an uncommitted fix silently reverts the fix and reports success. Exit 0, no output, work gone. That is the same shape as the shared-stash hazard inAGENTS.md— a restore mechanism that succeeds while giving you back the wrong content — and "commit before reverse-verification" is the rule that closes both. Restoring fromHEADwas the right correction.Not scanning your own source with
file(1)before the first push is a fair self-assessment and the cheapest possible check. I have queued it as a repo-level guard rather than leaving it to discipline — three occurrences in one session is a tooling gap, not a concentration lapse.Your three findings are all acted on: #26 goes to Warren as a product decision (a duty's timezone having no source at all is a data-model question for a product sold worldwide, not an implementation detail). #27 I have adjudicated — the object-bound twin on
duly_catalog_item's list toolbar. #30 stays attached to #8, correctly.Open questions: Q1 → B, as a follow-up, and your reasoning for keeping it out of this PR is right. Q2 → A, keep the optional narrowing; "destructive to authored cadence, so it must be legible" cuts before the fact as well as after. Q3 → A as shipped; silently resurrecting a retired duty would undo exactly the kind of deliberate local decision that keeps sync off
status.Merging.
Generated by Claude Code
- added 2 commits that reference this issue
on Sep 1, 2026
The onboarding path, and the single biggest adoption risk in the product. Customers arrive with their catalog already written — usually a spreadsheet. If taking a position means hand-typing 26 duties, the rollout dies in week one.
Files you own
src/actions/catalog.actions.ts(new), added todulyActionsinsrc/actions/index.tssrc/actions/catalog.handlers.ts(new)src/actions/register-handlers.ts— add your registration call inside the existing functiontest/catalog-instantiate.test.ts(new)duly_catalog_apply— global actionInput: a
position_codeand one or moresys_userids.For each active
duly_catalog_itemwith thatposition_code, create aduly_dutyfor each selected user:name,description,form,frequencydue_anchor,due_offset_days,lead_days,grace_daysownerbusiness_unitsys_user_position.business_unit_idanchortimezoneUTCsource'catalog'catalog_itemstatus'active'Idempotent. Applying twice creates nothing the second time — skip any item that already has a duty for that
(catalog_item, owner)pair. Report counts: created, skipped.duly_catalog_sync— global actionReplays cadence edits from the catalog onto duties already instantiated from it. Updates
frequency,due_anchor,due_offset_days,lead_days,grace_dayson everyduly_dutywheresource = 'catalog'andcatalog_itempoints at a changed item.Does not touch
owner,status,timezoneoreffective_*— those are local decisions the catalog has no business overwriting. Does not delete duties for retired catalog items; report them instead so a human decides.Return a summary of what changed, per duty. Sync is destructive to authored cadence, so it must be legible after the fact.
Notes
position_codeis free text on purpose: a customer can load their catalog on day one, before positions are modelled in the platform. Do not require asys_user_positionrow to exist.roleis a reserved word in the platform vocabulary and the author-time linter rejects it — the field isposition_code, and any new field you add must avoid the word too.Acceptance
source: 'catalog'withcatalog_itemsetdue_offset_daysthen syncing updates every derived duty and leavesowner/statusuntouchedsourceis'self'is never touched by sync, even if itscatalog_itemis somehow setGates
pnpm validate && pnpm typecheck && pnpm test && pnpm build.