chore(core): runtime.json sidecar follow-ups (round 2)#4084
Closed
BZ-D wants to merge 1 commit into
Closed
Conversation
Pick up three of the items left as out-of-scope in #4030, all from the original #3714 review. No runtime behavior change. - Sanitize sessionId in Storage.getRuntimeStatusPath. --resume <id> and Config.startNewSession(id) accept user-supplied strings, so values like '../foo' or 'a/b' would resolve outside chats/ via path.join. Reject anything that isn't [A-Za-z0-9._-]{1..128}, plus reject '..' substrings, leading dot, and empty. The two on-path callers (gemini.tsx startup, Config.startNewSession's IIFE) already swallow exceptions, so a throw is safe. - Add Config.getRuntimeStatusPath() pass-through. gemini.tsx was reaching into config.storage directly, which leaks the Storage internal across the package boundary. Route the call through Config so a future Storage refactor surfaces at compile time. - Clarify the module-header forward-compat wording in runtimeStatus.ts. The original 'treat unknown fields as forward-compatible additions' read as if schema_version itself were forward-compatible, which contradicts the strict-equality check in readRuntimeStatus. Reworded to scope forward-compat to unknown fields *within* a schema_version, and to state that a bump is a breaking change. Tests: extend runtimeStatus.config.test.ts with 8 sanitization cases (empty, '..', '/', '\\', leading '.', NUL, whitespace, plus a positive randomUUID-shape case). 32/32 pass in runtimeStatus*.test.ts; 132/132 in config.test.ts; 213/213 combined. Out of scope (still): rename-with-retry de-duplication against atomicFileWrite.ts (cross-module refactor, separate PR), and the fire-and-forget IIFE serialization under rapid /clear (final state is correct, PID liveness check covers the transient window).
5 tasks
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
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
1 participant
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
Picks up three of the items left as Out of scope in #4030, all from the original #3714 review. No runtime behavior change.
sessionIdinStorage.getRuntimeStatusPath(was OpenAI API Error: 401 Incorecct API Key provided #6 of the original 9 review items —storage.ts:241).--resume <id>andConfig.startNewSession(id)accept user-supplied strings, so values like../fooora/bwould resolve outsidechats/viapath.join. Reject anything that isn't[A-Za-z0-9._-]{1..128}, plus reject..substrings, leading dot, NUL, whitespace, and empty. The two on-path callers (gemini.tsxstartup,Config.startNewSession's IIFE) already swallow exceptions, so a throw is safe.Config.getRuntimeStatusPath()pass-through (was the encapsulation note ongemini.tsx:185).gemini.tsxwas reaching intoconfig.storagedirectly, which leaks theStorageinternal across the package boundary. Route throughConfigso a futureStoragerefactor surfaces at compile time.runtimeStatus.ts:27). The original “treat unknown fields as forward-compatible additions” read as ifschema_versionitself were forward-compatible, which contradicts the strict-equality check inreadRuntimeStatus. Reworded to scope forward-compat to unknown fields within aschema_version, and to state that a bump is a breaking change.Tests
Eight new cases in
Storage.getRuntimeStatusPathcover the rejection set (empty,../foo,a/../b,foo/bar,foo\\bar,.hidden, NUL, whitespace), plus one positive case for therandomUUIDshape.config.test.ts132/132 still pass. Workspacetypecheckis clean for the touched packages (the two pre-existing errors inrenameCommand.ts/summaryCommand.tsare unrelated and reproduce on cleanmain).Test plan
runtimeStatus.test.ts— 19/19 unchangedruntimeStatus.config.test.ts— 13/13 (added 9)config.test.ts— 132/132 unchangedcoretypecheck cleanclitypecheck clean forgemini.tsxOut of scope (still)
Two items from #3714 review remain unhandled and are deliberately not in this PR:
renameWithRetryde-duplication againstatomicFileWrite.ts. Cross-module refactor — better as its own PR where the consolidation can be reviewed independently of the sidecar code path.Config.startNewSession. Final on-disk state is correct after any rapid/clearburst (the lastrenamewins and external consumers PID-liveness-check anyway). Adding a promise-chain mutex would buy nothing observable.