From 9335f3d08c6b42f0d29f027e7d614d114b74589c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 13:33:09 +0000 Subject: [PATCH] fix(core,fields,plugin-dashboard): clear the last four raw control bytes and empty the baseline (objectstack#5450) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four source files still carried a raw control character in a string literal, all four listed in `KNOWN_OFFENDERS` when the gate landed in objectui#3388: - `packages/core/src/evaluator/listConditional.ts` — U+0000, the composite dedup key of the one-time evaluation-failure warning. - `packages/core/src/utils/record-title.ts` — U+0000, the `EMPTY_TOKEN` sentinel marking an empty titleFormat placeholder. - `packages/fields/src/widgets/PeoplePicker.tsx` — U+0000, the separator of the record-id signature the keyboard cursor is keyed on. - `packages/plugin-dashboard/src/DatasetWidget.tsx` — U+0001, the separator joining several row dimensions into a pivot row id. The first three were invisible to content search: grep and ripgrep classified the whole file as binary and printed no line at all, which is why grepping this repo for `EMPTY_TOKEN` or `recordsSignature` returned nothing. The fourth is a different harm — U+0001 never triggers binary classification — but written raw it is invisible in every editor and every diff, so no reviewer could tell what the separator actually was. Two shapes of fix, chosen per site rather than in bulk: - `listConditional.ts` no longer needs a separator at all. The key is now `JSON.stringify([label ?? '', source])`, so the label/source boundary is unambiguous for any input instead of resting on "this character cannot occur" — the same move objectui#3388 made for the include key. - The other three keep a byte-identical runtime value and are re-spelled as escapes. Each has a reason it must not become a printable character: `EMPTY_TOKEN` is interpolated verbatim into two RegExps and would strip real text out of titles; the PeoplePicker signature already spelled the very same separator as an escape twelve lines below; the pivot row id joins arbitrary dimension values and is a component of the cell-lookup key. All four `KNOWN_OFFENDERS` entries are deleted in this commit, as the map's contract requires — `scan()` reports a stale entry as loudly as a new offender, so the fix and the baseline cannot drift apart. The map is now empty. No changeset: nothing here is user-visible. Three changes are byte-identical at runtime and the fourth is an in-memory dedup key for a console warning. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GTRjn8xBqp75dk7kFupVRt --- .../__tests__/listConditional.test.ts | 24 ++++++++++ .../core/src/evaluator/listConditional.ts | Bin 12812 -> 13219 bytes .../src/utils/__tests__/record-title.test.ts | 43 ++++++++++++++++++ packages/core/src/utils/record-title.ts | Bin 18666 -> 19167 bytes packages/fields/src/widgets/PeoplePicker.tsx | 8 +++- .../plugin-dashboard/src/DatasetWidget.tsx | 8 +++- .../src/__tests__/DatasetWidget.test.tsx | 31 +++++++++++++ scripts/__tests__/check-control-bytes.test.ts | 43 ++++++++++++++++++ scripts/check-control-bytes.mjs | 9 ++-- 9 files changed, 160 insertions(+), 6 deletions(-) diff --git a/packages/core/src/evaluator/__tests__/listConditional.test.ts b/packages/core/src/evaluator/__tests__/listConditional.test.ts index 6a86d22b94..d9c6aedfc8 100644 --- a/packages/core/src/evaluator/__tests__/listConditional.test.ts +++ b/packages/core/src/evaluator/__tests__/listConditional.test.ts @@ -117,6 +117,30 @@ describe('evalRowPredicate', () => { expect(r).toBe(false); expect(warn).not.toHaveBeenCalled(); }); + + // The warning is deduped per (label, source) pair. These two pin the two + // halves of that contract across the objectstack#5450 key change: the key + // used to be the pair joined on a raw U+0000 byte (which is what made this + // file invisible to grep) and is now JSON.stringify of the pair. + it('warns only once for the same (label, source) pair', () => { + const faulty = 'record.dedupe_once =='; + evalRowPredicate(faulty, { a: 1 }, { warnOnError: true, label: 'twice' }); + evalRowPredicate(faulty, { a: 2 }, { warnOnError: true, label: 'twice' }); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it('keeps the label/source boundary unambiguous', () => { + // The obvious repair for an "impossible character" separator is to pick a + // printable one, which merely relocates the collision. These two pairs + // share a label+source CONCATENATION and would collapse into one key under + // a space separator, so the second warning would be swallowed. JSON needs + // no impossible character and no such assumption. + evalRowPredicate('beta record.boundary ==', { a: 1 }, { warnOnError: true, label: 'alpha' }); + evalRowPredicate('record.boundary ==', { a: 1 }, { warnOnError: true, label: 'alpha beta' }); + expect(warn).toHaveBeenCalledTimes(2); + expect(String(warn.mock.calls[0][0])).toContain('(alpha)'); + expect(String(warn.mock.calls[1][0])).toContain('(alpha beta)'); + }); }); }); diff --git a/packages/core/src/evaluator/listConditional.ts b/packages/core/src/evaluator/listConditional.ts index ae8ec04d2e5fdf44ab5442e35c2a239571a6c1d1..675a91c1ecd0642a7c788d2bc8dc3664e652b096 100644 GIT binary patch delta 463 zcmX|;J#HH@5Jvj|&J6;5kP1t%B)hQ6z(`Wrg;fZy3>UNHNIUXsxFBb<*h)D-%G|&g zFyI^c0s$ f4u%)WVs46Ty2gF28b&Mz%WPTjnP H|GhE*@!=7C diff --git a/packages/core/src/utils/__tests__/record-title.test.ts b/packages/core/src/utils/__tests__/record-title.test.ts index 86e9210b19..871eec473d 100644 --- a/packages/core/src/utils/__tests__/record-title.test.ts +++ b/packages/core/src/utils/__tests__/record-title.test.ts @@ -384,3 +384,46 @@ describe('formatTitleTemplate', () => { expect(formatTitleTemplate(undefined, { id: '1' })).toBe(''); }); }); + +/** + * objectstack#5450. The empty-placeholder sentinel `EMPTY_TOKEN` is a U+0000 + * that used to be written into the source as the raw byte, which made this + * module binary to grep. Re-spelling it as an escape leaves the runtime value + * identical — these pin the behaviour that identity is responsible for, so a + * later attempt to "make it printable" cannot pass quietly. + * + * Byte discipline: the assertions test CODE POINTS via charCodeAt. No control + * character is written into this file, as a literal or as an escape. + */ +describe('formatTitleTemplate — the empty-placeholder sentinel', () => { + /** True when every code point is printable (no C0 control, no U+007F). */ + const isPrintable = (s: string) => + Array.from(s).every((c) => { + const cp = c.codePointAt(0) as number; + return cp >= 0x20 && cp !== 0x7f; + }); + + it('never leaks the sentinel into the rendered title', () => { + const out = formatTitleTemplate('{full_name} - {company}', { company: 'Acme' }); + expect(out).toBe('Acme'); + expect(isPrintable(out), 'the sentinel must not survive into a rendered title').toBe(true); + }); + + it('strips several empty placeholders and their orphan separators at once', () => { + const out = formatTitleTemplate('{a} - {b} | {c}', { b: 'Only' }); + expect(out).toBe('Only'); + expect(isPrintable(out)).toBe(true); + }); + + it('leaves a separator that sits between two RESOLVED fields alone', () => { + // The strip passes are anchored on the sentinel, so a real separator with + // content on both sides must survive — otherwise "A - B" would become "AB". + expect(formatTitleTemplate('{a} - {b}', { a: 'A', b: 'B' })).toBe('A - B'); + }); + + it('does not swallow a record value just because it neighbours an empty one', () => { + const out = formatTitleTemplate('{missing}·{kept}', { kept: 'Kept' }); + expect(out).toBe('Kept'); + expect(isPrintable(out)).toBe(true); + }); +}); diff --git a/packages/core/src/utils/record-title.ts b/packages/core/src/utils/record-title.ts index aa8fcd4718e5f25b177d444b885cac6caf4813eb..72d15c3878527ae9af6bcdb1b6b0ab76587db52d 100644 GIT binary patch delta 537 zcmXYuF>ez=5QXIiIr$YM(SR_q3(~lvh$RXT2#ipqNMvqr>|4CvUCqwLabW?(UdMoS2Z7$G;v>SNazQDEdo|ouWf{ ZS-g6)y!`m>$N1#IRbMaZ_s{Xg^M6YVy1D=W delta 20 ccmcaVmGRX?#tjoVCO2@2GBRwQ&E?_%09nNcNdN!< diff --git a/packages/fields/src/widgets/PeoplePicker.tsx b/packages/fields/src/widgets/PeoplePicker.tsx index 76fbd4e209..04290ad252 100644 --- a/packages/fields/src/widgets/PeoplePicker.tsx +++ b/packages/fields/src/widgets/PeoplePicker.tsx @@ -316,8 +316,14 @@ export function PeoplePicker({ // returns the same records (StrictMode double-effect, refetch-on-focus) // re-emits a new array; resetting on identity alone yanked the cursor to -1 // mid-navigation (flaky ArrowDown→Enter, and a real UX nit). + // + // The id separator is the escaped U+0000 spelling, not the byte itself + // (objectstack#5450): one raw NUL made this whole file binary to grep and + // ripgrep — a content search printed `binary file matches` and no line. The + // runtime value is unchanged, and it is the same separator the `cursorEpoch` + // key below already spelled this way. const recordsSignature = useMemo( - () => query.records.map(r => String(getPersonId(r, idField))).join(''), + () => query.records.map(r => String(getPersonId(r, idField))).join('\u0000'), [query.records, idField], ); // The reset runs in the RENDER PHASE (the "adjusting state during render" diff --git a/packages/plugin-dashboard/src/DatasetWidget.tsx b/packages/plugin-dashboard/src/DatasetWidget.tsx index adf6e1c889..e1bdbdf2a0 100644 --- a/packages/plugin-dashboard/src/DatasetWidget.tsx +++ b/packages/plugin-dashboard/src/DatasetWidget.tsx @@ -86,7 +86,13 @@ export function buildPivot( const colSeen = new Set(); const cellIndex = new Map(); rows.forEach((row, index) => { - const rid = rowDims.map((d) => String(row[d] ?? '∅')).join(''); + // The row-dimension separator below is the escaped U+0001 spelling, not the + // byte itself (objectstack#5450). U+0001 does not blind grep the way U+0000 + // does, but written raw it is invisible in every editor and every diff, so no + // reviewer could tell what this separator actually was. The runtime value is + // unchanged: a character no dimension value can carry, which is what keeps + // two dimension values from merging into one ambiguous row id. + const rid = rowDims.map((d) => String(row[d] ?? '∅')).join('\u0001'); const cid = String(row[colDim] ?? '∅'); if (!rowSeen.has(rid)) { rowSeen.add(rid); rowHeaders.push({ id: rid, labels: rowDims.map((d) => formatDimensionValue(row[d])) }); } if (!colSeen.has(cid)) { colSeen.add(cid); colHeaders.push({ id: cid, label: formatDimensionValue(row[colDim]) }); } diff --git a/packages/plugin-dashboard/src/__tests__/DatasetWidget.test.tsx b/packages/plugin-dashboard/src/__tests__/DatasetWidget.test.tsx index e9c65d2531..5630055655 100644 --- a/packages/plugin-dashboard/src/__tests__/DatasetWidget.test.tsx +++ b/packages/plugin-dashboard/src/__tests__/DatasetWidget.test.tsx @@ -319,6 +319,37 @@ describe('DatasetWidget', () => { expect(p.cellIndex.get('Done Low')).toBeUndefined(); // sparse combo absent }); + // objectstack#5450. Every case above pivots on ONE row dimension, so the + // separator that joins several of them was never exercised — it could have + // been anything, including the empty string, and the suite stayed green. It + // was a raw U+0001 in the source (invisible in every editor and diff) and is + // now the same character written as an escape; these pin what that character + // is responsible for. + // + // Byte discipline: the expected id is built from a NUMBER via fromCharCode. + // No control character is written into this file, as a literal or an escape. + it('buildPivot keeps two row dimensions apart that would merge without a separator', () => { + const rows = [ + // "x" + "yz" and "xy" + "z" concatenate to the same string, so an empty + // (or absent) separator collapses these into ONE row header and the second + // row's cell overwrites the first. + { region: 'x', segment: 'yz', priority: 'High', total: 1 }, + { region: 'xy', segment: 'z', priority: 'High', total: 2 }, + ]; + const p = buildPivot(rows, ['region', 'segment'], 'priority'); + + expect(p.rowHeaders).toHaveLength(2); + expect(p.rowHeaders.map((r) => r.labels)).toEqual([ + ['x', 'yz'], + ['xy', 'z'], + ]); + + const SEP = String.fromCharCode(0x01); + expect(p.rowHeaders.map((r) => r.id)).toEqual([`x${SEP}yz`, `xy${SEP}z`]); + expect(p.cellIndex.get(`x${SEP}yz High`)).toBe(0); + expect(p.cellIndex.get(`xy${SEP}z High`)).toBe(1); + }); + it('renders a pivot (≥2 dims) as a true cross-tab, not a flat table', async () => { const src = { queryDataset: vi.fn(async () => ({ rows: [ diff --git a/scripts/__tests__/check-control-bytes.test.ts b/scripts/__tests__/check-control-bytes.test.ts index 24c307df77..2ecc14e9db 100644 --- a/scripts/__tests__/check-control-bytes.test.ts +++ b/scripts/__tests__/check-control-bytes.test.ts @@ -303,6 +303,49 @@ describe('objectstack#5425 — the file that started this is readable again', () }); }); +/** + * objectstack#5450 — the four files the baseline shipped with are now clean, and + * KNOWN_OFFENDERS is empty. + * + * Two harms, so two assertion shapes. Three of the four carried U+0000 and were + * genuinely invisible to content search, so the pin for those is a real grep + * that must print a line. The fourth carried U+0001, which (measured on GNU grep + * 3.11 and ripgrep 14) never blinded grep at all — claiming a search outage + * there would be a fabricated pin, so it is asserted only on the byte's absence, + * which is the harm it actually had: an unreviewable literal. + */ +describe('objectstack#5450 — the four baselined files are clean', () => { + const cleaned = [ + { file: 'packages/core/src/evaluator/listConditional.ts', grepFor: 'warnedError' }, + { file: 'packages/core/src/utils/record-title.ts', grepFor: 'EMPTY_TOKEN' }, + { file: 'packages/fields/src/widgets/PeoplePicker.tsx', grepFor: 'recordsSignature' }, + // U+0001, not U+0000: never a search outage, so no grep assertion below. + { file: 'packages/plugin-dashboard/src/DatasetWidget.tsx', grepFor: null }, + ]; + + it.each(cleaned.map((c) => c.file))('%s carries no control byte at all', (file) => { + const buf = fs.readFileSync(path.join(repoRoot, file)); + const found = [...buf].filter((b) => SCANNED_BYTES.has(b)); + expect(found).toEqual([]); + }); + + it.each(cleaned.filter((c) => c.grepFor).map((c) => [c.file, c.grepFor as string]))( + '%s is visible to a content search again', + (file, needle) => { + const out = execFileSync('grep', ['-n', needle, file], { cwd: repoRoot, encoding: 'utf8' }); + expect(out).toMatch(new RegExp(needle)); + expect(out).not.toMatch(/binary file matches/); + }, + ); + + it('has no baseline entry left for any of them', () => { + const baseline = KNOWN_OFFENDERS as Map; + for (const { file } of cleaned) { + expect(baseline.has(file), `${file} is clean; its KNOWN_OFFENDERS entry must be gone`).toBe(false); + } + }); +}); + describe('wiring — the gate is actually reachable and actually runs', () => { const pkg = JSON.parse(fs.readFileSync(path.join(repoRoot, 'package.json'), 'utf8')); const workflowPath = path.join(repoRoot, '.github/workflows/control-bytes.yml'); diff --git a/scripts/check-control-bytes.mjs b/scripts/check-control-bytes.mjs index 3a874e1008..0534bf32cc 100644 --- a/scripts/check-control-bytes.mjs +++ b/scripts/check-control-bytes.mjs @@ -148,10 +148,11 @@ const WIDE_BOMS = [ * The map is expected to reach empty and stay there. */ export const KNOWN_OFFENDERS = new Map([ - ['packages/core/src/evaluator/listConditional.ts', { bytes: [NUL], issue: 'objectstack#5450' }], - ['packages/core/src/utils/record-title.ts', { bytes: [NUL], issue: 'objectstack#5450' }], - ['packages/fields/src/widgets/PeoplePicker.tsx', { bytes: [NUL], issue: 'objectstack#5450' }], - ['packages/plugin-dashboard/src/DatasetWidget.tsx', { bytes: [0x01], issue: 'objectstack#5450' }], + // Empty, and expected to stay that way. The four entries this map shipped + // with (two in `@object-ui/core`, one in `@object-ui/fields`, one in + // `@object-ui/plugin-dashboard`) were cleaned by objectstack#5450 and deleted + // in the same PR, because `scan()` reports a stale entry as loudly as a new + // offender. Add one back only with an issue number, and only as debt. ]); function hasWideBom(buf) {