Conversation
bolinfest
force-pushed
the
pr8354
branch
6 times, most recently
from
3374782 to
7f38c98
Compare
joshka-oai
left a comment
joshka-oai
left a comment
There was a problem hiding this comment.
My main two comments are:
Do we want to allow a .codex folder with no config?
https://github.com/openai/codex/pull/8354/changes#r2637386529
Would this fail on the root of a windows drive?https://github.com/openai/codex/pull/8354/changes#r2637390433
The rest are observations / nits / typos / some future ideas as well as requests for docs to make understanding why this code is there a bit easier for future devs / agents.
Approval subject to the answers to the main questions being ok. I'd suggest updating docs and typos at a minimum (doesn't need a re-review)
| let Ok(resolved) = value.clone().try_into::<ConfigToml>() else { | ||
| return Ok(value); | ||
| }; | ||
| drop(_guard); | ||
| let resolved_value = TomlValue::try_from(resolved).map_err(|e| { | ||
| io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| format!("Failed to serialize resolved config: {e}"), |
There was a problem hiding this comment.
(docs)
I kind of don't understand what this is doing / why. Converting from a toml value to a config toml value, then back to a toml value, but with a guard around it (which I'm assuming applies to the thread?)
There was a problem hiding this comment.
Added comments here and in codex-rs/utils/absolute-path/src/lib.rs.
| Ok(copy_shape_from_original(&value, &resolved_value)) | ||
| } | ||
|
|
||
| fn copy_shape_from_original(original: &TomlValue, resolved: &TomlValue) -> TomlValue { |
There was a problem hiding this comment.
(docs)
This could do with a doc comment explaining input -> output and purpose.
There was a problem hiding this comment.
Added comments and a unit test in this file.
|
|
||
| async fn find_project_root(cwd: &AbsolutePathBuf) -> io::Result<AbsolutePathBuf> { | ||
| for ancestor in cwd.as_path().ancestors() { | ||
| let git_dir = ancestor.join(".git"); |
There was a problem hiding this comment.
(Not a blocker)
https://github.com/openai/codex/pull/8359/changes adds various other markers for this.
I think we should probably also add some specific roots to this instead of discovering them.
E.g.:
- the current git worktree (from
git worktree list - the current jj workspace (from
jj workspace rootorjj workspace list) - other vcs roots that give programmatic access
There was a problem hiding this comment.
Discussed via Slack: we're going to keep this feature SCM-agnostic right now and do not want to confuse users that our SCM layer is "pluggable." Features like undo are tied to Git today.
| /// [ConfigLayerSource::Project] as the source) between `cwd` and | ||
| /// `project_root`, inclusive. The list is ordered in _increasing_ precdence, | ||
| /// starting from folders closest to `project_root` (which is the lowest | ||
| /// precedence) to those closest to `cwd` (which si the highest precedence). |
There was a problem hiding this comment.
(typo)
| /// precedence) to those closest to `cwd` (which si the highest precedence). | |
| /// precedence) to those closest to `cwd` (which is the highest precedence). |
There was a problem hiding this comment.
Thanks, failed by codespell.
| Ok(()) | ||
| } | ||
|
|
||
| fn resolve_config_paths(value: TomlValue, base_dir: &Path) -> io::Result<TomlValue> { |
There was a problem hiding this comment.
(docs)
Please add a small doc comment here to clarify intent of this method.
What does resolving paths do?
Why is it paths not path?
There was a problem hiding this comment.
Renamed to resolve_relative_paths_in_config_toml(). The "paths" are the referenced paths in the file.
| if err.kind() != io::ErrorKind::NotFound { | ||
| return Err(io::Error::new( | ||
| err.kind(), | ||
| format!( | ||
| "Failed to read project config file {}: {err}", | ||
| config_file.as_path().display(), | ||
| ), | ||
| )); | ||
| } |
There was a problem hiding this comment.
This is the point where fs-err might come in handy for reducing a bit of code - not a big deal and if we do it probably worth doing as a general codex cleanup.
There was a problem hiding this comment.
Agree it seems more appropriate for a general code cleanup.
| )); | ||
| } | ||
| Err(err) => { | ||
| if err.kind() != io::ErrorKind::NotFound { |
There was a problem hiding this comment.
(blocking question)
Do we want to fail when a .codex folder exists without a config file in it?
There was a problem hiding this comment.
I would say no: ultimately one could have .codex/skills without .codex/config.toml, I believe.
There was a problem hiding this comment.
We only return an error when err is NOT ENOENT, so I don't believe we are failing in the current case.
Though we should still return a layer when err is ENOENT?
There was a problem hiding this comment.
Updated the logic and added project_layer_is_added_when_dot_codex_exists_without_config_toml() test.
There was a problem hiding this comment.
Ah, I fixed this in load_project_layers(), but not for ConfigLayerSource::User.
| dot_codex_folder: current, | ||
| } = &layer.name | ||
| { | ||
| if let Some(previous) = previous_project { |
There was a problem hiding this comment.
Is previous higher or lower precedence in this method? (requires reading the entire method to guess about this, making it difficult to judge the correctness of this change)
There was a problem hiding this comment.
Added a comment.
2 participants
.codex/config.tomlin repo (fromcwdup to the first.gitfound, if any) as layers inConfigLayerStack. A newConfigLayerSource::Projectvariant was added to support this.config.tomlafter merging everything into onetoml::Value, which is wrong: paths should be relativized with respect to the folder containing theconfig.tomlthat was deserialized. This PR introduces a deserialize/re-serialize strategy to account for this inresolve_config_paths(). (This is whySerializeis added to so many types as part of this PR.)Stack created with Sapling. Best reviewed with ReviewStack.