Skip to content

fix(plugin-audit): localize the tracked-change activity label and render lookup titles instead of raw ids (#7230) - #7291

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-7230-tracked-change-summary-translate-lookup
Aug 10, 2026
Merged

fix(plugin-audit): localize the tracked-change activity label and render lookup titles instead of raw ids (#7230)#7291
os-zhuang merged 1 commit into
mainfrom
claude/issue-7230-tracked-change-summary-translate-lookup

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes #7230

sys_activity.summary is composed at write time and shipped verbatim to every consumer at once — the record discussion feed, console home activity, the header inbox, the Setup sys_activity list, and mobile/REST/SDUI. Its tracked-change branch (ADR-0052 §5b, the "[label]: [old] → [new]" template) was producing Rating Owner: ∅ → oBK25… at the bottom of an otherwise fully-localized zh-CN page. One string, two independent causes — both fixed in one file, per the card's recommendation A.

Anchors re-verified at the branch point (a768f81a1)

All three still hold exactly where the card recorded them, so nothing had to be re-scoped:

anchor card at a768f81a1
displayFieldValue :308 :308
renderTrackedChangeSummary :330 :330
tracked-change call site, no translate :882 :882
sibling branches resolving through translate :848 / :852 / :856 :847 (translate) / :848 (displayLabelFor) / :853 / :857

objectui was not touched (verified pass-through per the card), and no file under content/docs/releases/ was touched.

Label half — the branch that was never handed the translator

renderTrackedChangeSummary now takes the object name and the same locale-bound translate its three siblings (messages.activityCreated / messages.activityDeleted / messages.activityUpdated, plus displayLabelFor for the object label) already resolve through, and reads the field label on the bundles' own key shape:

translate(`objects.${objectName}.fields.${key}.label`)
  ?? authored field.label
  ?? the machine key

That is the shape zh-CN.objects.generated.ts and every other shipped bundle actually carries, so the fix is against real data rather than against a convention. A miss still returns undefined, so both existing fallbacks answer exactly as before — ADR-0053 / #3039 write-time localization, applied to the one branch that missed it.

Value half — a reference renders its record's title

displayFieldValue gains a reference branch for lookup / master_detail / user. user is included because the spec defines it as "a lookup specialized to the sys_user system object — stored IDENTICALLY (FK string column)", and it is the exact field class the symptom was reported on. tree is excluded: it carries no reference, so there is no target object to read.

The title field comes from ADR-0079's resolveDisplayField (nameField → the deprecated displayNameField alias → deterministic derivation), imported rather than re-derived. A local "try name, then title, then subject" heuristic here would be a second de-facto contract that disagrees with the record picker, the search companion and the approval inbox the day an author sets nameField — the same argument the file's existing SECRET_MASK / collectMaskedReadFields import makes for itself.

The change is restore-invariant: an id with no resolved title — target removed out of band, unregistered object, failing read, or a title field that resolves to id — renders exactly as it did before. It can replace an id with a title, never a title with an id. The ∅ → notation is unchanged, and matchMilestone's {token} interpolation passes no resolver, so its behaviour is byte-identical.

Read cost — measured, because #6977 landed in this file one day ago

#6656 / PR #6977 retired captureBefore's redundant pre-image read from this write path under maintainer ruling Option A+ (2 → 1 reads per single-id write, 3 → 0 per predicate write). A naive per-row lookup resolution would have handed that straight back. Measured with the same copy-returning counting driver, and pinned as cases rather than asserted in prose:

write added reads
create (any) 0
delete (any) 0
update moving no tracked reference field, on a row that holds references 0
update moving an untracked reference field 0
update moving 1 tracked reference 1 on that target object
update moving 2 tracked references onto the same target 1 (batched, not 2)
update moving references onto 2 distinct targets 1 each
both sides of one reference change (old id + new id) 1 — one id: { $in: [...] }, not two point reads

#6977's own counts are re-asserted alongside: the single-id update still pays exactly one findOne on the audited object, and audit-bound-previous.test.ts is untouched and green.

Three properties do the work: the read plan is built from the diff, not from the written row; it is keyed by target object, not by field or by value; and only ['id', titleField] is selected, so resolving a title cannot drag a wide row through the write path.

What batching cannot remove, stated honestly: writeAudit is dispatched per row, so a predicate update over N rows that moves a tracked reference on each pays N reads (one per row per distinct target). That is inherent to per-row summary composition, not to this batching — and unlike the pre-image read #6977 removed, it is gated on "this object declares a tracked reference field and it moved", so the default write pays nothing.

#6977's masking is not regressed

resolveDisplayField honours an explicit nameField pointer even when it points at a title-ineligible type, so an object could in principle designate a credential field as its title. Rather than trust downstream masking, the read is skipped when the resolved title field is in collectMaskedReadFields(def) — the same contract predicate ledgerView masks with, so "no credential value reaches a user-facing activity summary" holds on this path by the same definition, not a second one. ledgerView, the secret mask and the virtual-field drop are otherwise untouched; the summary still renders from the masked views, and the read plan is built from those same masked views.

Tests

New file packages/plugins/plugin-audit/src/audit-lookup-summary.test.ts — 17 cases on a real ObjectQL engine with the copy-returning counting driver lifted from audit-bound-previous.test.ts (its copy-returning rule included deliberately: a driver handing out live store references lets the read path rewrite the store under a measurement).

The two halves are pinned separately, because a test asserting only the finished string could be satisfied by fixing either one. The label half is pinned against a real localecreateMemoryI18n loaded with real objects.[object].fields.[field].label data — not a stub translator that would pass whatever key shape the code used.

One case earns a note: the raw-id fallback could not be written the obvious way. The engine's assertReferencesResolve refuses to write a dangling reference (ValidationError: Account: no crm_account record has id "acc_missing"), so the realistic unresolvable case is a reference that was valid when written and whose target has since gone away — reproduced by removing the row out of band.

pnpm --filter @objectstack/plugin-audit test       Test Files 10 passed (10)   Tests 160 passed (160)
pnpm --filter @objectstack/plugin-audit typecheck  tsc --noEmit — clean
npx eslint packages/plugins/plugin-audit --no-inline-config  — clean
pnpm --filter @objectstack/plugin-audit build      — success

Family gates run locally, all green: check:nul-bytes, check:durability-log-level (this file is in its vocabulary), check:engine-double-contract, check:error-code-casing, check:route-envelope, check:wildcard-fallthrough, check:empty-changeset, check:adr-0087-registration. check:i18n / check:i18n-coverage report PREREQUISITE NOT MET locally (they run the built CLI); this diff adds no declared label and edits no translation bundle, so neither can drift from it — CI runs them regardless.

Reverse verification — direction predicted in writing before running

Predictions were written to a file before the first mutation ran. Four mutations, each reverting one property of the change.

# mutation predicted measured
A drop the translate(...) limb from the label RED: the zh-CN label case. GREEN: the def-label fallback, all value-half and read-count cases as predicted — 2 red (both zh-CN label cases), 158 green. expected 'Owner: ∅ → 张伟' to be '负责人: ∅ → 张伟'
B delete the reference branch from displayFieldValue RED: every case asserting a resolved title. GREEN: the raw-id fallback case and both read-count cases partly wrong — see below. 10 red, 150 green
C key the read plan by field instead of by target object RED: the same-target batching case (measures 2). GREEN: everything else as predicted — 1 red, expected 2 to be 1
D resolve from the written row instead of from the diff (the #6977 regression) RED: the zero-read guard GREEN — 160/160. The guard was passing for the wrong reason. See below

Mutation B, the two things I got wrong. The raw-id fallback case went RED, not green: after the engine refused a dangling reference the case was rewritten to a mixed assertion (Account: acc_2 → Acme Corp), where the unresolvable side stays raw and the resolvable side must still become a title — so it pins both directions and cannot survive the mutation. And three read-count cases went red too. The underlying claim still held exactly as stated — no count assertion failed, every one of those failures was the trailing lastSummary assertion — so read counts really are blind to a missing render branch; my prediction was wrong at the case level only because those cases carry a second, rendering assertion.

Mutation D is the one that changed the change. It came back 160/160 green where I predicted red. Diagnosed: the hot-path cases ran against a deal holding no references, where a naive resolve-from-the-written-row implementation also reads nothing — there is nothing on the row to read. The guard was green for the wrong reason and would not have caught the exact regression it exists to catch. The fixture was hardened (a second deal seeded with account_id and owner already set), and mutation D re-run against it:

× a tracked change with NO reference field pays ZERO extra reads, on a row that HAS references
× an UNTRACKED reference field changing pays ZERO reads
AssertionError: expected 1 to be +0

Both properties now go red on the regression, and the fixture comment records why the seeded references are load-bearing rather than incidental.

Deliberately not done


Generated by Claude Code

…itles (#7230)

`sys_activity.summary`'s tracked-change branch (ADR-0052 §5b) shipped strings
like `Rating Owner: ∅ → oBK25…` onto every feed surface at once. Two causes,
both fixed here:

- `renderTrackedChangeSummary` was the one summary branch never handed the
  locale-bound `translate` its three siblings resolve through (ADR-0053 /
  #3039). The field label now resolves through it on the bundles' own key
  shape, falling back to the authored label and then the machine key.
- `displayFieldValue` resolved select option labels only, so a lookup /
  master_detail / user value printed its raw id. It now renders the referenced
  record's title via ADR-0079's `resolveDisplayField`, falling back to the raw
  id when unresolvable.

The resolution is batched: 0 added reads unless a tracked reference field
actually moved, then exactly one `id: { $in: [...] }` per distinct target
object — so #6656 / PR #6977's retirement of the redundant pre-image read from
this write path is not handed back. Read counts are pinned with the same
counting driver that measured that retirement.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BM1tNf5U3nEbHKR4fo5qVQ
@vercel

vercel Bot commented Aug 10, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Aug 10, 2026 5:08am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-audit.

3 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/deployment/cli.mdx (via @objectstack/plugin-audit)
  • content/docs/deployment/production-readiness.mdx (via @objectstack/plugin-audit)
  • content/docs/plugins/packages.mdx (via @objectstack/plugin-audit)

1 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx (via @objectstack/plugin-audit)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Aug 10, 2026
@os-zhuang
os-zhuang marked this pull request as ready for review August 10, 2026 05:25
@os-zhuang
os-zhuang added this pull request to the merge queue Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

PM review — PASS. Marked ready and enqueued. Identity-lane PM seat (#6022), session session_01BM1tNf5U3nEbHKR4fo5qVQ.

CI on head fb33dad85: 25 runs, none non-green. 3 files, +790 −8. Boundaries: objectui untouched, nothing under content/docs/releases/, and — the one this seat flagged at dispatch — #6977's work is intact: zero deletions of ledgerView / SECRET_MASK / VIRTUAL_FIELD_TYPES, 19 occurrences still present.

The question this dispatch was built around, answered with numbers

The dispatch warned that #6656 / PR #6977 (ruling A+) had retired the redundant pre-image read from this exact file the day before — 2→1 per single-id write, 3→0 per predicate write — and that a naive per-row lookup resolution would hand those reads straight back. The answer:

  • 0 added reads on every create, every delete, every update that moves no tracked reference field (measured on a row that does carry references — the right control), and every update moving an untracked reference field. fix(plugin-audit): consume the engine's bound ctx.previous and record one normalised view on both sides of the diff (#6656) #6977's counts are re-asserted in the new suite rather than assumed.
  • When a tracked reference actually moves: exactly 1 read per DISTINCT target object. Two tracked lookups onto the same object cost 1, not 2; old id and new id are answered by the same id: { $in: [...] } projecting only ['id', titleField].
  • Measured with the copy-returning counting driver lifted from audit-bound-previous.test.ts — the one that does not let the engine's read path mutate the store and make the measurement lie.

And the limit is stated rather than buried: writeAudit is dispatched per row, so a predicate update over N rows each moving a tracked reference pays N reads. That is inherent to per-row summary composition, and — unlike the pre-image read #6977 removed — it is gated on "this object declares a tracked reference field and it moved". Correct call, honestly bounded.

Mutation D is the best thing in this report

Predicted RED (resolve from the written row instead of from the diff — the #6977 regression shape). First run came back 160/160 GREEN. Rather than banking the green, the author diagnosed why: the hot-path cases ran against a deal holding no references, where a naive resolve-from-the-written-row implementation also reads nothing. The guard was green for the wrong reason and would not have caught the regression it exists to catch. Fixture hardened with a second deal carrying account_id and owner, mutation re-run, both properties now go red — and the fixture comment records why those seeded references are load-bearing.

That is the "passes for the empty reason" failure mode this lane has been chasing all session (#6964's vacuous D7 denials, #7151's decorative-pin refusal), caught by an author in their own test, against their own prediction. It is also why the read-count claim above can be trusted: the guard behind it demonstrably bites.

Two prediction misses reported rather than smoothed: under mutation B the raw-id fallback case went red because it had been rewritten into a mixed assertion (Account: acc_2 → Acme Corp, unresolvable side stays raw and resolvable side must become a title — pinning both directions), and three read-count cases went red while no count assertion failed — every failure was the trailing summary assertion, so the underlying claim held exactly as stated and only the case-level prediction was wrong. The distinction between "my claim was wrong" and "my bookkeeping about which case carries the claim was wrong" is drawn correctly.

Other judgement calls this seat agrees with

  • The label half is pinned against a REAL locale (createMemoryI18n loaded with real objects.[object].fields.[field].label data), not a stub that would pass on any key shape. The dispatch asked for this specifically; '负责人: ∅ → 张伟' is the measured output, quoted verbatim in the PR body — quoting a measurement is not a language violation, translating it would falsify it.
  • resolveDisplayField imported, not re-derived (ADR-0079), so the title rule has one owner.
  • Restore-invariant: the change can replace an id with a title, never a title with an id. matchMilestone passes no resolver, so its behaviour is byte-identical.
  • The raw-id fallback could not be authored the obvious way because the engine's assertReferencesResolve refuses a dangling reference — so it is reproduced as a reference valid at write time whose target later vanished. Finding that the obvious fixture is unauthorable, and saying so, beats a fixture that quietly tests nothing.
  • multiple: true lookups stored as an unparsed JSON string left alone, because parsing a JSON column here would be this file inventing a storage contract it does not own. In-memory arrays are unpacked from the same batch.

Follow-ups filed, unassigned, for triage

#7289 — select option labels in the same function are still unlocalized (displayFieldValue reads authored field.options[].label while bundles carry objects.[object].fields.[field].options.[value]), so after this PR a zh-CN feed shows a localized label beside an authored-language value. Same defect class, one line over, deliberately not scoped in. #7290activityMilestones templates still interpolate raw lookup ids, and the milestone branch takes precedence over the branch this PR fixed, so on milestone-declaring objects it is the string users actually see; the body costs out both read placements and recommends the after-it-fires one.


Generated by Claude Code

Merged via the queue into main with commit 5777b1a Aug 10, 2026
26 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-7230-tracked-change-summary-translate-lookup branch August 10, 2026 05:43
os-zhuang pushed a commit that referenced this pull request Aug 10, 2026
…ne non-biting zero-read case (#7290)

The `seed()` doc block claimed "Every zero-read case below therefore runs
against `deal_2`". That is false for `a reference token whose value is EMPTY
pays ZERO reads`, which must run against the reference-free `deal_1` because an
empty reference is what it measures. On a row with nothing to read every
candidate placement pays zero, so that case cannot constrain read placement —
measured, it was the one case that stayed green under the mutation moving the
read before `matchMilestone`.

This is the doc block that teaches the next reader about the trap PR #7291
measured, so as written it taught something untrue about its own file and a
reader would have counted a non-biting case as a guard. The exception is now
stated in `seed()` and flagged at the call site itself.

Comments only — no assertion, fixture, source or changeset change; counts are
unchanged and were not re-derived.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BM1tNf5U3nEbHKR4fo5qVQ
os-zhuang pushed a commit that referenced this pull request Aug 10, 2026
…e activity summary (#7289)

`displayFieldValue` rendered a select/picklist value by returning the matching
option's authored `label` from `engine.getSchema(name)` — locale-independent
metadata — while the shipped bundles carry those labels under
`objects.<object>.fields.<field>.options.<value>`. After #7230 localized the
field label, a zh-CN workspace read `阶段: Proposal → Closed Won`.

The tracked-change branch now resolves the option label through the same
locale-bound translator its field label already uses, authored label as
fallback. The fired-milestone branch is deliberately NOT localized and the
opt-out is by construction: it passes no option resolver, so its author-written
sentence and the author's own option label stay in one language.

Zero added reads on every write shape — a bundle lookup, not I/O — so #6656 /
PR #6977's retirement, preserved by #7291 and #7333, still stands, and
`displayFieldValue` stays synchronous.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BM1tNf5U3nEbHKR4fo5qVQ
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

1 participant