fix trampling projects table when accepting trusted dirs #3434
Conversation
…st migration test
- set_project_trusted now converts a top-level inline `projects = { ... }` map into explicit `[projects."<path>"]` tables, preserving existing entries and setting `trust_level = "trusted"`.
- Add unit test to cover migrating three existing trusted paths and approving a new directory "/Users/mbolin/code/codex2".
Co-Authored-By: Codex <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
- Convert inline project entries to explicit tables by cloning the inner inline table into a table, preserving all keys.\n- Create the projects table as implicit when creating it, and insert after migration when needed.\n- Update tests to assert preservation of extra fields.\n\nCo-Authored-By: Codex <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
| fn test_set_project_trusted_migrates_top_level_inline_projects_preserving_entries() | ||
| -> anyhow::Result<()> { | ||
| let initial = r#"foo = "bar" | ||
| projects = { "/Users/mbolin/code/codex4" = { trust_level = "trusted", foo = "bar" } , "/Users/mbolin/code/codex3" = { trust_level = "trusted" } } |
There was a problem hiding this comment.
Just to avoid ambiguity in the test output, I wouldn't do foo = "bar" in both places because it makes it more confusing to conclusively determine what moved.
There was a problem hiding this comment.
I would also include some TOML comments on various things to see where they end up (or if they get lost).
There was a problem hiding this comment.
@bolinfest those seem like tests that would be better off in the toml_edit crate, i don't feel we should be testing dependencies here.
i'll update foo/bar so it's distinct.
| Err(e) => return Err(e.into()), | ||
| }; | ||
|
|
||
| set_project_trusted_inner(&mut doc, project_path)?; | ||
|
|
||
| // ensure codex_home exists | ||
| std::fs::create_dir_all(codex_home)?; |
There was a problem hiding this comment.
I noted a similar thing on #2799, but I would just limit this to the NotFound arm above because that's the only case where it should be necessary.
There was a problem hiding this comment.
eh, i sort of feel like it's conceptually cleaner to ensure the dir exists before writing. in this case i think it's moot because with a blank initial config i don't think set_project_trusted_inner will ever return Err, but it's sort of weird if we create the dir before knowing whether set_project_trusted_inner will succeed—we could end up creating the directory but not creating the file inside it.
i'll leave this as-is for now as i feel it reads better and guards against a possible "read->create dir->error" case in future.
No description provided.