Skip to content

Commit 792df27

Browse files
fix: guard os.chown for Windows, reject non-string JSON keys
- _atomic.py: guard os.chown calls with hasattr(os, 'chown') so the fix command works on Windows where os.chown does not exist. Both atomic_write_text and atomic_write_bytes now skip ownership restoration on platforms without chown support. - cli.py JSON write: reject non-string keys before json.dumps to prevent type coercion (int 1 → string 1) that causes perpetual drift on reload, or duplicate keys when the target already contains the string form. Addresses Codex review: _atomic.py:43 (P1), cli.py:481 (P2)
1 parent 0a6d925 commit 792df27

2 files changed

Lines changed: 31 additions & 6 deletions

File tree

src/configdrift/_atomic.py

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,13 @@ def atomic_write_text(path: Path, text: str, encoding: str = "utf-8") -> None:
4040
st = resolved.stat()
4141
os.chmod(tmp, st.st_mode)
4242
try:
43-
os.chown(tmp, st.st_uid, st.st_gid)
43+
if hasattr(os, "chown"):
44+
os.chown(tmp, st.st_uid, st.st_gid)
45+
else:
46+
# os.chown is Unix-only; on Windows ownership is managed
47+
# by the filesystem ACLs and mkstemp already creates
48+
# the temp file with the caller's identity.
49+
pass
4450
except OSError as chown_err:
4551
with contextlib.suppress(OSError):
4652
os.unlink(tmp)
@@ -79,7 +85,13 @@ def atomic_write_bytes(path: Path, data: bytes) -> None:
7985
st = resolved.stat()
8086
os.chmod(tmp, st.st_mode)
8187
try:
82-
os.chown(tmp, st.st_uid, st.st_gid)
88+
if hasattr(os, "chown"):
89+
os.chown(tmp, st.st_uid, st.st_gid)
90+
else:
91+
# os.chown is Unix-only; on Windows ownership is managed
92+
# by the filesystem ACLs and mkstemp already creates
93+
# the temp file with the caller's identity.
94+
pass
8395
except OSError as chown_err:
8496
with contextlib.suppress(OSError):
8597
os.unlink(tmp)

src/configdrift/cli.py

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -469,15 +469,28 @@ def fix(
469469
if ext == ".json":
470470
import json as _json
471471

472+
# Reject non-string keys before JSON write-back.
473+
# json.dumps coerces int keys to strings (1 → "1"), so
474+
# reloading the JSON produces a string key that no longer
475+
# matches the baseline's integer key, causing perpetual
476+
# drift. It can also create duplicate names when the
477+
# target already contains the string form.
478+
non_str_keys = [k for k in target_data if not isinstance(k, str)]
479+
if non_str_keys:
480+
console.print(
481+
f"[red]Error: JSON keys must be strings. "
482+
f"Non-string keys from baseline: {', '.join(repr(k) for k in non_str_keys[:5])}[/red]"
483+
)
484+
failed_targets.append(str(target_path))
485+
continue
486+
472487
# Preserve nested JSON structure: rebuild from flat keys.
473488
# Literal dotted keys (keys that already contain '.') in the
474489
# source document are kept as single mapping keys rather
475490
# than being re-split into nested levels.
476491
nested: dict[str, Any] = {}
477492
for k, v in target_data.items():
478-
# Preserve non-string keys (valid in YAML) without
479-
# applying string operations that would raise TypeError.
480-
if not isinstance(k, str) or "." not in k:
493+
if "." not in k:
481494
nested[k] = v
482495
else:
483496
parts = k.split(".")
@@ -487,7 +500,7 @@ def fix(
487500
d[part] = {}
488501
d = d[part]
489502
d[parts[-1]] = v
490-
atomic_write_text(target_path, _json.dumps(nested_json, indent=2, default=_json_null_handler) + "\n")
503+
atomic_write_text(target_path, _json.dumps(nested, indent=2, default=_json_null_handler) + "\n")
491504
elif ext in (".yaml", ".yml"):
492505
# Reconstruct nested structure from flat keys for YAML output
493506
nested: dict[str, Any] = {}

0 commit comments

Comments
 (0)