Repository navigation
fix: stop the test suite from reading and rewriting the user's usage cache - #359
Merged
Merged
Conversation
…cache LocalUsageProvider uses LocalUsageCache.shared, whose default path is the user's real Application Support/PokeTokenBar/usage-cache.json with no gate. Any refresh() that runs the real providers during `swift test` (a store built with default providers, a detached Task that outlives its test) reads and rewrites that file. Measured with the app quit: one full suite run changed the real file's checksum and grew it from 965 KB to 1.02 MB; no other state file changed. Tests cannot catch this by injecting paths: injection covers the test that does it, not the process, so the default-path branch is invisible to them. The gate belongs in the store. AppEnv.persistsToUserLocation owns the rule (an injected path is always live; the default path only in the app bundle) so later stores reuse it instead of copying it. Reads are gated as well as writes, so a test cannot assert on fixtures mixed with the user's data. Verified by fault injection: removing the read gate loads 156 real blobs next to the single fixture; removing the write gate changes the real file's mtime. With the fix, a full run leaves every state file unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PNenigaFnwvcTs2kDND16Z
Owner
|
Thank you for your contribution! |
3 tasks done
cte13
added a commit
to cte13/PokeTokenBar-iOS
that referenced
this pull request
Sep 26, 2026
chore: sync fork with upstream (chattymin#357, chattymin#359, chattymin#360, chattymin#361)
7 of 10 tasks
donky-ey
added a commit
to donky-ey/PokeDexBar
that referenced
this pull request
Oct 16, 2026
…c and plan (SP4 batch A) (#20) * docs: add SP4 upstream port triage spec Classify all 191 non-merge commits in main..upstream/main (v2.5.6, 77f789d): 56 already in the fork, 87 not applicable, 26 port candidates grouped into batches A-H, 22 needing a user decision (new providers, small UX features, save backups). Also checks open upstream issues chattymin#337, chattymin#443 and chattymin#439 against the fork's code; all three apply. * docs: record the confirmed SP4 scope in the spec The user confirmed the triage recommendations: new providers, the pet size ceiling, variable menu-bar width, the Low Power GIF policy and save backups stay out; the right-click menu (chattymin#373) joins batch E; chattymin#337 and chattymin#443 are in, with chattymin#443 skipping claude-code mirror rows unconditionally. chattymin#216 is deferred. * docs: add the SP4 upstream port plan Eight batches (A-H), one PR each, executed A -> D -> G -> B -> C -> E -> F -> H. * fix: isolate every app-support store from test runs (upstream chattymin#359) `swift test` uses the same storage name as the installed app, so any code that reached a store's default path read or wrote the user's real Application Support folder. Upstream chattymin#359 measured usage-cache.json being rewritten by one suite run. A sweep found the same class in more places: the sprite cache (a test wrote fake GIFs into it), and the save, base-index.json and weather.json, which were safe only because every test happened to inject a path. Instead of gating each store, isolate at the root: for non-bundled runs AppEnv.supportDirectory() returns a per-process temporary folder, and every Application Support path now goes through it. The session-key store already did this on its own and now shares the root. AppEnv.userSupportDirectory names the real location for comparisons without creating it. Guards: AppStorageIsolationTests pins the root branch, each store's default path, and a source scan that forbids composing Application Support paths outside AppEnv. All three fail when the isolation branch is reverted. The upstream regression test is ported as testDefaultPathCacheNeverRewritesTheUserFile. The screenshot generator clones the installed app's sprite cache read-only into the isolated cache so its renders still have sprites. CLAUDE.md: the test-storage rule now covers all app storage, not only credentials. * test: pin the locale when asserting grouped numbers (upstream chattymin#166) TokenFormatter.grouped follows the system region, as macOS does, but the test hardcoded comma grouping, so it failed on machines whose region groups digits differently. grouped now takes a locale defaulting to .current (production unchanged); the test pins en_US and adds an es_ES case that fails if the argument is ignored. * test: guard UI sources against Hangul string literals (upstream chattymin#222) Interpolation tests only see strings that already go through L, so a literal that never reaches the table is invisible to them. Scan the UI sources (plus the app entry file, which holds UI copy in this fork) and fail on any Hangul string literal; Korean comments stay allowed. Core and Player are left out because their per-language data tables hold Hangul legitimately. Only the test is ported; the Antigravity notice it shipped with does not exist here. * test: check the bundled storage namespace now that test runs are isolated testEveryStoredPathDerivesFromTheNamespace asserted that the support folder ends in the storage name, which held only because swift test used the real folder. Check the bundled path with the pure supportDirectoryURL(isBundledApp:) and keep the save-under-the-root check for the running process. * test: stop the seal test from failing on random ciphertext testPayloadIsNotReadableInTheFile searched the envelope for "42". The seal uses a fresh AES-GCM nonce each time, so those two characters show up in the base64 by chance in roughly 1.5% of runs, which failed the gate intermittently. Assert that the plaintext payload bytes are absent instead. * fix: tighten the storage-isolation guards and correct their description (review) - Put @discardableResult back on writeFile. The new tests had been inserted between the attribute and the helper, which added 41 unused-result warnings. - The source scan now also forbids AppEnv.userSupportDirectory outside AppEnv.swift. A future store could otherwise reach the real folder through the new accessor and pass every guard. - Comment stripping in the scan walks characters (shared SourceScan, also used by LocalizedUILiteralTests), so a "//" inside a string no longer hides the rest of the line. - The isolated folder is named with a UUID instead of the PID. PIDs are reused, so a later run could inherit an earlier run's cache. - Correct the claim in 6b52c9e and CLAUDE.md that all three isolation tests fail when the root branch is reverted. Mutation checks show: reverting the root fails the root-branch and per-store tests (plus the session-key test); the source scan does not react to that, and instead fails when a store composes the path itself (the old LocalUsageCache.defaultFileURL, restored) or uses userSupportDirectory. --------- Co-authored-by: 이동기 <ldg0831@midasin.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
swift testreads and rewrites the contributor's real~/Library/Application Support/PokeTokenBar/usage-cache.json.LocalUsageProviderusesLocalUsageCache.shared, whose default path is that file, with no gate. Anyrefresh()that runs the real providers during the suite reaches it. That includes aUsageStorebuilt with default providers, and a detached Task that outlives its test. I measured it: with the app quit, one fullswift testrun onmainchanged the real file's checksum and grew it from 965 KB to 1.02 MB. No other state file in that folder changed (save file, session keys, Cursor cache), so this PR gates only the usage cache.Tests can't catch this by injecting a path. Injection only covers the test that does it, not the whole process. Every existing
LocalUsageCachetest injects afileURL, so the default-path branch is invisible to them.The fix puts the gate in the store:
AppEnv.persistsToUserLocation(injectedFileURL:)holds the rule in one place. An injected path is always live, and the default path is live only in the app bundle. Later stores can reuse it instead of copying it.LocalUsageCacheskips both the read and the write when the rule says no. Reads are gated too, so a test can't assert on fixtures mixed with the user's data. Parsing and totals work as before; only disk IO is skipped.Trade-off
A raw
swift buildbinary (.build/debug/PokeTokenBar) no longer saves its usage cache between launches, so it rescans logs on each start. Bundles fromscripts/build-app.share unaffected.AppEnv.isBundledAppalready gates notifications, Keychain reads, and log writes the same way.Type of change
Checklist
swift buildandswift testpass locally (1357 tests). One failure,SessionKeySettingsRenderingTests.testSessionKeyEntryOpensInsideViewportAndPreservesDifficultyControls, fails the same way on currentmainon my machine and is unrelated (it still fails with an isolatedPTB_STATE_DIR).testDefaultPathCacheNeitherReadsNorWritesUserFile: the cache stays inert on the default path, with a blob count of 1 and the real file's mtime unchanged.testInjectedPathCacheStillPersists: the opposite direction.UserLocationGateTests: the rule itself.🤖 Generated with Claude Code
https://claude.ai/code/session_01PNenigaFnwvcTs2kDND16Z