chore(core): runtime.json sidecar follow-ups from #3714 review#4030
Merged
Conversation
- Drop dead `typeof !== 'boolean'` check in the integer-type guard (Number.isInteger(true) is already false; rename to isFiniteInteger). - Drop dead utf-8-message branch in readRuntimeStatus's catch: fs.readFile(p, 'utf-8') returns replacement chars rather than throwing, so that branch never fires; the JSON.parse fallback already handles the truncated-UTF-8 case the test exercises. - Move runtimeStatus.js export from between fileUtils and filesearch into alphabetical position next to runtimeFetchOptions. - Add Config.startNewSession integration tests pinning the runtimeStatusEnabled gate: flag-off leaves sibling sidecars alone, flag-on swaps old->new on the same PID, no-op when sessionId is unchanged. No behavior change for the runtime-status sidecar itself; this just trims dead defensive code and adds the integration coverage that was missing in #3714.
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. |
wenshao
commented
May 10, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: self-PR. — deepseek-v4-pro via Qwen Code /review
tanzhenxin
approved these changes
May 11, 2026
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
Clean cleanup follow-up from the #3714 review. Both dead-code removals are individually verifiable: Number.isInteger(true) is spec-required to be false, and fs.readFile(p, 'utf-8') substitutes U+FFFD rather than throwing on invalid UTF-8. The isInteger → isFiniteInteger rename is module-private with no stale references, and the four new integration tests cover the right Config.startNewSession seams — flag-off, flag-on, and same-id no-op.
Verdict
APPROVE — verifiable no-op refactors with meaningful test backfill.
wenshao
commented
May 11, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: self-PR. — glm-5.1 via Qwen Code /review
5 tasks
Collaborator
|
Follow-up on the three items left as Out of scope here: #4084.
Still deliberately out of scope: |
wenshao
added a commit
that referenced
this pull request
May 17, 2026
) - Drop dead `typeof !== 'boolean'` check in the integer-type guard (Number.isInteger(true) is already false; rename to isFiniteInteger). - Drop dead utf-8-message branch in readRuntimeStatus's catch: fs.readFile(p, 'utf-8') returns replacement chars rather than throwing, so that branch never fires; the JSON.parse fallback already handles the truncated-UTF-8 case the test exercises. - Move runtimeStatus.js export from between fileUtils and filesearch into alphabetical position next to runtimeFetchOptions. - Add Config.startNewSession integration tests pinning the runtimeStatusEnabled gate: flag-off leaves sibling sidecars alone, flag-on swaps old->new on the same PID, no-op when sessionId is unchanged. No behavior change for the runtime-status sidecar itself; this just trims dead defensive code and adds the integration coverage that was missing in #3714.
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
3 participants
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
Cleanup follow-ups from my post-merge review of #3714. Pure trim + test backfill — no behavior change.
typeof !== 'boolean'clause in the integer-type guard.Number.isInteger(true) === false, so the extra check never fires. Renamed toisFiniteIntegerto match what it actually does.err.message.includes('utf-8')branch inreadRuntimeStatus. Node'sfs.readFile(p, 'utf-8')returns replacement characters rather than throwing on invalid UTF-8, so that arm never executes; the existingJSON.parsefallback already covers the truncated-UTF-8 case the unit test exercises (returns null on invalid UTF-8 bytesstill passes).runtimeStatus.jsre-export inpackages/core/src/index.tsfrom betweenfileUtilsandfilesearchinto alphabetical position next toruntimeFetchOptions.Config.startNewSessionintegration tests (runtimeStatus.config.test.ts) covering theruntimeStatusEnabledownership gate, which the unit-level tests inruntimeStatus.test.tscouldn't reach:/clearscenario)started_atpreserved, no stray files inchats/Tests
npm run typecheckandnpm run buildclean.Test plan
runtimeStatus.test.ts— 19/19 still pass after the dead-code removalruntimeStatus.config.test.ts— new 4 cases passconfig.test.ts— 198 pre-existing tests still passtypecheckcleanbuildcleanOut of scope
Three other items from the review are intentionally not in this PR:
schema_versioncheck is consistent with that. No change needed.sessionIdinStorage.getRuntimeStatusPath:sessionIdcomes fromrandomUUID()or the resume path (loaded from disk we wrote ourselves). Defense-in-depth only; can do separately if wanted./clearbursts: worst case is residual on-disk files in transient states; the final state is still correct and external consumers PID-liveness-check anyway. Cost/benefit doesn't justify a mutex here.