Skip to content

fix(sight): preserve user config on schema migration#1505

Merged
chengshuyi merged 1 commit into
alibaba:mainfrom
jfeng18:fix/config-migration-preserve-user
Jul 16, 2026
Merged

fix(sight): preserve user config on schema migration#1505
chengshuyi merged 1 commit into
alibaba:mainfrom
jfeng18:fix/config-migration-preserve-user

Conversation

@jfeng18

@jfeng18 jfeng18 commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

Replace the destructive config overwrite in ensure_default_agents_config with a shallow merge: start from the embedded default, overlay all top-level keys the user has set (except schema_version), bump to CURRENT_SCHEMA_VERSION.

Fixes #1496.

Problem

When upgrading from schema v1 → v2, ensure_default_agents_config (added in 98158d2) backed up the old config and overwrote it entirely with the embedded default. User customizations — cmdline rules, https rules, codex_offsets, feature overrides — were silently lost.

Fix

  • Parse both the on-disk config and the embedded default as serde_json::Value
  • Shallow-merge: for each top-level key in the user's config (except schema_version), overwrite the default's corresponding key
  • Set schema_version = CURRENT, write back pretty-printed
  • Backup (.bak) is still created (using fs::copy instead of fs::rename so the merged file has a fresh inode)

This preserves user data while adding any new sections (features, runtime_limits, etc.) from the default that the old config was missing.

Testing

Two discriminating tests added:

  • ensure_default_agents_config_preserves_user_cmdline_rules_on_merge: custom agent rule + https + deny rule survive migration
  • ensure_default_agents_config_adds_missing_sections_on_merge: missing features/runtime_limits are pulled from default

Both tests fail if reverted to the old overwrite logic (discriminating).

cargo test --lib -- config::tests::ensure_default_agents_config
# 8 passed, 0 failed

Verification

Check Status
cargo fmt --check pass
cargo clippy --all-targets -D warnings pass
cargo test --lib (1144 tests) pass
Discriminating: revert to overwrite → new tests fail yes

Replace the destructive overwrite in ensure_default_agents_config with a
shallow merge: start from the embedded default, overlay all top-level keys
the user has set (except schema_version), bump to CURRENT_SCHEMA_VERSION.

This preserves user customizations (cmdline rules, https rules,
codex_offsets, feature overrides) while adding any new sections from the
default that the old config was missing.

Fixes alibaba#1496
@jfeng18 jfeng18 requested a review from chengshuyi as a code owner July 15, 2026 15:29
@github-actions github-actions Bot added the component:sight src/agentsight/ label Jul 15, 2026

@qoderai qoderai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

  • 合并逻辑中的二次 JSON 解析使用 expect,配置损坏会导致进程 panic,建议改为返回错误。
  • 顶层“浅合并”目前对对象字段执行全覆盖,可能丢失新默认子字段,建议对 features/runtime_limits 等采用子字段级 merge 并澄清语义。
  • 备份文件按秒级时间戳命名且使用 copy,同秒多次迁移会静默覆盖旧备份,建议避免备份文件名冲突。

🤖 Generated by QoderView workflow run

Comment thread src/agentsight/src/config.rs
Comment thread src/agentsight/src/config.rs
Comment thread src/agentsight/src/config.rs
@jfeng18

jfeng18 commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

Responding to the bot review:

Point 1 (.expect panic on re-parse): Dismissed — false positive.

content is an immutable let binding (line 517, no mut). Line 527 already returns early if the JSON is invalid. Between 527 and 546, only shared references are taken — no file re-read, no mutation, no interior mutability. The same bytes that passed the parse gate are parsed again at 546. The .expect path is unreachable for any runtime input.

Point 2 (shallow merge loses nested default keys): Acknowledged as follow-up, not a blocker.

The shallow merge is intentional and documented (comment at line 537). Deep merge risks overwriting user intentional nested changes with defaults — the opposite of what #1496 set out to fix. New nested keys introduced by future schema bumps can be handled by a dedicated deep-merge enhancement if real-world breakage surfaces. Not blocking the current fix which is strictly better than the destructive overwrite it replaces.

Point 3 (timestamp collision in backup naming): Acknowledged as cosmetic nit, not blocking.

Requires two processes to start in the same second AND both find outdated schema_version (a once-per-upgrade event). Even if triggered, one valid backup still exists. Not worth the diff churn.

@chengshuyi chengshuyi 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.

LGTM. 浅合并逻辑清晰正确,测试 discriminating,对 bot review 的回应到位。可以合入。

@chengshuyi chengshuyi merged commit bf4706a into alibaba:main Jul 16, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component:sight src/agentsight/

Projects

None yet

2 participants