Skip to content

fix(sight): don't auto-overwrite invalid JSON configs#1504

Merged
jfeng18 merged 1 commit into
alibaba:mainfrom
zhangtaibo:fix/agentsight-no-overwrite-invalid-json
Jul 15, 2026
Merged

fix(sight): don't auto-overwrite invalid JSON configs#1504
jfeng18 merged 1 commit into
alibaba:mainfrom
zhangtaibo:fix/agentsight-no-overwrite-invalid-json

Conversation

@zhangtaibo

Copy link
Copy Markdown
Contributor

What & Why

Fixes #1502.

ensure_default_agents_config (added in 98158d2 to fix #1427) silently overwrote invalid JSON config files with the embedded default, suppressing the documented WARN Failed to load config from {path:?}: {e}, using embedded defaults log line.

Root cause: extract_schema_version internally does serde_json::from_str::<VersionProbe>(json).ok() — for invalid JSON it returns None. Then None < Some(CURRENT_SCHEMA_VERSION) is true, so the function backed up the file to .bak.<ts> and overwrote it with the default. By the time load_from_file ran, the file was valid JSON → parse_json_rules succeeded → the config_load_err warn branch in AgentSight::new (src/unified.rs:186) never fired.

Reproduction (on 0.8.0 RPM, before this fix)

echo 'NOT_VALID_JSON_AT_ALL{{{' > /tmp/agentsight_invalid_cfg_test.json
timeout 8 agentsight trace --config /tmp/agentsight_invalid_cfg_test.json > /tmp/invalid_config_test.log 2>&1
cat /tmp/invalid_config_test.log
# empty — no "JSON parse error" warn
ls /tmp/agentsight_invalid_cfg_test.json*
# original silently backed up to .bak.<ts>, file rewritten with default schema_version=2

This breaks agentic-os-tests/agentsight-tests/tests/test_config_validation.py::TestInvalidConfigHandling::test_invalid_json_config_returns_error and TestConfigFileParsingErrors::test_config_truncated_json_error (both failing on the 2026-07-15 regression run).

Fix

In ensure_default_agents_config (src/agentsight/src/config.rs), probe JSON validity with serde_json::from_str::<serde_json::Value> before calling extract_schema_version. If the parse fails, return Ok(()) without touching the file — this lets the caller's load_from_file → parse_json_rules surface JSON parse error: <serde error> and the warn-fallback path fire. Auto-upgrade is only meaningful for valid JSON whose schema_version is outdated.

let content = std::fs::read_to_string(path)
    .with_context(|| format!("Failed to read existing config at {path:?}"))?;

// If the file is not valid JSON at all, leave it untouched so the caller's
// load_from_file → parse_json_rules surfaces "JSON parse error: ..." and
// AgentSight::new emits its `Failed to load config from {path:?}: {e},
// using embedded defaults` warn log. Auto-upgrade is only meaningful for
// *valid* JSON whose schema_version is outdated; silently overwriting an
// invalid file masks the parse error and breaks the documented fallback
// contract (see issue #1502).
if serde_json::from_str::<serde_json::Value>(&content).is_err() {
    return Ok(());
}

let on_disk_version = extract_schema_version(&content);
// ... existing auto-upgrade logic unchanged ...

This preserves the #1427 auto-upgrade behavior for valid-but-stale configs (still backed up + overwritten when schema_version < CURRENT_SCHEMA_VERSION) while restoring the parse-error warn log for invalid JSON.

Tests

Added 2 regression tests in config::tests:

  • ensure_default_agents_config_leaves_invalid_json_untouched — writes "NOT_VALID_JSON_AT_ALL{{{", calls ensure_default_agents_config, asserts file content unchanged and no .bak file created.
  • ensure_default_agents_config_leaves_truncated_json_untouched — same with a truncated-JSON variant.

All existing ensure_default_agents_config_* tests still pass:

running 6 tests
test config::tests::ensure_default_agents_config_leaves_truncated_json_untouched ... ok
test config::tests::ensure_default_agents_config_leaves_invalid_json_untouched ... ok
test config::tests::ensure_default_agents_config_preserves_current_config ... ok
test config::tests::ensure_default_agents_config_creates_file_when_missing ... ok
test config::tests::ensure_default_agents_config_upgrades_old_schema_version ... ok
test config::tests::ensure_default_agents_config_upgrades_stale_config ... ok

test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 1083 filtered out

End-to-end verification on test env (47.239.61.17)

Built agentsight release binary with this fix, scp'd to test env, ran manual repro:

$ echo 'NOT_VALID_JSON_AT_ALL{{{' > /tmp/agentsight_invalid_cfg_test.json
$ timeout 8 /tmp/agentsight-fixed trace --config /tmp/agentsight_invalid_cfg_test.json > /tmp/invalid_config_test.log 2>&1
$ cat /tmp/invalid_config_test.log
[2026-07-15T13:55:17.835Z WARN  agentsight::unified:186] Failed to load config from "/tmp/agentsight_invalid_cfg_test.json": Failed to parse config from "/tmp/agentsight_invalid_cfg_test.json": JSON parse error: expected value at line 1 column 1, using embedded defaults
$ cat /tmp/agentsight_invalid_cfg_test.json
NOT_VALID_JSON_AT_ALL{{{
$ ls /tmp/agentsight_invalid_cfg_test.json*
/tmp/agentsight_invalid_cfg_test.json

The expected WARN ... JSON parse error: ... using embedded defaults log line now fires, and the invalid file is left untouched (no .bak backup). Same behavior verified for truncated JSON ({"cmdline": {"allow": [{"rule": ["*cosh*").

Related

ensure_default_agents_config (added by 98158d2 to fix alibaba#1427) treated
invalid JSON files as "outdated schema_version" configs: extract_schema_version
internally swallows the serde error and returns None, None < Some(CURRENT)
is true, so the function backed up the file to .bak.<ts> and overwrote it
with the embedded default. By the time load_from_file ran, the file was
valid JSON, so parse_json_rules succeeded and the config_load_err warn
branch in AgentSight::new (src/unified.rs:186) never fired.

Net effect: `agentsight trace --config <invalid.json>` produced empty
output — the documented "Failed to load config from {path:?}: {e}, using
embedded defaults" WARN log was silently suppressed, and the user's file
was rewritten without warning.

Fix: probe JSON validity with serde_json::from_str::<Value> before
extract_schema_version. If parse fails, return Ok without touching the
file so load_from_file surfaces "JSON parse error: <serde error>" and
the warn-fallback path fires. Auto-upgrade is only meaningful for
*valid* JSON whose schema_version is outdated.

Adds 2 regression tests:
- ensure_default_agents_config_leaves_invalid_json_untouched
- ensure_default_agents_config_leaves_truncated_json_untouched

Verified locally: 6 config::tests::ensure_default_agents_config_* tests
pass. On test env 47.239.61.17, the rebuilt agentsight binary now emits
the expected WARN log for both invalid and truncated JSON configs, and
leaves the original file untouched (no .bak backup).

Closes alibaba#1502.
@github-actions github-actions Bot added the component:sight src/agentsight/ label Jul 15, 2026

@jfeng18 jfeng18 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. Fix is minimal and correct: invalid JSON → early return without touching the file, letting the downstream load_from_file warn-fallback fire as documented. Three discriminating tests cover invalid/truncated/binary cases. Consistent with our root-cause analysis of #1502.

@jfeng18 jfeng18 merged commit e538b05 into alibaba:main Jul 15, 2026
20 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