Skip to content

fix(setup): keep session_start silent for legacy-only configurations - #632

Closed
samsen3 wants to merge 1 commit into
openpi-dev:mainfrom
samsen3:fix/setup-session-start-legacy-warning
Closed

samsen3 wants to merge 1 commit into
openpi-dev:mainfrom
samsen3:fix/setup-session-start-legacy-warning

Conversation

@samsen3

@samsen3 samsen3 commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

Problem

After pulling a newer OpenPI, every session_start immediately printed a warning whose text mentioned both legacy format and unknown fields, even when the on-disk my-pi-setup.json only lacked configVersion and every other key was recognised. This contradicted the documented contract — SETUP.md ("Unversioned documents migrate only on an explicit save") and the design from #498 — and forced users who had valid pre-configVersion files to re-run /openpi-setup for a non-problem. The notification also conflated two unrelated categories ("legacy format" vs "unknown fields") so the user could not tell which problem they actually had without inspecting diagnostics manually.

Fixes #631

Value

  • Upgrades from any pre-configVersion OpenPI are silent: the existing file keeps loading, the file is not modified, and the user is not pushed to /openpi-setup.
  • When a startup notification is genuinely warranted, it names the actual category and the affected path, so the user does not have to rerun /openpi-setup just to find out where the warning lives.
  • Trivial regression risk: behaviour for malformed JSON, unsupported version, unknown fields, and the safe-default fallback is preserved unchanged.

Approach

Branch on the diagnostics in extensions/setup/index.ts's session_start handler instead of treating the whole downstream as one bucket:

  • legacy-only writable case → no notification. Per SETUP.md and setup: 配置诊断与 fail-closed 回滚,不新增第二条配置入口 #498 the file is already a safe, opt-in-to-migrate state.
  • any severity: "error" → keep the existing safe-default notification.
  • any other warning → keep notifying, but drop the misleading "legacy format or unknown fields" wording and surface the actual severity @ path for the first three diagnostics (; … for the rest).

Two small private helpers (isLegacyUnversionedDiagnostic, buildSessionStartNotification) are added inside extensions/setup/index.ts. extensions/shared/setup-config.ts is not touched: no new exports, no schema changes, no changes to inspectDocument, no changes to the writable semantics or the write path. Migration stays opt-in via /openpi-setup; the file is never auto-rewritten on startup.

Validation

  • bun run format:check ✅

  • bun run lint ✅

  • bun run check:config-contract ✅

  • bun run check:discipline ✅

  • bun run check:docs-contract ✅

  • tests/extensions/setup/** — 39/39 passing

    • new file tests/extensions/setup/session-start-notification.test.ts covers: legacy-only known keys stays quiet; legacy + unknown field still warns and names the path; malformed JSON still routes through the safe-default branch; inspectSetupConfig agrees with the new behaviour.
    • existing tests/extensions/setup/index.test.ts split the old combined assertion: the unknown-field case keeps warning and the new assertion drops "legacy format or unknown fields" and expects "warning @ future"; a new test pins "legacy-only file is silent on session_start".
  • bun run test — 1927 pass / 1 fail. The remaining failure is tests/web/pi-coding-agent-entry.test.ts:299 (real checkout 0.85.1 exports resolve from this install), which expects the workbuddy-managed Pi checkout to be 0.85.1 but it is 0.84.1 in this environment. Pre-existing, unrelated to this change.

  • bun run check and bun run typecheck also surface web/ui errors (@types/react, zustand, i18next not installed in web/ui). Pre-existing environment issue, unrelated to this change. Root tsconfig.json only includes extensions, tests, benchmarks, scripts; files touched by this PR type-check cleanly against that config.

  • User-visible behavior: minimal-diff notification fix; no new tooling, no schema change, no auto-migration.

  • Model-visible context/tools: none.

  • Runtime/lifecycle: session_start keeps the same early-return shape; no change to the episode machine.

  • Persisted config/data: my-pi-setup.json is not rewritten by this PR.

  • Compatibility: pre-configVersion files continue to load; existing /openpi-setup save path still migrates them on explicit user action.

The session_start notification previously warned every time the
configuration file was missing configVersion, even when every key in the
file was recognised and the file loaded successfully. That contradicts the
contract in SETUP.md / openpi-dev#498: an unversioned document migrates only on
explicit save, so an upgrade from an older OpenPI must not require a
fresh run of /openpi-setup.

Branch on the diagnostics:
- legacy-only writable configuration: no notification
- error: keep the existing safe-default notification
- any other warning: keep notifying, but drop the misleading
  "legacy format or unknown fields" wording and surface the actual
  severity @ path so the user does not need to rerun /openpi-setup just
  to learn where the problem is

Behaviour for malformed JSON / unsupported version / unknown fields is
unchanged.

Refs openpi-dev#631
@github-actions github-actions Bot added the area:setup OpenPI setup, configuration, or setup documentation label Sep 28, 2026

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Bound and sanitize unknown-field paths in the startup warning (extensions/setup/index.ts)

The new notification interpolates diagnostic.path directly. inspectDocument() derives that path from configuration JSON keys, which can be arbitrarily long and contain control characters. Even though this candidate displays at most three diagnostics, one long or control-bearing key can make every session_start notification noisy or malformed. Cap the displayed path length and sanitize terminal text, with a regression for an unknown key containing long/control text. #633 is an overlapping implementation of the same issue and should be reconciled before merge.

Reviewed head f592e58 against current main ad68e44. CI is green, but this input is not covered by the added tests.

@tt-a1i

tt-a1i commented Oct 4, 2026

Copy link
Copy Markdown
Collaborator

Superseded by #633, whose reviewed startup diagnostics and tests are now merged through the fully validated #643 integration. The adopted implementation additionally bounds path count/length and strips terminal control characters; merging this alternative would duplicate or regress that behavior. No branch deletion.

@tt-a1i tt-a1i closed this Oct 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:setup OpenPI setup, configuration, or setup documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(setup): session_start warning conflates legacy and unknown-field cases and stays noisy when only configVersion is missing

2 participants