Skip to content

crm_product.tax_rate is inert, and the consumer scan cannot see it — a same-named field on another object masks it #1193

Description

@os-steve

The field

crm_product.tax_rate ("Default Tax Rate %") is declared, in the pricing field group, and read by nothing:

$ grep -rn "tax_rate" src/ --include=*.ts | grep -v views/ | grep -v translations/
src/objects/product.object.ts:...    tax_rate: Field.percent({ label: 'Default Tax Rate %', ... })
src/objects/quote_line_item.object.ts:...  tax_rate: Field.percent({ label: 'Tax Rate %', ... })
src/objects/quote_line_item.object.ts:...  expression: F`... * (1 + record.tax_rate / 100)`

Every hit after the first belongs to a different object. crm_quote_line_item.tax_rate is its own stored field, defaulting to 0 and typed per line; createLineItemPriceFill (src/objects/_line-item-price-fill.ts) reads only list_price from the product and never touches either tax field. So setting a default tax rate on a product changes no total anywhere.

The product documentation states the opposite, in all three locales — content/docs/revenue/products.mdx and its .zh-Hans / .zh-Hant faces:

Field Purpose
Default Tax Rate % Auto-applied to quote line items

It is not applied to anything. (That row was reworded to "the rate carried on the product record" in PR for #1182, which removed the neighbouring is_taxable; the field itself was out of that card's scope and is still there.)

Why this is filed separately from #1182

Because of how it was missed, which is the more useful half of this report.

#1182's row set came from a scan that treats a field as consumed when the token appears in a src/**/*.ts file outside views/ and translations/. That scan is line-oriented and object-blind: tax_rate matches lines in quote_line_item.object.ts, so the field reads as consumed, and it never reached the card. is_taxable — which is only ever spelled on crm_product — did reach it. The two fields are the same defect, one object, adjacent lines; only the one with a unique name was visible to the method.

That is a false negative, so it is invisible by construction: the scan's output cannot show what it filtered out. Any other field whose name is shared across objects has the same immunity. A quick check of the obvious candidates is worth doing as part of this issue rather than assuming tax_rate is the only one.

Suggested fix

Two parts, and the second is the one with leverage:

  1. Adjudicate crm_product.tax_rate under the same enforce-or-remove ruling of 2026-08-17 that Decorative-field sweep: enforce-or-remove every declared field with zero business consumers #1182 carried out. Enforce would be createLineItemPriceFill stamping tax_rate from the product the way it already stamps list_price — a few lines in a hook that exists, on the write path that already loads the product row, and it would make the documented sentence true. Remove would delete the field and the docs row. Enforce looks cheap here and matches a promise already published, but this is a published-field decision and belongs to the maintainer.
  2. Make the scan object-aware — resolve a hit to the object whose fields block it sits in, rather than to the file it appears in — so a shared field name stops granting immunity. Without this, the next sweep re-derives the same blind spot.

How it was found

Working #1182: crm_product.is_taxable was on the card and tax_rate sat two declarations away in the same field group, so the price-fill hook was read to confirm the card's claim that "crm_quote_line_item.tax_rate never reads it". It doesn't — and neither does anything read the product's own rate.

Out of #1182's declared scope (its unit of adjudication was the listed row, and this is not one). Filed unassigned; no work started.

Activity

  1. added
    metadataDeclarative metadata — schema, security posture, UI surfaces
    pm:dispatchedDispatched to a dev agent by /pm-dispatch
    and removed on Aug 17, 2026
  2. self-assigned this
    on Aug 17, 2026
  3. os-steve commented on Aug 17, 2026

    @os-steve
    CollaboratorAuthor

    Claim: PM loop round 3
    Session: session_01XeAz8tSjwAibjdSDn9niiZ
    Branch: claude/issue-1193-object-aware-field-scan
    Worktree: hotcrm-issue-1193
    Domain: metadata
    File surface: scripts/, src/objects/product.object.ts, src/objects/_line-item-price-fill.ts, content/docs/revenue/products*.mdx (all three locales), test/ (stop on breach; explain in the report)
    Scope note: #1194 is folded into this card — it edits the same three products MDX files, and splitting would create a same-file serialization for no benefit. Close both.
    Serial constraints: #1195 (#1182) merged at 540e4887, so product.object.ts is free. #1184 (comment slimming) is held behind this card because it edits the same object files. #1187 runs in parallel on a disjoint surface (README.md).


    Generated by Claude Code

  4. os-steve commented on Aug 17, 2026

    @os-steve
    CollaboratorAuthor

    Claim amendment — file surface widened, same round.

    Added beyond the claimed surface, both as bounded same-class fixes under the "extend a guard to close the whole class" rule rather than as new scope:

    • content/docs/marketing/campaigns*.mdx (3 locales) — the Standard list views section named six views (Active Campaigns, Upcoming Campaigns, Completed This Quarter, Over Budget, By Type, Best ROI) and src/views/campaign.view.ts ships four, none of them those. Identical defect to Product docs list three "standard list views" that do not exist, in all three locales #1194, one page over.
    • content/docs/service/cases*.mdx (3 locales) — the roster says crm_case ships "seven" views and lists seven; the stack ships eight. Unassigned — triage was missing from a page that claims to enumerate them all.
    • test/docs-view-rosters.test.ts (new file rather than an addition to test/docs-drift.test.ts — that file is 97KB against the repo's 100KB source-file cap, measured: appending there fails pnpm hygiene).

    Both pages are red under the new guard on the pre-fix tree and green after; neither is held by another open claim (checked the open list, and #1145 touches the triage view's filter, not the docs roster). Evidence and the reverse-verification output are in the PR body.

    Not done, and deliberately: crm_product.tax_rate is unchanged. Enforcing it is measurably wrong and removing a published field is not a dev's call, so the verdict is split out as #1198 with the measurement, so it survives this card closing.


    Generated by Claude Code


    Generated by Claude Code

  5. os-steve commented on Aug 17, 2026

    @os-steve
    CollaboratorAuthor
    {
      "issue": 1193,
      "status": "done",
      "branch": "claude/issue-1193-object-aware-field-scan",
      "pr": "https://github.com/objectstack-ai/hotcrm/pull/1202",
      "premise_still_valid": true,
      "summary": "Shipped `scripts/scan-field-consumers.ts` (`pnpm scan:fields`), an object-aware field consumer ledger that resolves each hit to the object whose declaration encloses it — walking the registered stack rather than grepping files, with hook/action bodies scanned in-blob and `api.object('x')` switching context mid-body. Report-only, not a `pnpm verify` gate: the 2026-08-17 ruling asked for a verdict per field, so gating the count would encode the blanket rule it declined to make; what is guarded instead is the scan's ability to see (test/field-consumer-scan.test.ts). It surfaces 15 inert fields, 6 masked by a shared name — 5 of them new — all listed in the PR body and filed for adjudication as #1199. `crm_product.tax_rate` is UNCHANGED: enforce was measured on the real engine and is wrong (stamping the product rate makes a quote total 200 below the sum of its own line totals, because `quote_total_rollup` never reads the per-line rate and `crm_quote.tax` is a manual amount), and removal is a maintainer decision — so the verdict request is #1198, filed as its own card so it survives this one closing. #1194 folded in: products' view roster corrected in 3 locales against `src/views/product.view.ts`, plus a new guard `test/docs-view-rosters.test.ts` requiring every shipped view to be named in its page's roster. That guard turned two more pages red, both fixed in place as same-class bounded fixes with the claim's file surface amended in the same round: campaigns (6 named views, 0 exist) and cases (roster said 'seven', stack ships eight, `Unassigned — triage` missing).",
      "tests": "All on final commit 62560210, re-run after the last commit; local scope only, CI convergence left to the PM. `pnpm validate` exit 0 (pre-existing colSpan advisories). `pnpm typecheck` exit 0. `pnpm lint` exit 0 (83 warnings pre-existing). `pnpm lint:i18n-gate` exit 0 — '0 i18n/missing-* issues'. `pnpm hygiene` clean, 304 files (5/5 checks). `pnpm hygiene:tokens` clean and UNCHANGED before/after — business semantics ~80,767 (ceiling 85,000) / interaction layer ~38,848 (42,000) / authored total ~133,465 (140,000); before == after because `git diff origin/main -- src/ objectstack.config.ts` is empty, this PR touches no src/ file. `pnpm build` exit 0 (18 Objects, 334 Fields, 14 Views). `pnpm test` — 'Test Files 112 passed (112) / Tests 2693 passed | 1 skipped (2694)', exit 0. `node scripts/check-stackblitz-lock.mjs` — 'package-lock.json is in sync with package.json (v3)', run explicitly since pnpm verify omits it (#1149). Heavy runs serialized under flock /tmp/os-heavy-verify.lock, vitest --maxWorkers=2. REVERSE VERIFICATION 1 (the scan), in a comparison worktree at c83aa744, the commit before #1195 removed the fields — predicted the removed rows would read non-live, measured exactly that: of the 12 fields #1182 deleted, 2 read inert and 10 display-only, none live; on the SAME tree `crm_product.tax_rate` read inert (read=0 display=0) while `crm_quote_line_item.tax_rate` read live (read=1), which is the discrimination the old grep could not make. Positive controls same run: crm_product.list_price read=23, crm_opportunity.amount read=56. The #1182 enforce row tracks too: crm_account.parent_account display-only pre / live now. REVERSE VERIFICATION 2 (the scan's guard test), ablation — replacing the per-object credit with an object-blind one turned exactly the 2 discrimination assertions red ('expected live to be inert') and left the 7 population-level ones green; restored via `git checkout <branch> -- <path>` from the commit, re-run 9/9 green. REVERSE VERIFICATION 3 (the docs guard), restored the 3 English pages from origin/main on top of the guard: red as predicted, naming 7 missing views across 3 pages, and the locale-parity half additionally caught the half-fixed zh faces; restored, green. NOT COMPILED: per #1169 no workflow compiles these MDX edits, so all 9 files were validated structurally instead (frontmatter + title, table column counts vs separator rows, balanced `**`, no bare `<`+letter, no bare `{` outside code spans) — all 9 OK, and section headings/line counts stay identical across the three faces of each page. The tax_rate enforce measurement booted the real engine (ObjectKernel + memory driver + shipped stack, skipSeedData) — it is scratch measurement, not a committed test, since the conclusion was to ship no code change.",
      "open_questions": [
        {
          "question": "`crm_product.tax_rate`: enforce is measurably wrong, so the remaining options are remove or redesign. Removing a published field is not a dev's call. Filed as #1198 — flagging it here because this PR closes #1193, and without #1198 the decision would disappear with the card.",
          "options": [
            "A — REMOVE the field, its 4 locale rows, the docs row in 3 locales and the crm_product field list in docs/developers/api_reference.md. Cost: it is published on an apiEnabled object, so any customer value written via REST is lost; nothing in this repo has ever written one (all 13 seeded products leave it at the 0 default), so demo org and tests are unaffected.",
            "B — KEEP it declared and inert, accepting four locale rows and a docs sentence per release plus the misreading the page carried for months (the page is now honest about it either way).",
            "C — REDESIGN: make quote_total_rollup derive `crm_quote.tax` as Σ(line subtotal × product rate) instead of reading the manual amount. Makes the field live and the numbers consistent, but changes the meaning of an existing published field on a different object and turns the seeds' explicit tax amounts into derived values. A product decision, not an enforce-or-remove verdict."
          ],
          "recommendation": "A, remove. Under the startup-focus principle it is a declared surface with no pull on all three axes: no real business need (no consumer, no seeded value, and it is not even on the product form, so no admin can set it in the UI — only the API can); poor long-term soundness (its only plausible consumer is a second, contradictory tax model this app deliberately does not have — `src/data/revenue.seed.ts` already refuses to seed a line rate for that reason, and #1195 removed `is_taxable`, so the concept it belonged to is gone); and it actively works against making AI-written metadata hard to get wrong, since a published field named 'Default Tax Rate %' that applies to nothing is precisely the affordance an AI author would wire up wrongly — which is how the docs came to promise 'auto-applied to quote line items' in three locales. C is defensible but is a real feature with a migration, not a cleanup. B keeps the trap."
        }
      ],
      "out_of_scope_findings": [
        "filed as #1198: [Decision] crm_product.tax_rate — enforce measured wrong, removal needs a maintainer ruling (carries the measurement table and removal cost)",
        "filed as #1199: the ledger of the other 14 inert fields, 5 masked by a shared name — with two clusters called out: the five crm_contact.mailing_* fields (inert AND requested on the shipped customer import template) and three description fields declared but on no form",
        "filed as #1200: the engine stores a field the object never declared — insert with an undeclared key on crm_opportunity_line_item succeeded, was persisted and was returned on read; makes every misspelled `input.<field>` in a hook a silent successful write, outside field-level permissions by construction. Driver-dependence unverified.",
        "filed as #1201: the products page contradicts itself about the datasheet — one section says no skill opens an attachment, a tip 20 lines later says the AI assistant uses it to draft customer-facing content; all three locales"
      ]
    }

    Generated by Claude Code

  6. os-steve commented on Aug 17, 2026

    @os-steve
    CollaboratorAuthor

    ACCEPT — PM review of #1202 (closes #1193 and #1194). All 9 checks green. Branch based on 540e4887, own diff is 14 files; src/ genuinely untouched, which is why the token gate reads identical before and after.

    The boundary held, and that is the most important thing in this delivery. The dispatch said: enforce is yours to make, removal is not, and if enforce turns out wrong then stop, change nothing, and report for a ruling. The dev measured enforce on the booted engine, found it wrong — stamping the product rate puts a quote total 200 below the sum of its own line totals — shipped no code change to the field, and filed #1198 as its own card so the decision survives this one closing. Filing it separately rather than leaving it in a report was the right instinct; a decision buried in a closed card's comments is a decision that evaporates.

    I verified the mechanism independently rather than taking the number: crm_quote.tax is a plain Field.currency — a manually entered amount, not derived — and quote_total_rollup discounts each line, sums, then applies the quote's own percentage, never reading a per-line rate. So a stamped line rate necessarily produces a line total that includes tax against a quote total that does not. The divergence is structural, not a fixture artifact.

    Report-only rather than a pnpm verify gate is correctly argued. The 2026-08-17 ruling asked for a verdict per field; a gate that fails on any inert field would encode the blanket rule the maintainer specifically declined to make, and would go red the moment anyone declares a field before wiring it. What is guarded instead is the scan's ability to see — which is the property that actually failed here. That distinction is subtle and the dev got it right.

    The reverse verification of the new instrument is the strongest method I have seen this session. The scan was run in a comparison worktree at c83aa744 — the tree before #1195 removed the twelve decorative fields — with the outcome predicted first: all twelve should read non-live. Measured exactly that (2 inert, 10 display-only, 0 live). On that same tree crm_product.tax_rate read inert while crm_quote_line_item.tax_rate read live: the precise discrimination the old grep could not make, demonstrated on the tree where the old grep failed. Positive controls in the same run (list_price 23 reads, amount 56) rule out a scanner that simply reports everything as dead, and #1182's enforce row tracks correctly — parent_account display-only before, live now. Validating a new instrument against a historical tree with a known answer is how you show it works, rather than asserting it.

    The ablation (VR2) is the companion half: replacing the per-object credit with an object-blind one turned exactly the two discrimination assertions red and left the seven population-level ones green. That proves the test is sensitive to the property the card exists for, not merely to the scan running.

    #1194 folded in, and the guard earned its place immediately. The dispatch said extending coverage to view-name rosters would be strictly better than fixing three sentences. It was: test/docs-view-rosters.test.ts requires every shipped view to be named in its page's roster, and on first run it turned two further pages red — campaigns named six views of which zero exist, and cases claimed seven where the stack ships eight, missing Unassigned — triage. Both fixed in place, with the claim's file surface amended in the same round rather than silently widened. Nine MDX files, validated structurally since #1169 means CI compiles none of them.

    Findings filed, all labelled and left unassigned:

    Flipping to ready and queueing.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

metadataDeclarative metadata — schema, security posture, UI surfacespm:dispatchedDispatched to a dev agent by /pm-dispatch

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions