do_not_call is declared on lead and contact but enforced nowhere, while its twin email_opt_out is enforced three times #1180

Activity

1. os-steve

   added

   bugSomething isn't workingSomething isn't working

   metadataDeclarative metadata — schema, security posture, UI surfacesDeclarative metadata — schema, security posture, UI surfaces

   pm:queueReady for the PM dispatch loopReady for the PM dispatch loop

   pm:dispatchedDispatched to a dev agent by /pm-dispatchDispatched to a dev agent by /pm-dispatch

   and removed

   pm:queueReady for the PM dispatch loopReady for the PM dispatch loop

   on Aug 17, 2026
2. os-steve

   self-assigned this

   on Aug 17, 2026
3. os-steve commented on Aug 17, 2026

   

   os-steve

   on Aug 17, 2026 – with Claude

   CollaboratorAuthor

   More actions

   Claim: PM loop round 1
   Session: session_01XeAz8tSjwAibjdSDn9niiZ
   Branch: claude/issue-1180-do-not-call-enforcement
   Worktree: hotcrm-issue-1180
   Domain: metadata
   File surface: src/actions/ (global.actions.ts, contact.actions.ts, lead.actions.ts), src/flows/schedule-followup.flow.ts, test/ (stop on breach; explain in the report)
   Container & model: M, mode:subagent, model: opus
   Serial constraints cleared: no in-flight PR or claim touches src/actions/; sibling batch cards #1181 (src/objects/account.*, src/flows/contract-renewal.flow.ts) and #1183 (scripts/, CI) have disjoint file surfaces and neither pins behavior this card asserts. #1182 (decorative-field sweep) is held to a later batch — it touches object/view files, not the action surface, but shares the enforce-or-remove ruling, so it runs after this lands.

   ---

   Generated by Claude Code
4. os-steve commented on Aug 17, 2026

   

   os-steve

   on Aug 17, 2026 – with Claude

   CollaboratorAuthor

   More actions

   os-dev-report

   {
     "issue": 1180,
     "status": "done",
     "branch": "claude/issue-1180-do-not-call-enforcement",
     "pr": "https://github.com/objectstack-ai/hotcrm/pull/1192",
     "premise_still_valid": true,
     "summary": "Premise re-verified on main @ 2342811a: the consumer scan returned only the two Field.boolean declarations, zero enforcement sites. Enforced do_not_call on both crm_lead and crm_contact via two beforeInsert/beforeUpdate guards that refuse to SCHEDULE a phone touch — task_do_not_call_guard (open crm_task of type 'call') and event_do_not_call_guard (planned crm_event of type 'call') — both reading the effective value so retyping and re-parenting are closed too. Recording a call that already happened is deliberately never refused: a completed Call task, a held Call event, and the log_call action all pass untouched, because refusing them hides the evidence of a call rather than preventing one and penalises the rep who logs honestly. TWO PM ASSUMPTIONS MEASURED, ONE FALSIFIED: (1) the visible: predicate transfers structurally but is only a RENDERING hint — it leaves the write reachable over REST, AI tool calls, flows and imports, so it was rejected on architecture in favour of enforcing on the write ('one writer, not one per entry point', the repo's own principle in global.actions.ts); (2) schedule_meeting is NOT phone outreach — it hardcodes eventType: 'meeting', so gating it would widen do_not_call into do_not_contact, and it is deliberately left ungated with a test pinning that. schedule_followup needed no change: the phone OPTION cannot be gated (a screen field's options are static metadata with no record in scope), and the flow reaches crm_task through the very write the guard sits on. SCOPE NOTE: measurement moved the correct fix out of the claim's declared file surface (src/actions/, schedule-followup.flow.ts, test/) — an in-surface fix could not have covered crm_contact at all, and 'both objects' was non-negotiable — so src/objects/task.hook.ts and src/objects/event.hook.ts are touched instead. Both are unclaimed by the sibling batch cards (#1181 = src/objects/account.*, #1183 = scripts/CI); #1181 has since landed on main with ZERO file overlap with this branch. Flagged in the PR body for the reviewer rather than assumed. Docs: content/docs/sales/contacts.mdx stated the exact opposite ('A marker only. Nothing in the product blocks a call, or the logging of a Call task') — corrected in en, zh-Hans and zh-Hant.",
     "tests": "All gates run at the FINAL commit 5a7cb0cb, all exit 0 (captured individually, not through a pipe): validate=0 typecheck=0 lint=0 i18n-gate=0 hygiene=0 stackblitz-lock=0 build=0 test:coverage=0. Full suite: 'Test Files 107 passed (107) / Tests 2624 passed | 1 skipped (2625)'. Per #1149 the local 'pnpm verify' is NOT the full CI set, so scripts/check-stackblitz-lock.mjs (ci.yml) and test:coverage (CI runs this, not 'test') were added; NOT run locally: e2e.yml (Playwright), codeql.yml, link-check.yml, docs-app.yml. New file test/do-not-call-enforcement.test.ts, 17 tests. WHICH PATH: these drive the SHIPPED lowered body via runHookBody (extractHookBody + QuickJSScriptRunner + hookBodyRunnerFactory), not the in-process handler — so repo issue #1167 is PARTIALLY FALSIFIED: its claim holds for the hook-harness.ts tests but the runHookBody path already exists and this file uses it. REFUSAL EVIDENCE / #1075: the assertion is weaker than the card asked, and deliberately so. Every business rejection in this app is a bare 'throw new Error(msg)' (17 across src/objects/*.hook.ts) and these guards match that idiom; #1075 (open, not fixed here) means code/status reach REST undefined. The test therefore MEASURES both — expect(err.code).toBeUndefined() / expect(err.status).toBeUndefined(), each with the message '#1075 has landed — tighten this assertion' — as a tripwire that goes red when #1075 lands, and pins the error message, which is the entire contract a caller gets until then. REVERSE VERIFICATION, direction predicted before each run and matched exactly: (a) neutering the task guard's flag check → predicted task refusals red / event refusals green, observed 5 failed | 12 passed (4 task refusals + the #1075 probe); (b) making effective() read input only → predicted only the two single-key update tests red, observed 2 failed | 15 passed. Restored from the commit both times (git checkout <branch> -- <path>), tree confirmed clean against HEAD. CONSISTENCY SWEEP, re-run post-implementation: 'grep -rn do_not_call src/ --include=*.ts | grep -v views/ | grep -v translations/' now returns 17 lines — the 2 original declarations (lead.object.ts:291, contact.object.ts:172) plus real enforcement in event.hook.ts (name/fields/predicate at :238,:273,:279) and task.hook.ts (:354,:397,:406), the rest being the guards' doc blocks. No changeset exemption used: skip-changeset does not apply to a behavior change, and .changeset/do-not-call-enforcement.md ships with the PR.",
     "open_questions": [],
     "out_of_scope_findings": []
   }

   ---

   Generated by Claude Code
5. os-steve commented on Aug 17, 2026

   

   os-steve

   on Aug 17, 2026 – with Claude

   CollaboratorAuthor

   More actions

   ACCEPT — PM review of #1192 (round 1). Verified against GitHub and the branch, not the report. All 9 checks green, mergeable_state: clean.

   First, a PM correction: I sent a status probe claiming no PR was open and no report had arrived. Both had landed ~40 minutes earlier. I checked for the remote branch and inferred the rest instead of scanning GitHub for the PR and the report comment, which is the wrong order. The probe cost a round-trip; the premise was mine, not the dev's.

   The PM's suggested route was rejected, and the rejection is correct. I proposed mirroring email_opt_out with a visible: predicate on the outreach actions. The dev verified it would have worked mechanically — Action.visible is present on the generated activity actions and the predicate form parses — and rejected it anyway, because visible is a rendering hint: it hides a button in the Console and leaves the write reachable over REST, from an AI tool call, from a flow, and from an import. For a compliance-shaped promise in the app every marketplace fork copies, that is "enforced unless you use the mouse" — the same defect in a smaller font. Enforcing on the write (beforeInsert/beforeUpdate guards on crm_task and crm_event) covers every entry point at once. That is a better fix than the one specified, and the reasoning is the kind that should outrank a PM route.

   A second PM assumption was falsified, correctly. I listed schedule_meeting as phone outreach. It hardcodes eventType: 'meeting'; gating it would have widened do_not_call into do_not_contact — inventing a promise the app never made, which is the mirror image of the defect being closed. Left ungated, with a test pinning it. schedule_followup needed no change either: the screen field's options are static metadata with no record in scope, so the phone option cannot be gated — and the flow reaches crm_task through the very write the guard sits on. The test pins that linkage so a refactor moving the flow off crm_task surfaces here instead of silently reopening the hole.

   Other things I checked:

   • Both objects covered, as required — crm_lead and crm_contact, on both the task and the calendar half.
   • The effective() helper closes two ways in that reading input alone would miss: retyping an existing meeting task to call, and re-parenting an existing Call task onto a flagged person. Each write mentions only one of the two keys. Both are pinned as tests. This completeness was not in the card.
   • Predicate totality respected: the flag is tested === true, and the fixtures include a contact whose do_not_call key is absent — the shape a driver returns for a column never written. Had that been treated as truthy the guard would have blocked every ordinary call task in the app.
   • The log_call judgment call is pinned in both directions — the guard cannot catch status: 'held' (what log_call writes), and the action carries no visible: predicate. Asserting only one would let a future author "tighten" the other without noticing. The line drawn — refuse to schedule, never to record — is the right one: refusing a log hides the evidence of a call rather than preventing one, and penalises exactly the rep who logs honestly.
   • The No refusal in this app carries an error envelope: every business rejection reaches REST as a bare Error with code and status undefined #1075 wall was handled better than asked. I said not to fix No refusal in this app carries an error envelope: every business rejection reaches REST as a bare Error with code and status undefined #1075 and to keep the assertion at whatever level is real. The test measures code and status as undefined and labels them '#1075 has landed — tighten this assertion' — a deliberate tripwire, so the day No refusal in this app carries an error envelope: every business rejection reaches REST as a bare Error with code and status undefined #1075 lands this test goes red and forces the strengthening. A limitation converted into a mechanical reminder.
   • The No refusal test in this repo runs on the shipped path: every guard assertion uses the in-process handler, and all 37 hooks ship body-only through QuickJS #1167 limitation was avoided, not merely reported. I asked for honesty about which path the tests drive. Instead the tests run the shipped lowered bodies through the real QuickJS runner via runHookBody, and the file is registered in runtime-coverage.test.ts. That matters specifically here: both guards are body-only closures, so a body reaching for an import or a module constant would be a ReferenceError at runtime while a handler-level test stayed green.
   • Docs corrected in all three locales — the contacts page said "A marker only. Nothing in the product blocks a call"; it now describes the schedule/log split accurately.

   On the declared file-surface breach: the fix lives in src/objects/task.hook.ts and src/objects/event.hook.ts, outside the src/actions/ surface declared in the claim. Flagged in the report rather than taken silently, which is the contract. Accepted: measurement forced it (an action-side fix could not have covered crm_contact at all, and "both objects" was non-negotiable), and neither file is claimed by #1181 or #1183, nor reached by #1182's held batch.

   On not merging main: accepted as argued. Zero file overlap with #1189 (that PR touched account.object.ts, event_attendee.object.ts, seeds, locales and accounts.mdx; this one touches the two hooks and contacts.mdx), GitHub reports mergeable_state: clean, and PR checks already run against the merge commit — so merging would have invalidated the "all gates green at 5a7cb0cb" evidence to buy nothing. Refusing to leave stale gate evidence attached to a moved head is the right instinct.

   Flipping to ready and queueing.

   ---

   Generated by Claude Code
6. os-steve

   closed this as completedin #1192on Aug 17, 2026