Skip to content

fix(plugin-audit): resolve reference titles in activityMilestones summary tokens (#7290) - #7333

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-7290-milestone-token-lookup-titles
Aug 10, 2026
Merged

fix(plugin-audit): resolve reference titles in activityMilestones summary tokens (#7290)#7333
os-zhuang merged 2 commits into
mainfrom
claude/issue-7290-milestone-token-lookup-titles

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes #7290

matchMilestone (ADR-0052 §5b.2) interpolated a milestone summary's {token}s from the after-row by calling displayFieldValue with no title map, so a lookup / master_detail / user token fell through to String(value) — the raw 32-char id. A milestone declared as { field: 'stage', value: 'closed_won', summary: 'Deal won by {owner}' } shipped Deal won by oBK25… to the record timeline.

The milestone branch takes precedence over the tracked-change branch, so on every object that declares activityMilestones this was the string users actually saw, and #7230 / PR #7291's fix never reached them. Closing that is what this card is.

Anchors re-verified at the branch point (7fa2aae45)

The card recorded its anchors at a768f81a1 (pre-#7230) and warned the file had moved three times in a day. It had:

anchor card (a768f81a1) at 7fa2aae45
displayFieldValue :308 :344 (gained #7230's titlesFor param)
matchMilestone :355 :472
the {token} interpolation :368-375 :485-490
matchMilestone call site :872 :1071

The premise itself was re-verified rather than taken on trust: at the branch point the interpolation still read return field ? displayFieldValue(field, v) : String(v) — two arguments, no title map — and matchMilestone had exactly one caller, already inside an async function. Premise valid, unchanged by #7291.

Placement — re-derived, and measured both ways

The card recommended resolving after a milestone fires, keyed on the matched template's tokens, at the cost of making matchMilestone async. That direction is right and is what shipped. The async part turned out to be avoidable, so it was avoided:

matchMilestone is split rather than made async — detection (matchMilestone, still sync, no longer takes fields), planning (planMilestoneTokenReads), rendering (renderMilestoneSummary). The call site awaits one read between them. This gives the recommended read profile with zero async blast radius, and it mirrors the planTrackedLookupReads + resolver shape #7291 established one commit earlier in the same file rather than inventing a second one beside it.

resolveTrackedLookupTitles became resolveLookupTitles(api, plan) — it takes the plan instead of building it. Both branches now end in one read/mask/title implementation, which is what keeps #6977's masking contract from being re-typed for the milestone path.

Read counts, measured with the copy-returning counting driver

write shape added reads
create (any) 0
delete (any) 0
update of a milestone-declaring object that fires nothing, on a row holding references 0
non-stage update of a milestone-declaring object, row holding references 0
repeat write while the record already sits at the milestone value 0
fired milestone whose template names no reference token 0
fired milestone whose reference token is empty 0
fired milestone, 1 reference token 1 on that target
fired milestone, 2 tokens onto the same target 1 (batched)
fired milestone, 2 distinct targets 1 each
the same token twice 1
target designating a masked field as its title 0 (read skipped)

The rejected placement, measured rather than argued. Resolving before knowing whether a milestone fired was implemented as mutation M3 and run: on a non-firing update of a row holding references it measured 3 reads (sys_user 1, crm_account 1, crm_region 1; crm_vault 0, its title being masked) where the shipped shape pays 0. That is the shape #6656 / PR #6977's Option A+ ruling was obtained to remove from this write path, so it was not adopted.

Why the milestone plan does not filter on trackHistory. #7230's gate is "a field that is BOTH tracked AND a reference actually moved". A milestone token is none of those: it is the author's own token, read from the whole after-row. The reported declaration itself ('Deal won by {owner}') names a field that need not be tracked at all, so reusing that gate would leave the exact reported defect shipping — mutation M4 below measures it.

#7291 and #6977 are re-asserted, not assumed

audit-lookup-summary.test.ts and audit-bound-previous.test.ts are untouched and green, and #6977's single-id count is re-asserted inside the new file on the firing path (still exactly one findOne on the audited object). ledgerView, the secret mask and the virtual-field drop are unchanged; the milestone read plan is built from the same masked after-view the interpolation renders from.

crm_vault in the new fixture declares nameField: 'api_key' on a real secret field (with a crypto provider wired, so the column is genuinely encrypted rather than conveniently empty). The read is skipped, not masked afterwards.

What did not change

The author's template wording is theirs: only what a token resolves to changes. Punctuation, emoji, and the empty-token rule are byte-identical — an empty value still renders as the empty string, not displayFieldValue's , because a milestone sentence is prose and not a diff row. Milestone templates are still not translated. Select option-label rendering is untouched — localizing those is #7289, deliberately held behind this card in the same file and left alone here.

Tests

New file packages/plugins/plugin-audit/src/audit-milestone-summary.test.ts — 28 cases on a real ObjectQL engine with the copy-returning counting driver (a driver handing back live store references lets the read path rewrite the store under a measurement).

The label/locale fixture is a real createMemoryI18n bundle, and that earned its keep immediately: the first draft wrote {object} in the bundle and measured 已更新{object}"{label}" — the shipped bundles interpolate {{param}}, which is deliberately not the milestone template's single-brace {token}. A stub translator would have hidden that.

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

Family gates, all green locally: check:nul-bytes, check:durability-log-level, check:engine-double-contract, check:error-code-casing, check:route-envelope, check:wildcard-fallthrough, check:empty-changeset, check:adr-0087-registration, check:changeset-gate-self-tests.

Reverse verification — direction predicted in writing before the first mutation

Predictions were written to a file and the tree committed before any mutation ran.

# mutation predicted measured
M1 drop the title map from renderMilestoneSummary (the pre-#7290 interpolation, plan and read kept) 10 red: 9 rendering cases + the trailing summary assertion inside one read-count case; every pure count case green as predicted — 10 red / 18 green, the same 10
M2 key the milestone plan by field instead of by target object 1 red (the same-target batching case); the repeated-token case green wrong — 14 red. See below
M2' issue one read per id instead of one batched id: { $in: [...] } isolates batching only 3 red: my same-target case and both of #7291's batching cases, which also confirms the shared resolver really is shared
M3 the rejected placement — resolve before matchMilestone 10 red, all the zero/exact-count guards on the reference-holding row; rendering untouched as predicted — 10 red / 18 green, the same 10, and the measured cost is the 3 reads quoted above
M4 wrongly reuse #7230's trackHistory gate on the milestone plan 14 red as predicted — 14 red / 14 green

M2 is the one I got wrong, and the reason is worth recording. I predicted the mutation would only coarsen batching. It did not: the plan map's key is the object name resolveLookupTitles reads from and the key renderMilestoneSummary looks titles up by. Keying it by field therefore made the resolver query objects named owner / account_id, the best-effort catch swallowed the failures, and no title resolved at all — 14 red, mostly rendering. The mutation was wrong, not the code. M2' was written to isolate the property M2 was supposed to test, and it does. My sub-prediction inside M2 — that the repeated-token case pins token dedup rather than object grouping, so it should stay green — held under M2'.

The trap that PR #7291 documented is live here and was designed around, not discovered. Every zero-read guard runs against deal_2, a row that already holds all six reference fields; one case (a reference token whose value is EMPTY) necessarily runs against the reference-free deal_1, and it is the one case that stayed green under M3. That is green-for-the-wrong-reason, predicted as such in writing beforehand and flagged in the file rather than counted as coverage. Likewise the raw-id fallback case was written from the start as a mixed assertion (Won by usr_1 for APAC — unresolvable side stays raw, resolvable side must still become a title), because #7291 measured that a single-token fallback assertion survives deletion of the render limb; under M1 it went red, as intended.

Deliberately not done


Generated by Claude Code

…mary tokens (#7290)

`matchMilestone` (ADR-0052 §5b.2) interpolated `{token}` from the after-row
with no title map, so a `lookup` / `master_detail` / `user` token fell through
to the raw id: `Deal won by oBK25…`. The milestone branch takes precedence over
the tracked-change branch, so #7230's fix never reached milestone-declaring
objects.

`matchMilestone` is split into detection (`matchMilestone`, still sync),
planning (`planMilestoneTokenReads`) and rendering (`renderMilestoneSummary`),
so the read plan is built from the tokens of the template that actually fired.
`resolveTrackedLookupTitles` becomes `resolveLookupTitles(api, plan)` — one
read/mask/title implementation shared by both summary branches.

Measured with a copy-returning counting driver: 0 added reads on every create,
every delete, every non-firing update of a milestone-declaring object and every
fired milestone naming no reference token; 1 read per distinct target object on
the transition itself. The rejected before-match placement measured 3 reads on
a write that fires nothing.

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 7:32am

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.

…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
os-zhuang marked this pull request as ready for review August 10, 2026 07:40
@os-zhuang
os-zhuang added this pull request to the merge queue Aug 10, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⛔ merge queue 构建失败 — 先分诊,再决定要不要重排

队列构建 31366863728 红了。队列跑的是全量套件(PR 侧 CI 只跑 affected 子集),
所以失败的测试可能在本 PR 没碰过的包里 —— 那不是重排能修的。每次盲目重排都会让排在后面的所有 PR 重建一轮。

失败的 job(日志抽取,best effort):

  • Test Core (2/3) — 失败步骤: Run this shard's tests

    �[41m�[1m FAIL �[22m�[49m test/watch-dot-root.test.ts�[2m > �[22mFileSystemRepository watcher — dot-rooted watch root (#7150)�[2m > �[22msees an external edit when the root is under a dot-directory
    

历史信号:

  • 本 PR 过去 24h 无队列失败记录(首次)。
  • 过去 24h 队列共有 5 个失败构建(不含本次)。

分诊清单:

  1. 失败测试在本 PR 改动的包里 → 真回归,修 PR。
  2. 失败测试与本 PR 无关 → 在其他 PR 的同类评论里搜同名测试;出现过 ⇒ flaky 实锤,开 issue 修/隔离那条测试。修好前重排只会再烧一轮全队列。
  3. 两者都不是 → 可能与同组 PR 语义冲突;等前面的 PR 落地或失败出队后再重排一次即可,不要连续重排。

Generated by Claude Code · merge-queue-triage workflow (#4859)

Copy link
Copy Markdown
Contributor Author

Merge-queue ejection triaged — this is a deliberate single re-queue, not a blind one.

Walking the merge-queue-triage checklist rather than hitting re-queue:

Step 1 — is the failing test in a package this PR changed? No. This PR changes three files, all @objectstack/plugin-audit plus a changeset; the failure is packages/metadata-fs/test/watch-dot-root.test.ts. No import path, no shared fixture, no shared global.

Step 2 — has the same test appeared in another PR's queue-failure comment? Not in the 60 most recently updated PRs — this is the first recorded sighting, so I cannot claim "flaky 实锤" on a repeat. Filed #7369 anyway, because step 2 asks for the issue regardless and the analysis holds without a second sighting: the case races a chokidar delivery against two fixed wall-clock budgets (a sleep(400) clearing a 200 ms self-write suppression window, then Promise.race against EVENT_WAIT_MS) and asserts an exact count of 1, so a late or suppressed delivery yields 0. The queue runs the full suite while PR-side CI runs the affected subset, which is why this test never ran on this PR's own CI — Test Core (1/3, 2/3, 3/3) were green on both head commits (c6871848e, 508a96643). The test landed at 04:12:38Z today (684ab2218, #7208 / #7150), under four hours before this build.

Step 3 — semantic conflict with the batch? Nothing in the diff can interact with a filesystem watcher.

Action: re-queued once. If it ejects again on the same test, that is the second sighting, and I will stop re-queueing and escalate on #7369 rather than burn further full-queue rounds — the checklist is explicit that repeated re-queues rebuild everything behind me.

Nothing about this PR changed: still the two reviewed commits, still 23 success + 2 skipped-by-design on head 508a96643.


Generated by Claude Code

@os-zhuang
os-zhuang added this pull request to the merge queue Aug 10, 2026
Merged via the queue into main with commit 3c03725 Aug 10, 2026
26 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-7290-milestone-token-lookup-titles branch August 10, 2026 08:27
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

Development

Successfully merging this pull request may close these issues.

plugin-audit: activityMilestones summary templates still interpolate raw lookup ids — {owner_id} renders oBK25… after #7230

2 participants