Skip to content

fix: escape all control characters in TOML basic strings - #7

Closed
hobostay wants to merge 1 commit into
MoonshotAI:mainfrom
hobostay:fix/toml-escape-control-chars
Closed

fix: escape all control characters in TOML basic strings#7
hobostay wants to merge 1 commit into
MoonshotAI:mainfrom
hobostay:fix/toml-escape-control-chars

Conversation

@hobostay

Copy link
Copy Markdown

Summary

  • escapeTomlBasicString only handled \b, \t, \n, \f, \r but not other control characters in U+0000-U+001F and U+007F
  • Per the TOML spec, basic strings may only contain certain escape sequences; unescaped control chars produce invalid TOML that smol-toml cannot parse
  • Now escapes all remaining control characters as \uXXXX sequences

Test plan

  • Verify config saves and loads correctly with normal values
  • Test that strings containing control characters produce valid TOML

🤖 Generated with Claude Code

escapeTomlBasicString only handled \b \t \n \f \r but not other
control characters in U+0000-U+001F and U+007F. Per the TOML spec,
basic strings may only contain certain escape sequences; unescaped
control chars produce invalid TOML that smol-toml cannot parse,
silently corrupting the config on next load.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@liruifengv

Copy link
Copy Markdown
Collaborator

Thank you for your interest in contributing to kimi-code.
However, we currently do not accept bulk AI-generated submissions that have not undergone thorough human review, so we will be closing these PR.

@liruifengv liruifengv closed this May 23, 2026
sailist added a commit that referenced this pull request Jun 5, 2026
…alue (P1.2)

Honour `SyncDescriptor.supportsDelayedInstantiation: true` by wrapping the
service in a `Proxy` that defers real construction until the first
non-event property access. Eager services (the default,
`supportsDelayedInstantiation: false`) remain unchanged — same eager
Reflect.construct path P1.1 installed.

  - NEW `di/util/idleValue.ts` — vendors `GlobalIdleValue<T>` from krow
    `base/async.ts` plus the `runWhenGlobalIdle` helper. Internally falls
    back to `setTimeout` (one frame ~15 ms) when `requestIdleCallback` is
    absent (the typical Node environment); reading `.value` before the
    idle tick fires cancels the schedule and runs the executor inline.
    Only `GlobalIdleValue` is exported; `runWhenGlobalIdle` is
    module-private because the DI subsystem is the sole consumer.

  - NEW `di/util/linkedList.ts` — vendors krow `base/linkedList.ts` (~80
    LOC, zero deps). Used by the Proxy `get` trap to park
    `onDid*`/`onWill*` event subscriptions while the real instance is
    still pending. Each `push` returns its own disposer for O(1) removal.

  - `_createServiceInstance(id, desc, supportsDelayedInstantiation,
    _trace)` — eager path unchanged; lazy path captures `_ctor`, `_args`,
    `earlyListeners: Map<string, LinkedList<EarlyListenerData>>` and
    builds a `GlobalIdleValue<T>` whose executor runs `_createInstance`,
    then replays every parked listener against the real event by calling
    `(result as any)[key](callback, thisArg, disposables)`. The returned
    Proxy traps `get` to (a) intercept `onDid*`/`onWill*` keys before
    materialisation and park subscribers, (b) materialise + memoise
    bound function references for other keys, and `getPrototypeOf`
    returns `_ctor.prototype` so `instance instanceof Foo === true`
    without forcing materialisation.

  - `_servicesToMaybeDispose: Set<any>` — new field on
    `InstantiationService`. Materialised lazy instances are added inside
    the `GlobalIdleValue` executor (not via `_setCreatedServiceInstance`)
    so `dispose()` can tear them down without ever reading the Proxy.

  - `_setCreatedServiceInstance` gains a `lazy: boolean = false`
    parameter; when true, the Proxy is written to `services` +
    `_instances` (so subsequent `get(id)` returns the SAME Proxy) but is
    intentionally NOT pushed to `_constructionOrder`. Disposing a Proxy
    would require reading a property of it, which would force
    materialisation just to probe for a `.dispose` method — exactly the
    pessimisation `supportsDelayedInstantiation` was designed to avoid.

  - `dispose()` now runs in two passes (PLAN D8 LIFO preserved for the
    eager set): first walk `_constructionOrder` in reverse and dispose
    each instance (also evicting from `_servicesToMaybeDispose` so we
    don't double-dispose), then iterate the remaining
    `_servicesToMaybeDispose` for Proxy-materialised real instances.

Tests (delayed.test.ts):

  - `a.get(IFoo)` on a lazy descriptor returns a Proxy WITHOUT triggering
    Foo's ctor.
  - Reading any non-event property triggers exactly one ctor call.
  - `proxy instanceof Foo === true` even pre-materialisation
    (`getPrototypeOf` trap).
  - Subscribing to `onDidChange` BEFORE first non-event access parks the
    listener; subsequent property read materialises Foo; firing the real
    event delivers the payload to the parked listener (replay logic).

Quality gates:
  - agent-core: 2458 pass / 1 skip / 1 todo.
  - daemon: 240 / 241 — the single failure is the known
    `fs-watch.e2e.test.ts:257` / `fs-search.e2e.test.ts` timing flake at
    baseline `57e7bfc69` (verified by checking out baseline files and
    re-running: same failure). Per Phase 0 reviewer note #7 this is
    treated as a pre-existing flake, NOT a P1.2 regression.

Review verdict: `/tmp/di-uplift-2026.06.05/phase-1/reviews.log`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sailist added a commit that referenced this pull request Jun 5, 2026
…alue (P1.2)

Honour `SyncDescriptor.supportsDelayedInstantiation: true` by wrapping the
service in a `Proxy` that defers real construction until the first
non-event property access. Eager services (the default,
`supportsDelayedInstantiation: false`) remain unchanged — same eager
Reflect.construct path P1.1 installed.

  - NEW `di/util/idleValue.ts` — vendors `GlobalIdleValue<T>` from krow
    `base/async.ts` plus the `runWhenGlobalIdle` helper. Internally falls
    back to `setTimeout` (one frame ~15 ms) when `requestIdleCallback` is
    absent (the typical Node environment); reading `.value` before the
    idle tick fires cancels the schedule and runs the executor inline.
    Only `GlobalIdleValue` is exported; `runWhenGlobalIdle` is
    module-private because the DI subsystem is the sole consumer.

  - NEW `di/util/linkedList.ts` — vendors krow `base/linkedList.ts` (~80
    LOC, zero deps). Used by the Proxy `get` trap to park
    `onDid*`/`onWill*` event subscriptions while the real instance is
    still pending. Each `push` returns its own disposer for O(1) removal.

  - `_createServiceInstance(id, desc, supportsDelayedInstantiation,
    _trace)` — eager path unchanged; lazy path captures `_ctor`, `_args`,
    `earlyListeners: Map<string, LinkedList<EarlyListenerData>>` and
    builds a `GlobalIdleValue<T>` whose executor runs `_createInstance`,
    then replays every parked listener against the real event by calling
    `(result as any)[key](callback, thisArg, disposables)`. The returned
    Proxy traps `get` to (a) intercept `onDid*`/`onWill*` keys before
    materialisation and park subscribers, (b) materialise + memoise
    bound function references for other keys, and `getPrototypeOf`
    returns `_ctor.prototype` so `instance instanceof Foo === true`
    without forcing materialisation.

  - `_servicesToMaybeDispose: Set<any>` — new field on
    `InstantiationService`. Materialised lazy instances are added inside
    the `GlobalIdleValue` executor (not via `_setCreatedServiceInstance`)
    so `dispose()` can tear them down without ever reading the Proxy.

  - `_setCreatedServiceInstance` gains a `lazy: boolean = false`
    parameter; when true, the Proxy is written to `services` +
    `_instances` (so subsequent `get(id)` returns the SAME Proxy) but is
    intentionally NOT pushed to `_constructionOrder`. Disposing a Proxy
    would require reading a property of it, which would force
    materialisation just to probe for a `.dispose` method — exactly the
    pessimisation `supportsDelayedInstantiation` was designed to avoid.

  - `dispose()` now runs in two passes (PLAN D8 LIFO preserved for the
    eager set): first walk `_constructionOrder` in reverse and dispose
    each instance (also evicting from `_servicesToMaybeDispose` so we
    don't double-dispose), then iterate the remaining
    `_servicesToMaybeDispose` for Proxy-materialised real instances.

Tests (delayed.test.ts):

  - `a.get(IFoo)` on a lazy descriptor returns a Proxy WITHOUT triggering
    Foo's ctor.
  - Reading any non-event property triggers exactly one ctor call.
  - `proxy instanceof Foo === true` even pre-materialisation
    (`getPrototypeOf` trap).
  - Subscribing to `onDidChange` BEFORE first non-event access parks the
    listener; subsequent property read materialises Foo; firing the real
    event delivers the payload to the parked listener (replay logic).

Quality gates:
  - agent-core: 2458 pass / 1 skip / 1 todo.
  - daemon: 240 / 241 — the single failure is the known
    `fs-watch.e2e.test.ts:257` / `fs-search.e2e.test.ts` timing flake at
    baseline `57e7bfc69` (verified by checking out baseline files and
    re-running: same failure). Per Phase 0 reviewer note #7 this is
    treated as a pre-existing flake, NOT a P1.2 regression.

Review verdict: `/tmp/di-uplift-2026.06.05/phase-1/reviews.log`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@trustmiao

Copy link
Copy Markdown

Planning note for issue/PR #7 (no code changes made yet).

Requirement confirmed:

  • The report is about TOML basic string escaping for client preferences written to ~/.kimi-code/tui.toml.
  • Current apps/kimi-code/src/tui/config.ts uses a hand-written escapeTomlBasicString that escapes \, ", \b, \t, \n, \f, and \r, but leaves the other disallowed control characters in U+0000-U+001F and U+007F unescaped.
  • That can make renderTuiConfig / saveTuiConfig write invalid TOML, which later fails through smol-toml in parseTuiConfig / loadTuiConfig.

Code impact found:

  • Affected path: apps/kimi-code/src/tui/config.ts (renderTuiConfig, saveTuiConfig, escapeTomlBasicString).
  • Existing focused tests: apps/kimi-code/test/tui/config.test.ts already covers normal save/load and quote/backslash round-trips.
  • config.toml write paths in packages/agent-core/src/config/toml.ts use smol-toml.stringify, so they do not appear to share this hand-written escaping issue.
  • Migration TOML write paths in packages/migration-legacy/src/steps/config.ts also use smol-toml.stringify.
  • TUI settings commands call saveTuiConfig, so fixing the serializer should cover /theme, editor preference, notifications, and upgrade preference writes without touching command handlers.

Implementation plan:

  1. Update escapeTomlBasicString in apps/kimi-code/src/tui/config.ts to escape every TOML-disallowed control character not already covered by the named escapes as uppercase \u00XX sequences, including U+0000-U+0007, U+000E-U+001F, and U+007F.
  2. Keep the existing named escapes for backspace, tab, line feed, form feed, and carriage return so current rendered output remains readable and stable.
  3. Do not broaden the change to config.toml serialization unless tests reveal a separate issue, because core already delegates serialization to smol-toml.stringify.
  4. Keep the helper local unless another current hand-written TOML string renderer is found during implementation; avoid adding a shared abstraction for a single call site.

Validation plan:

  1. Extend apps/kimi-code/test/tui/config.test.ts rather than adding a new test file.
  2. Add a regression test that saves a TUI config whose theme or editor command contains representative controls such as NUL (U+0000), SOH (U+0001), ESC (U+001B), and DEL (U+007F), then reloads it and verifies the original string round-trips.
  3. Assert the written TOML contains escaped sequences such as \u0000, \u0001, \u001B, and \u007F, and does not contain the raw control bytes for those cases.
  4. Re-run the existing normal save/load and quote/backslash tests to confirm no regression.
  5. Run focused verification: pnpm --filter @moonshot-ai/kimi-code exec vitest run test/tui/config.test.ts.
  6. If implementation touches shared config behavior unexpectedly, expand verification to the relevant core config tests; otherwise keep the test scope focused.

wintrover added a commit to wintrover/kimy that referenced this pull request Jun 29, 2026
- kimy wrapper: move hash writes after smoke test (MoonshotAI#1)
- kimy wrapper: add public/ to web hash inputs (MoonshotAI#3)
- kimy wrapper: widen vis hash to include config files (MoonshotAI#4)
- kimy wrapper: move lockfile from /tmp to ~/.kimy/bin (MoonshotAI#5)
- kimy wrapper: use explicit package list for native hash (MoonshotAI#7)
- 01-bundle.mjs: skip vis-asset build when already done (MoonshotAI#2)
- justfile: sync deploy with new wrapper, add deploy-full (MoonshotAI#9,MoonshotAI#10,MoonshotAI#11)
- flake.nix: add unpin guidance to nixpkgs comment (MoonshotAI#12)
wintrover added a commit to wintrover/kimy that referenced this pull request Jun 29, 2026
…nshotAI#8

Replace manual file-set declarations with build engine introspection.

- tsdown native config: add manifestPlugin() that writes build-manifest.json
  via OutputChunk.moduleIds (850 bundled modules, zero test files)
- kimy wrapper: lockfile-based dependency hashing via pnpm-lock.yaml importers
- kimy wrapper: manifest-based native hashing (bundledModules only)
- kimy wrapper: double-checked locking inside flock (TOCTOU fix)
- kimy wrapper: atomic hash file writes via temp + mv
asdshuaishuai pushed a commit to d2rabbit/kimi-code that referenced this pull request Jul 21, 2026
…oonshotAI#7)

SettingsView previously had two separate entry points (添加供应商 and
添加自定义模型) that opened ProviderModelDialog in mode='provider' or
mode='model' — disjoint forms that couldn't compose (e.g. you couldn't
create a provider AND a model AND configure thinking in one go).

Rewrote ProviderModelDialog as a single unified form with three sections:

  ① 供应商    — type/id/key/baseURL OR pick existing provider
  ② 模型     — optional alias for the provider (toggleable off)
  ③ 思考     — toggle + effort level pills (minimal/low/medium/high)

The thinking section mirrors kimi-code's config.toml [thinking] schema:
  - enabled = true → effort pick is one of minimal/low/medium/high
  - enabled = false → submits 'off' (no thinking)

Effort levels are taken from the user's explicit request
(minimal/low/medium/high), reflecting the actual values kimi-code
accepts in [thinking] effort = "…". The footer of the dialog shows
the equivalent TOML snippet for clarity.

Both SettingsView entry points ('添加供应商' and '添加自定义模型') now
open the same unified dialog — they default to the same form; the user
can toggle the model section off if they only want a provider.

svelte-check 0 errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants