fix(setup): make session_start diagnostics actionable - #633
Conversation
tt-a1i
left a comment
There was a problem hiding this comment.
[P2] Bound and sanitize diagnostic paths before putting them in startup notifications (extensions/setup/index.ts)
inspectDocument() builds each diagnostic path from keys in the on-disk JSON, and this change joins every path into a UI notification with no count or length limit. A valid configuration containing many unknown keys, or one very long key, can flood every session_start notification; a key containing control characters is also passed through verbatim. Keep the useful diagnostic category and location, but cap the number and length of displayed paths and sanitize terminal text. Please add a regression using a long or control-bearing unknown key. The same concern applies to the overlapping #632 candidate, so these should be resolved together rather than merged separately.
Reviewed head e81644b against current main ad68e44. The existing CI is green; this path is not covered by the current tests.
727d220 to
074189a
Compare
074189a to
04d5dc8
Compare
Problem
On
session_start, a valid pre-configVersionfile with only recognized fields should stay quiet, while actual configuration diagnostics should retain their category and location. Review found that unknown keys from the on-disk JSON can produce an unbounded number of paths, arbitrarily long paths, and terminal control characters in every startup notification. Fixes #631.Value
Startup notifications stay concise and readable for malformed or forward-compatible configuration files, while still pointing users to the affected location. The full diagnostic report remains available through
/openpi-setup.Approach
session_startdoes not write the file.Validation
node --experimental-strip-types --test --test-concurrency=1 --test-name-pattern="session.?start|session start" tests/extensions/setup/index.test.ts— 8/8 passed, including the new long/control-bearing-key regression.bun run check— passed (contracts, web build/typecheck, formatting, lint, and root typecheck).bun run test— 1,835 passed, 5 failed, 10 skipped. Failures:tests/extensions/git-info/process.test.tsexpected exit code 7 but received -1; one detached-render workflow timed out; threetests/web/transcript-search.test.tsassertions failed, including two WindowsEPERMsymlink creations. All setup tests passed.bun test --max-concurrency=1 tests/extensions/setup/index.test.ts— 36 passed, 2 unrelated failures (post-edittimed out and an invalid-footer-style assertion failed); both passed through the repository's Node-basedbun run testpath.git diff --check origin/main...HEAD— passed.Impact
Maintainer integration validation (2026-10-04)
Final head
933912c8. Independent comparison recommends this implementation over #632: legacy-only configurations remain silent, real diagnostics preserve category/path, and paths have count/length/control-character bounds. Fixed only obsolete/flaky test assumptions: dynamic spinner frames, asynchronous rename-dialog closure, macOS long temporary paths. Setup + child page 61/61 and sidebar UI 19/19 passed; final-headbun run checkpassed in 7.93 s.Final follow-up head
4c15d3427f523e3ca8e57256dabe8c1006e236b4. Combined localbun run checkpassed (13.87 s);bun run testpassed: 2,262 Node + 1,156 UI tests, 9 skips, 328.74 s at combined commit53a93e6d. A mistakenly lingering validation process overlapped part of this run; this wall time is not a speed benchmark. Final test-only follow-ups have 19/19 focused page/provider tests, TypeScript/Biome, and independent cross-review. Dynamic listeners restore original model configuration through the revision-checked API and exact readback; restore failures remain failures and listener cleanup still runs.Five Plan browser cases passed with exact configuration restoration. Broader Plan → Questions run was 10 passed / 2 failed: both failures are an existing duplicate setup-request projection tracked in #654, reproduced with fresh backends and the #643 integration baseline. Uniqueness assertions remain intact; this PR does not claim #654 is fixed or all provider acceptance is green. Final-head Linux/Windows Node 22/24/26 and Web E2E CI must pass before the maintainer-authorized admin merge.