Skip the consent drift cases that read files this export omits - #1469
aniruddhaadak80 wants to merge 1 commit into
Conversation
… omits The `every surface asks the same question` block in `common/src/ads/sponsored-consent.test.ts` reads two files that live in the Desktop Electron app: - `freebuff-desktop/electron/mcp-consent-bridge.cjs` - `freebuff-desktop/electron/consent-window.html` `freebuff-desktop/` is not one of the in-scope paths in CONTRIBUTING.md and the public export does not ship it, so the directory does not exist in this tree and all 8 cases in that block die with ENOENT before asserting anything. The block exists to stop Desktop and the CLI asking the user two different questions, so in this mirror the guard is not merely failing, it is inert: it can never detect drift, and it fails loudly enough that anyone running the suite learns to ignore this file. Public CI never runs `bun test` (it installs, builds the SDK, builds the binary and smoke-tests the binary), which is why a permanently red file has gone unnoticed on main. Guard each case on the presence of the file it reads, using the same `isInThisTree` + `test.skipIf` shape already used for the planner and desktop sources in `common/src/__tests__/free-agents.test.ts`. The check keeps running in the private tree, where those sources do exist, and reactivates here by itself if the export ever starts including them. The one case that needs no external file, `the Windows sentence says the three things, in plain words`, still runs here. Fixes CodebuffAI#1467
|
Closing this one: upstream I confirmed this rather than assuming:
Re-adding a deleted test file is a product decision, not a bug fix, so I am not going to smuggle it back in under a "fix the ENOENT" title. Leaving the diagnosis in case it is useful: The block The same defect class in files that still exist is fixed by #1458 and #1459 (both still apply cleanly to current |
Summary
common/src/ads/sponsored-consent.test.tsis permanently red onmain. Eight cases in itsevery surface asks the same questionblock read two files in the Desktop Electron app, and this repository does not ship that tree, so every one of them dies withENOENTbefore it asserts anything.This skips exactly those eight cases, on the presence of the file each one reads.
Fixes #1467
Problem
The block reads:
freebuff-desktop/electron/mcp-consent-bridge.cjsfreebuff-desktop/electron/consent-window.htmlfreebuff-desktop/is not one of the in-scope paths inCONTRIBUTING.md(cli/,sdk/,common/,agents/,packages/*,freebuff/excluding the private web app,scripts/tmux/, public docs), and this repository is an export of the private source tree. The directory simply does not exist here.The comment above the block states its purpose: the two Desktop surfaces carry their own copies of these strings, and the block "is what stops the two surfaces drifting into asking two different questions."
In the public mirror the guard is therefore not just failing, it is inert — it can never detect drift, and it fails loudly enough that anyone running the suite learns to ignore this file.
Repro on
main(0cd284b)$ bun test common/src/ads/sponsored-consent.test.tsThis is not a platform artifact: the path is absent from the export on every platform, so it fails identically on Linux and macOS.
Why CI never caught it
.github/workflows/ci.ymlinstalls dependencies, builds the SDK, builds the Freebuff binary and smoke-tests the binary. It never runsbun test. A permanently red test file therefore produces a green check on every PR.Root cause
The export strips
freebuff-desktop/, but the tests that read it were exported with it. There is no presence check between the two, so a file that cannot exist by construction is asserted on unconditionally.Changes
common/src/ads/sponsored-consent.test.tsonly:existsSyncalongsidereadFileSync.isInThisTree(rel), documented with why the guard exists.BRIDGE/WINDOWinstead of repeating the literals in six places.test.each/test(...)blocks that read those files totest.skipIf(!isInThisTree(rel)), iterating[BRIDGE, WINDOW]where both surfaces are checked.This is the same
isInThisTree+test.skipIfshape already used for the planner and desktop sources incommon/src/__tests__/free-agents.test.ts, so the repo has one idiom for this rather than two.The check is not disabled. It keeps running in the private tree, where the sources do exist, and reactivates here by itself if the export ever starts including them.
the Windows sentence says the three things, in plain wordsneeds no external file and still runs here unchanged.Testing
Whole
common/src/adssuite, 36 files, same command on both trees:$ bun test $(git ls-files 'common/src/ads/*.test.ts')main(0cd284b)Same test count. The 8 ENOENT failures are gone. The 1 remaining failure is pre-existing on
mainand untouched by this diff:The single file, before and after:
Regression proof: the guard is not a blanket mute
I created
freebuff-desktop/electron/consent-window.htmlwith deliberately wrong content and re-ran. The fourconsent-window.htmlcases ran and failed on the content, while the still-absentmcp-consent-bridge.cjscases stayed skipped:So when the source is present, the drift check still fails on real drift. The scratch file was removed afterwards; the branch diff is the test file only.
Formatting
bunx prettier --checkreports this file both before and after the change. I confirmed the committed blob is byte-identical to Prettier's own output (0 differing lines) and contains no CR bytes, so the warning is the localcore.autocrlf=trueWindows checkout, not the patch. The committed blob is LF-only, as onmain. No reformat churn is included.Compatibility / risk
None. Test-only, no production code path is touched. The eight cases remain registered and will run automatically wherever the sources exist. Test names are preserved (the
test.eachrows become template-literal names carrying the same relative path), so nothing downstream that filters on test name loses a case.Notes for the maintainer
This is the third file in the tree with the same shape, after
common/src/__tests__/free-agents.test.tsandcommon/src/__tests__/freebuff-public-data-use-copy.test.ts. The underlying gap is thatci.ymlnever runsbun test, so an export-stripped test file is indistinguishable from a passing one. Abun teststep over the packages CI already installs would surface the whole class at once; I kept this PR to the one file so it stays reviewable, but happy to follow up with that separately if useful.