Repository navigation
examples/app-crm: two comments reference a Discount Approval flow the app does not contain #14516
Description
Activity
Triage — graded,
findingcleared. Measured atorigin/mained44512, not taken from the card.Confirmed
examples/app-crm/src/objects/opportunity.object.ts:70— "Mirror target for the Discount Approval flow's approval nodes (ADR-0019)".examples/app-crm/src/security/sales-positions.ts:22— "Referenced by the Discount Approval second step."examples/app-crm/src/flows/holdsconvert-lead.flow.tsandindex.ts, nothing else.flows/index.tsstates the split deliberately ("One flow … Automation breadth (approvals, schedules, connectors, subflows, …) is the showcase's job").- The showcase does carry that breadth:
examples/app-showcase/src/automation/flows/approver-bindings.flow.tsanddynamic-approval.flow.ts.
So both comments name a relationship that is not in the tree, and the tree says on purpose that it never will be here. The card is right.
Correction — resolution 3 rests on a false premise, do NOT act on it
FinanceApproverPositionis not an unreferenced declaration:examples/app-crm/objectstack.config.ts:17imports it and:100registers it inpositions: [SalesRepPosition, SalesManagerPosition, FinanceApproverPosition].examples/app-crm/src/security/bind-position-sets.ts:33binds['finance_approver', 'crm_sales_user'].
The position is live scaffolding with a real consumer. What is missing is only the consumer the comment names. Enforce-or-remove does not fire here, and deleting the position would break the config and the permission-set binding. Recording this because the card invites exactly that reading and an implementer following it would remove working code.
Scope ruled
Resolution 1 only — rewrite the two comments to match the tree: point the
approval_statusnote at the showcase's approval flows as the worked example, and replace theFinanceApproverPositionnote with its actual consumer (thecrm_sales_userbinding), not an absent flow.Resolution 2 is out of scope on this card. Adding a Discount Approval flow to the CRM is a feature — human floor — and it argues against
flows/index.ts's stated one-flow scope. If anyone wants it, that is a separate card for the maintainer, not a comment fix.Routing
domain:services—examples/*anchors to the subsystem it exercises (lane table), and both comments are about approvals and positions.priority:p3: comment-only staleness, no runtime or contract effect; it earns a card rather than nothing because the examples are copy-source for AI authors, which is the card's own argument and it holds.pm:queue.
Generated by Claude Code
os-dev-report
{ "issue": 14516, "status": "done", "branch": "claude/issue-14516-crm-stale-approval-comments", "pr": "https://github.com/objectstack-ai/objectstack/pull/14840", "premise_still_valid": true, "summary": "Resolution 1 only, exactly as the triage ruling scoped it. Both stale comments existed verbatim on origin/main and both now name what is actually in the tree: opportunity.object.ts's approval_status is described as the mirror target for an approval node's `approvalStatusField` (ADR-0019; plugin-approvals reads it into `mirrorField`), pointing at examples/app-showcase/src/automation/flows/ as the worked example and at skills/objectstack-automation/SKILL.md for the wiring; sales-positions.ts's FinanceApproverPosition now names its real consumers, the objectstack.config.ts `positions` registration and the ['finance_approver','crm_sales_user'] binding in bind-position-sets.ts. The ruling's correction was re-verified on the tree before any edit and the fence held: FinanceApproverPosition is imported at objectstack.config.ts:17, registered at :100, re-exported at src/security/index.ts:6, and bound at bind-position-sets.ts:33 — all four intact on the branch head, nothing removed. Resolutions 2 and 3 were not acted on. Context for the PM, deliberately NOT filed: no example app anywhere sets `approvalStatusField` (the showcase's approval flows use approvers/behavior/lockRecord only), so the status-mirror surface has no worked example in examples/** — that is the substance of resolution 2, which the ruling reserved for the maintainer, so re-filing it would re-litigate the ruling.", "tests": "All at branch head 94d444d87, after the final commit. Under the shared verify lock: `pnpm --workspace-concurrency=2 --filter '@objectstack/example-crm^...' build` -> 'VERDICT command-exit 0' (first attempt was SIGTERMed by the 10-minute foreground cap after a 5m29s lock wait; re-run acquired in 49s and completed green). `pnpm --filter @objectstack/example-crm run typecheck` (tsc --noEmit; script name echoed, so not a zero-match no-op) -> 'VERDICT command-exit 0'. `pnpm --filter @objectstack/example-crm exec vitest run --maxWorkers=2` -> 'Test Files 5 passed (5)' / 'Tests 45 passed (45)', 'VERDICT command-exit 0'. `pnpm check:nul-bytes` -> exit 0, 'check-nul-bytes: OK (scanned 8070 text file(s) ... no raw ASCII control bytes)', plus my own grep -naP control-byte scan of both changed files (no hits). Gate family re-derived from the real diff via `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` (2 paths vs merge base 2d40f9146; it confirmed the --repo assertion against this checkout's origin) = 15 families, all run, exit codes captured by redirect-then-read: 13 green (check-comment-mask-adoption, check-cross-package-test-inputs, check-keyed-text-bounds, check-position-name-fold-loaders, check-shard-attestation, check-system-context-census, check-undeclared-dep-imports, check:examples-live-imports, check:logger-receiver-detach, check:page-declaration-shape, check:published-files, check:test-source-alias, check:type-source-resolution); 2 NOT MEASURED by their own verdict text and neither a red — check-test-completeness exit 3 ('There is no local log to hand it, so the local reading for this gate is NOT MEASURED. It is not a red'), check:dual-build-cjs-loads exit 3 ('PREREQUISITE NOT MET - this gate reads built output, and some package has no dist/ ... This is NOT a pass: nothing was measured'; it needs a full pnpm build, and it reads no file in this diff). No ablation was run: the change is comment-only, so there is no behaviour to ablate. DECLARED NARROWING - repo-wide `pnpm lint` was narrowed to the two changed files, with all three pieces of evidence: (1) population came from eslint's own config resolution, which accepted both paths as non-ignored; (2) --format json reports 2 files linted, 0 errors, 0 warnings, exit 0; (3) invariance for untouched files rests on eslint.config.mjs's own line 328, 'this repo runs one eslint.config.mjs, which never enables type-aware linting (no parserOptions.project, no typed @typescript-eslint rules) for ANY file, test or not', so a two-file comment-only diff cannot move any untouched file's verdict. COMMENT-ONLY PROOF, two independent zero-difference measurements: (a) of the 16 changed lines in git diff BASE..HEAD, the count of changed lines that are not comment or blank is 0; (b) token-stream equality using the repo's own TypeScript scanner (ts.createScanner, skipTrivia true) at BASE vs HEAD - 'opportunity.object.ts: base tokens=605 head tokens=605 identical=true' and 'sales-positions.ts: base tokens=266 head tokens=266 identical=true'. Clause 2 answer: no, the diff is comment-only. No changeset; skip-changeset applied by additive POST and confirmed by read-back (labels now size/s + skip-changeset, so the size-labeler did not strip it) - examples/app-crm is private:true and the diff is comment-only, both entries on the closed list.", "mcp_calls": "0 — gh CLI is absent in this container, a repo-scoped REST read probed 200, so every GitHub write (PR create, label POST, read-backs, this comment) went over REST.", "open_questions": [], "out_of_scope_findings": [ "NOT filed, deliberately: no app under examples/** sets `approvalStatusField`, so the ADR-0019 status-mirror surface has no worked example in the example apps. This is exactly resolution 2 of this card, which the triage ruling reserved for the maintainer as a separate feature card; filing it would re-litigate that ruling, so it is reported here for the PM instead." ] }Generated by Claude Code
Generated by Claude Code
PM ACCEPT — PR #14840, head
94d444d87. Verified against the tree, not the report.domain:servicesexecution seat, sessionsession_01AUF1NoViznQK32gqpK8wS8. R19.Ruling of record — comments read: 1, and it IS the ruling:
issuecomment-5507719028, read in full before dispatch. That mattered on this card specifically: the ruling's correction is the whole fence, and the card itself invites the opposite.⭐ The fence held — the thing this card was most likely to break
The card invites deleting
FinanceApproverPositionas an unreferenced declaration. The ruling measured that it is live and forbade it: "deleting the position would break the config and the permission-set binding … an implementer following it would remove working code."Verified by me at head — all four sites intact, nothing removed:
site at 94d444d87import objectstack.config.ts:17registration objectstack.config.ts:100—positions: [SalesRepPosition, SalesManagerPosition, FinanceApproverPosition]re-export src/security/index.ts:6declaration src/security/sales-positions.ts:28Resolutions 2 and 3 untouched, as ruled.
Comment-only — proved twice, and the second proof is the better one
- My own filter: of the 16 changed lines, 0 are non-comment/non-blank.
- The dev's independent proof is stronger and I am recording it because it is the right technique for this claim: token-stream equality using the repo's own TypeScript scanner (
ts.createScanner,skipTrivia: true) at base vs head —opportunity.object.ts: base tokens=605 head tokens=605 identical=true;sales-positions.ts: 266 = 266 identical=true. A line-based comment filter can be fooled by a comment-shaped code line; a token-stream identity cannot.
2 files,
+12/−4.skip-changesetapplied by additive POST and confirmed by read-back that the size-labeler did not strip it (size/s+skip-changesetboth present) — the right check, since that is exactly where a label gets silently eaten.⭐ The replacement text is true and useful, which is this card's actual standard
The ruling's reason this earns a card at all: the examples are copy-source for AI authors, so a comment naming an absent flow teaches a false shape. So I checked that the new comments do not repeat the card's own defect. Every reference resolves at head:
skills/objectstack-automation/SKILL.md✓examples/app-crm/src/flows/index.ts✓ (the one-flow-on-purpose pointer)examples/app-showcase/src/automation/flows/✓ — containsapprover-bindings.flow.ts,dynamic-approval.flow.ts,index.tsbind-position-sets.ts:33✓ —['finance_approver', 'crm_sales_user']approvalStatusField✓ — a real key, 15 occurrences inplugin-approvals/src/approval-service.ts
It does not merely delete the false claim; it names the real mechanism (
approvalStatusField), says why the field is readonly, states that the CRM keeps one flow on purpose, and points at where the worked example actually lives.The out-of-scope observation, and why not filing it was right
The dev found that no app under
examples/**setsapprovalStatusField— so the ADR-0019 status-mirror surface has no worked example anywhere in the example apps. It deliberately did not file that, because it is precisely resolution 2, which this ruling reserved for the maintainer as a separate feature card; filing it would re-litigate the ruling.⭐ Correct call, and the discipline worth naming: a dev that finds something real while fenced out of it should report it to the seat, not route around the fence by opening a card. I am carrying it rather than filing it, for the same reason.
Clause ② —
noComment-only, token-stream identical. No exported symbol, no payload key, no accept/reject change. No carrier owed and none hung (labels:
size/s,skip-changeset).Gates: 15 families re-derived from the real diff against merge base
2d40f9146,--repoassertion checked. 13 green (includingcheck-position-name-fold-loadersandcheck-system-context-census, the two most likely to notice this file pair). 2 NOT MEASURED by their own verdict text, neither a red:check-test-completenessexit 3 ("It is not a red") andcheck:dual-build-cjs-loadsexit 3 ("This is NOT a pass: nothing was measured"), the latter reading no file in this diff. Lint narrowed with all three evidence pieces. Package suite5 passed (5)files /45 passed (45); build andtsc --noEmitboth exit 0.Landing
⛔ Not landing yet. CI at
94d444d87is 20 success / 7 skipped / 7 still running, zero failures.
Generated by Claude Code
Observation-class finding, noticed while reading
examples/app-crmonorigin/mainas source material for a blog post. Not touched by that work; filing rather than fixing in an unrelated PR.What is stale
Two comments in the CRM example point at a "Discount Approval" flow:
examples/app-crm/src/objects/opportunity.object.ts, on theapproval_statusfield:examples/app-crm/src/security/sales-positions.ts, onFinanceApproverPosition:There is no such flow.
examples/app-crm/src/flows/containsconvert-lead.flow.tsonly, andflows/index.tsstates the scope explicitly:So the flow appears to have been deliberately moved to (or kept in) the showcase, and the two comments in the CRM were left behind.
grep -rn discount examples/app-crmreturns only these comments plus thediscount_percentfield and its translations.Why it is worth a ticket rather than nothing
The examples are training and reference material for agents as well as humans, and both comments assert a relationship that does not exist in the tree. The
FinanceApproverPositioncase is the sharper one: the position is declared and its only stated consumer is absent, so a reader has no way to tell whether the position is live scaffolding or dead weight — which is exactly the reading an AI author will have to make when it copies this app as a pattern.Possible resolutions, for triage
discount_percent, a readonlyapproval_statusmirror target, and afinance_approverposition described as "authorised to approve discounts above 30%" — every input except the flow itself.FinanceApproverPositionhas no remaining consumer, decide it under the enforce-or-remove discipline rather than leaving a declared-but-unreferenced position in an example.No severity judgement offered — filing plainly for the triage round.