feat(config): dynamic configuration reload during batch execution (#402)#450
Conversation
Polling-based file watcher for hot-reloading CODEFRAME.md/AGENTS.md/ CLAUDE.md during batch execution. Thread-safe shared state with last-known-good fallback on parse failure. Adds CONFIG_RELOADED and CONFIG_RELOAD_FAILED event types.
Start/stop ConfigFileWatcher in _execute_serial and _execute_parallel. Check for config reloads between task dispatches. Store reload timestamps in batch results. Display in batch status output.
End-to-end tests: modify config during watcher execution, verify reload detection, multiple reloads, invalid config preservation, has_reloaded_since tracking, and clean thread shutdown.
WalkthroughAdds a polling-based config file watcher and in-memory reload state; integrates watcher into the conductor to detect and apply config reloads between task dispatches; records reload timestamps in batch results and surfaces them in the CLI batch status output. Changes
Sequence Diagram(s)sequenceDiagram
actor OP as Operator
participant COND as Conductor
participant CW as ConfigFileWatcher
participant FS as Filesystem
participant TASKS as TaskDispatcher
participant EV as EventSystem
OP->>COND: Start batch execution
COND->>CW: start(initial_prefs)
CW->>FS: capture initial mtimes
CW->>CW: spawn polling thread
loop for each task/group
COND->>CW: poll / check for pending reloads
CW->>FS: compare mtimes
alt file changed
CW->>CW: reload() -> parse prefs
alt valid
CW->>CW: apply_reload(new_prefs, timestamp)
CW->>EV: emit CONFIG_RELOADED (diff)
CW-->>COND: notify reload detected
else invalid
CW->>CW: record_error(msg)
CW->>EV: emit CONFIG_RELOAD_FAILED
CW-->>COND: notify error (non-fatal)
end
end
COND->>TASKS: dispatch next task with current prefs
end
OP->>COND: Batch completes
COND->>CW: stop()
CW->>CW: signal thread exit / join
COND->>OP: return batch results (includes reload timestamps)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
|
Review: feat(config): dynamic configuration reload (Issue 402) Good implementation overall - the architecture follows the existing StallMonitor pattern closely, the fallback-to-last-known-good is correct, and the test coverage is solid. A few issues worth addressing before merge. BUGS
_check_for_changes wraps the first mtime loop in try/except OSError, but the second loop (detecting newly-created files) does not: If a file is created and deleted between is_file() and stat(), this raises an unhandled OSError in the daemon thread, silently killing the watcher. Wrap in the same try/except as the first loop.
Both _execute_serial and _execute_parallel have: This evaluates but the result is discarded. Not introduced by this PR but now sits inside the new try block, making it more visible. DESIGN
Local variables config_watcher_p, reload_state_p, _last_seen_reload_p have a _p suffix that is not needed since they are in a separate function scope from the serial equivalents. The suffix adds visual noise without preventing any shadowing.
_emit_reload_success and _emit_reload_failed call get_workspace(self._workspace_path), which hits SQLite from the watcher daemon thread. While wrapped in try/except, it introduces DB contention. Since the workspace path is known at construction time, consider passing the workspace in at start() or storing a reference rather than re-fetching on every reload event.
ConfigReloadState.reload_timestamps grows without bound. For multi-hour batches with frequent config changes or a flapping config file, this list could accumulate indefinitely. Consider capping at a reasonable limit (e.g. 100 most recent). MINOR
Python caches module imports so this is not a perf issue, but it reads oddly. Move the import above the loop. What is working well:
The OSError gap in the newly-created file detection (point 1) is the only change I would call required before merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@codeframe/core/conductor.py`:
- Line 1641: The standalone expression completed_count + failed_count +
blocked_count is dead code; follow the same fix used in _execute_serial by
computing and using the total instead of discarding it. Replace the expression
with an assignment like finished_count = completed_count + failed_count +
blocked_count and then either return finished_count from the enclosing function
or use finished_count in the subsequent logic where the total is needed; do not
leave the sum as an unused expression.
- Line 1459: There is a dead expression computing completed_count + failed_count
+ blocked_count that isn’t assigned or used; remove that standalone expression
or, if the intent was to capture the total, replace it with an assignment like
total_count = completed_count + failed_count + blocked_count and use total_count
where needed (or return/log it). Locate the orphan expression referencing
completed_count, failed_count and blocked_count in the conductor code and either
delete it or convert it into a used variable/return value so the computed sum is
not discarded.
- Around line 1308-1332: The function _apply_pending_config_reload currently
only records reloads in batch.results and logs them; update it to emit audit
events by importing and using the event API (e.g., CONFIG_RELOADED and
CONFIG_RELOAD_FAILED plus the emitter function from core.events). Wrap the
has_reloaded_since/check-and-append logic in a try/except: on success emit
CONFIG_RELOADED with a payload containing identifying info (workspace id/name,
batch id, and the ISO timestamp 'now'), then save the batch and return now; on
exception emit CONFIG_RELOAD_FAILED with workspace/batch ids and the error
details, ensure the error is logged, and return last_seen_reload (or rethrow if
that matches project convention). Reference symbols:
_apply_pending_config_reload, reload_state.has_reloaded_since, CONFIG_RELOADED,
CONFIG_RELOAD_FAILED.
In `@codeframe/core/config_watcher.py`:
- Around line 122-127: The watcher currently only snapshots workspace-level
files and ignores inherited files/deletions, causing missed reloads; update
_snapshot_watched_files() to mirror load_preferences() by building the full
effective config set (repo-local files plus parent-directory traversals and the
user-level ~/.codeframe/AGENTS.md/CODEFRAME.md/CLAUDE.md used by
load_preferences()), populate self._watched_mtimes with entries for every path
in that set (use None for missing files) instead of only existing workspace
files, and change the change-detection logic that compares against
self._watched_mtimes to treat a transition between None and a real mtime (or
differing mtimes) as a trigger to call _reload(); ensure names referenced are
_snapshot_watched_files(), load_preferences(), _watched_mtimes, and _reload so
the watcher and loader remain in sync.
- Around line 122-127: The loop that populates self._watched_mtimes for names in
_CONFIG_FILES inside start() and the "new file" detection that assigns
self._watched_mtimes[path] = path.stat().st_mtime in the watch/poll loop must
guard against a race where the file is removed between is_file() and stat();
wrap the path.stat() call in a try/except FileNotFoundError (mirroring the
existing try/except used earlier around stat in the poll logic) and skip/ignore
that file when the exception occurs (optionally log at debug), ensuring the
daemon thread doesn't crash.
- Around line 50-74: The class exposes mutable internals: get_prefs returns a
live AgentPreferences and public fields reload_timestamps, last_error,
last_reload_at are mutable; change get_prefs to return a defensive copy (e.g.,
copy.deepcopy(self._prefs) or an immutable snapshot) and stop exposing mutable
lists directly by making reload_timestamps private and returning a copy (e.g.,
list(self._reload_timestamps) or tuple) from an accessor; ensure apply_reload
updates the private storage (_prefs, _reload_timestamps) and
record_error/has_reloaded_since continue to operate on private fields while
external readers only get immutable or copied snapshots to avoid cross-thread
mutation.
In `@tests/core/test_config_reload_integration.py`:
- Around line 21-25: The workspace fixture currently uses the real HOME so
global ~/.codeframe/AGENTS.md can bleed into tests; update the fixture to be
hermetic by accepting the pytest monkeypatch fixture (def workspace(tmp_path,
monkeypatch):), then set monkeypatch.setenv("HOME", str(tmp_path)) (and
optionally monkeypatch.setenv("XDG_CONFIG_HOME", str(tmp_path)) to be safe)
before calling create_or_load_workspace(repo_path) so load_preferences() will
only see the empty temp location instead of the developer/CI home; keep the rest
of the fixture (repo_path.mkdir() and return
create_or_load_workspace(repo_path)) the same.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a791b88-fce7-4851-8e8d-73d9a263d73b
📒 Files selected for processing (7)
codeframe/cli/app.pycodeframe/core/conductor.pycodeframe/core/config_watcher.pycodeframe/core/events.pytests/cli/test_config_reload_cli.pytests/core/test_config_reload_integration.pytests/core/test_config_watcher.py
| def _apply_pending_config_reload( | ||
| batch: BatchRun, | ||
| workspace: Workspace, | ||
| reload_state: "ConfigReloadState", | ||
| last_seen_reload: datetime, | ||
| ) -> datetime: | ||
| """Check for config reloads and record them in batch results. | ||
|
|
||
| Args: | ||
| batch: Current batch run. | ||
| workspace: Target workspace. | ||
| reload_state: Shared reload state from ConfigFileWatcher. | ||
| last_seen_reload: Timestamp of the last check. | ||
|
|
||
| Returns: | ||
| Updated last_seen_reload timestamp. | ||
| """ | ||
| if reload_state.has_reloaded_since(last_seen_reload): | ||
| now = datetime.now(timezone.utc) | ||
| reloads = batch.results.setdefault("__config_reloads__", []) | ||
| reloads.append(now.isoformat()) | ||
| _save_batch(workspace, batch) | ||
| print(f" [config] Configuration reloaded at {now.strftime('%H:%M:%S')}") | ||
| return now | ||
| return last_seen_reload |
There was a problem hiding this comment.
Missing CONFIG_RELOADED event emission.
The PR objectives state that CONFIG_RELOADED and CONFIG_RELOAD_FAILED events should be emitted, but this function only appends to batch results and prints a log message without emitting any events.
As per coding guidelines: "Core modules in codeframe/core/ must emit events for state transitions via core/events.py for audit and observability."
🛠️ Proposed fix to emit CONFIG_RELOADED event
def _apply_pending_config_reload(
batch: BatchRun,
workspace: Workspace,
reload_state: "ConfigReloadState",
last_seen_reload: datetime,
) -> datetime:
"""Check for config reloads and record them in batch results.
Args:
batch: Current batch run.
workspace: Target workspace.
reload_state: Shared reload state from ConfigFileWatcher.
last_seen_reload: Timestamp of the last check.
Returns:
Updated last_seen_reload timestamp.
"""
if reload_state.has_reloaded_since(last_seen_reload):
now = datetime.now(timezone.utc)
reloads = batch.results.setdefault("__config_reloads__", [])
reloads.append(now.isoformat())
_save_batch(workspace, batch)
print(f" [config] Configuration reloaded at {now.strftime('%H:%M:%S')}")
+ events.emit_for_workspace(
+ workspace,
+ events.EventType.CONFIG_RELOADED,
+ {"batch_id": batch.id, "timestamp": now.isoformat()},
+ print_event=False,
+ )
return now
return last_seen_reload📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _apply_pending_config_reload( | |
| batch: BatchRun, | |
| workspace: Workspace, | |
| reload_state: "ConfigReloadState", | |
| last_seen_reload: datetime, | |
| ) -> datetime: | |
| """Check for config reloads and record them in batch results. | |
| Args: | |
| batch: Current batch run. | |
| workspace: Target workspace. | |
| reload_state: Shared reload state from ConfigFileWatcher. | |
| last_seen_reload: Timestamp of the last check. | |
| Returns: | |
| Updated last_seen_reload timestamp. | |
| """ | |
| if reload_state.has_reloaded_since(last_seen_reload): | |
| now = datetime.now(timezone.utc) | |
| reloads = batch.results.setdefault("__config_reloads__", []) | |
| reloads.append(now.isoformat()) | |
| _save_batch(workspace, batch) | |
| print(f" [config] Configuration reloaded at {now.strftime('%H:%M:%S')}") | |
| return now | |
| return last_seen_reload | |
| def _apply_pending_config_reload( | |
| batch: BatchRun, | |
| workspace: Workspace, | |
| reload_state: "ConfigReloadState", | |
| last_seen_reload: datetime, | |
| ) -> datetime: | |
| """Check for config reloads and record them in batch results. | |
| Args: | |
| batch: Current batch run. | |
| workspace: Target workspace. | |
| reload_state: Shared reload state from ConfigFileWatcher. | |
| last_seen_reload: Timestamp of the last check. | |
| Returns: | |
| Updated last_seen_reload timestamp. | |
| """ | |
| if reload_state.has_reloaded_since(last_seen_reload): | |
| now = datetime.now(timezone.utc) | |
| reloads = batch.results.setdefault("__config_reloads__", []) | |
| reloads.append(now.isoformat()) | |
| _save_batch(workspace, batch) | |
| print(f" [config] Configuration reloaded at {now.strftime('%H:%M:%S')}") | |
| events.emit_for_workspace( | |
| workspace, | |
| events.EventType.CONFIG_RELOADED, | |
| {"batch_id": batch.id, "timestamp": now.isoformat()}, | |
| print_event=False, | |
| ) | |
| return now | |
| return last_seen_reload |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/conductor.py` around lines 1308 - 1332, The function
_apply_pending_config_reload currently only records reloads in batch.results and
logs them; update it to emit audit events by importing and using the event API
(e.g., CONFIG_RELOADED and CONFIG_RELOAD_FAILED plus the emitter function from
core.events). Wrap the has_reloaded_since/check-and-append logic in a
try/except: on success emit CONFIG_RELOADED with a payload containing
identifying info (workspace id/name, batch id, and the ISO timestamp 'now'),
then save the batch and return now; on exception emit CONFIG_RELOAD_FAILED with
workspace/batch ids and the error details, ensure the error is logged, and
return last_seen_reload (or rethrow if that matches project convention).
Reference symbols: _apply_pending_config_reload,
reload_state.has_reloaded_since, CONFIG_RELOADED, CONFIG_RELOAD_FAILED.
|
|
||
| # Determine final batch status | ||
| total = len(batch.task_ids) | ||
| completed_count + failed_count + blocked_count |
There was a problem hiding this comment.
Dead code: expression computed but never used.
This line computes a sum but doesn't assign or use the result. Likely a leftover or debugging artifact.
🔧 Proposed fix
- completed_count + failed_count + blocked_count📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| completed_count + failed_count + blocked_count |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/conductor.py` at line 1459, There is a dead expression
computing completed_count + failed_count + blocked_count that isn’t assigned or
used; remove that standalone expression or, if the intent was to capture the
total, replace it with an assignment like total_count = completed_count +
failed_count + blocked_count and use total_count where needed (or return/log
it). Locate the orphan expression referencing completed_count, failed_count and
blocked_count in the conductor code and either delete it or convert it into a
used variable/return value so the computed sum is not discarded.
|
|
||
| # Determine final batch status | ||
| total = len(batch.task_ids) | ||
| completed_count + failed_count + blocked_count |
There was a problem hiding this comment.
Dead code: same issue as in _execute_serial.
Expression computed but result is discarded.
🔧 Proposed fix
- completed_count + failed_count + blocked_count📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| completed_count + failed_count + blocked_count |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/conductor.py` at line 1641, The standalone expression
completed_count + failed_count + blocked_count is dead code; follow the same fix
used in _execute_serial by computing and using the total instead of discarding
it. Replace the expression with an assignment like finished_count =
completed_count + failed_count + blocked_count and then either return
finished_count from the enclosing function or use finished_count in the
subsequent logic where the total is needed; do not leave the sum as an unused
expression.
| def __init__(self, initial_prefs: AgentPreferences) -> None: | ||
| self._prefs = initial_prefs | ||
| self._lock = threading.Lock() | ||
| self.last_reload_at: Optional[datetime] = None | ||
| self.last_error: Optional[str] = None | ||
| self.reload_timestamps: list[str] = [] | ||
|
|
||
| def get_prefs(self) -> AgentPreferences: | ||
| with self._lock: | ||
| return self._prefs | ||
|
|
||
| def apply_reload(self, new_prefs: AgentPreferences, timestamp: datetime) -> None: | ||
| with self._lock: | ||
| self._prefs = new_prefs | ||
| self.last_reload_at = timestamp | ||
| self.last_error = None | ||
| self.reload_timestamps.append(timestamp.isoformat()) | ||
|
|
||
| def record_error(self, msg: str) -> None: | ||
| with self._lock: | ||
| self.last_error = msg | ||
|
|
||
| def has_reloaded_since(self, since: datetime) -> bool: | ||
| with self._lock: | ||
| return self.last_reload_at is not None and self.last_reload_at > since |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
nl -ba codeframe/core/config_watcher.py | sed -n '50,74p'
printf '\n'
nl -ba codeframe/core/agents_config.py | sed -n '31,60p'Repository: frankbria/codeframe
Length of output: 144
🏁 Script executed:
cat -n codeframe/core/config_watcher.py | sed -n '50,74p'Repository: frankbria/codeframe
Length of output: 1192
🏁 Script executed:
fd -t f -n "agents_config.py" codeframe/core/Repository: frankbria/codeframe
Length of output: 293
🏁 Script executed:
fd -t f -n "*.py" codeframe/core/ | grep -E "(config|preference)" | head -20Repository: frankbria/codeframe
Length of output: 293
🏁 Script executed:
rg -A 10 "class AgentPreferences" codeframe/core/Repository: frankbria/codeframe
Length of output: 1002
🏁 Script executed:
rg "get_prefs()" codeframe/ -A 3 -B 1Repository: frankbria/codeframe
Length of output: 1072
🏁 Script executed:
rg "class AgentPreferences" codeframe/core/ -A 30Repository: frankbria/codeframe
Length of output: 2406
🏁 Script executed:
rg "always_do|ask_first|never_do" codeframe/core/agents_config.py -B 2 -A 2Repository: frankbria/codeframe
Length of output: 3273
🏁 Script executed:
rg "get_prefs()" codeframe/ -B 2 -A 5 | head -60Repository: frankbria/codeframe
Length of output: 1637
🏁 Script executed:
rg "\.always_do|\.ask_first|\.never_do" codeframe/ -A 2Repository: frankbria/codeframe
Length of output: 3238
ConfigReloadState leaks mutable state across threads without proper encapsulation.
get_prefs() returns a live reference to the internal AgentPreferences object, which contains mutable lists (always_do, ask_first, never_do, source_files) and dicts (tooling, code_style, commands). After the lock is released, callers can freely mutate these collections outside any synchronization. Additionally, public fields like reload_timestamps (a mutable list), last_error, and last_reload_at are directly accessible and modifiable. Thread-safety here relies only on caller convention, not enforcement. Return defensive copies via copy.deepcopy() or a frozen snapshot object to prevent accidental mutations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/config_watcher.py` around lines 50 - 74, The class exposes
mutable internals: get_prefs returns a live AgentPreferences and public fields
reload_timestamps, last_error, last_reload_at are mutable; change get_prefs to
return a defensive copy (e.g., copy.deepcopy(self._prefs) or an immutable
snapshot) and stop exposing mutable lists directly by making reload_timestamps
private and returning a copy (e.g., list(self._reload_timestamps) or tuple) from
an accessor; ensure apply_reload updates the private storage (_prefs,
_reload_timestamps) and record_error/has_reloaded_since continue to operate on
private fields while external readers only get immutable or copied snapshots to
avoid cross-thread mutation.
| # Snapshot current mtimes | ||
| self._watched_mtimes = {} | ||
| for name in _CONFIG_FILES: | ||
| path = self._workspace_path / name | ||
| if path.is_file(): | ||
| self._watched_mtimes[path] = path.stat().st_mtime |
There was a problem hiding this comment.
Watch the full effective config set, including deletions.
load_preferences() merges repo-local files, parent-directory files, and ~/.codeframe/AGENTS.md, but this watcher only snapshots workspace_path / {CODEFRAME, AGENTS, CLAUDE}.md and explicitly treats deleted files as a no-op. Editing or removing an inherited config therefore won't trigger _reload(), so later tasks can keep running with stale preferences until some repo-local file changes.
💡 Fix direction
- self._watched_mtimes = {}
- for name in _CONFIG_FILES:
- path = self._workspace_path / name
- if path.is_file():
- self._watched_mtimes[path] = path.stat().st_mtime
+ self._watched_mtimes = self._snapshot_watched_files()
...
- changed = False
- ...
- if changed:
+ current_mtimes = self._snapshot_watched_files()
+ changed = current_mtimes != self._watched_mtimes
+ self._watched_mtimes = current_mtimes
+ if changed:
self._reload()_snapshot_watched_files() should mirror codeframe/core/agents_config.py:load_preferences() so the watcher and loader stay in sync.
Also applies to: 153-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/config_watcher.py` around lines 122 - 127, The watcher
currently only snapshots workspace-level files and ignores inherited
files/deletions, causing missed reloads; update _snapshot_watched_files() to
mirror load_preferences() by building the full effective config set (repo-local
files plus parent-directory traversals and the user-level
~/.codeframe/AGENTS.md/CODEFRAME.md/CLAUDE.md used by load_preferences()),
populate self._watched_mtimes with entries for every path in that set (use None
for missing files) instead of only existing workspace files, and change the
change-detection logic that compares against self._watched_mtimes to treat a
transition between None and a real mtime (or differing mtimes) as a trigger to
call _reload(); ensure names referenced are _snapshot_watched_files(),
load_preferences(), _watched_mtimes, and _reload so the watcher and loader
remain in sync.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
nl -ba codeframe/core/config_watcher.py | sed -n '122,127p;172,176p'Repository: frankbria/codeframe
Length of output: 103
🏁 Script executed:
# First, let's check the file size and read the target sections
wc -l codeframe/core/config_watcher.pyRepository: frankbria/codeframe
Length of output: 99
🏁 Script executed:
# Read lines 122-127 and surrounding context to understand the code
cat -n codeframe/core/config_watcher.py | sed -n '115,135p'Repository: frankbria/codeframe
Length of output: 854
🏁 Script executed:
# Read lines 173-176 and surrounding context
cat -n codeframe/core/config_watcher.py | sed -n '165,185p'Repository: frankbria/codeframe
Length of output: 943
🏁 Script executed:
# Search for _poll_loop method and its exception handling
rg -n "_poll_loop|except|FileNotFoundError" codeframe/core/config_watcher.py -A 2Repository: frankbria/codeframe
Length of output: 803
🏁 Script executed:
# Check the entire file structure to understand class and method organization
cat -n codeframe/core/config_watcher.py | grep -E "^\s*(def|class)" | head -20Repository: frankbria/codeframe
Length of output: 45
🏁 Script executed:
# Read the complete _poll_loop method to see exception handling scope
cat -n codeframe/core/config_watcher.py | sed -n '144,180p'Repository: frankbria/codeframe
Length of output: 1647
🏁 Script executed:
# Verify FileNotFoundError is OSError subclass and check if there's any outer exception handling
python3 << 'EOF'
# FileNotFoundError is a subclass of OSError in Python 3.3+
print("FileNotFoundError is OSError:", issubclass(FileNotFoundError, OSError))
EOFRepository: frankbria/codeframe
Length of output: 97
🏁 Script executed:
# Double-check by looking for any try-except around _check_for_changes call
cat -n codeframe/core/config_watcher.py | sed -n '144,148p'Repository: frankbria/codeframe
Length of output: 311
Add exception handling for the file stat() race at lines 122-127 and 173-176.
Race condition: editors that save via atomic rename can delete the file between the is_file() check and the stat() call, raising FileNotFoundError. The first loop (lines 153–170) protects against this with a try-except at lines 162-165, but the "new file" detection loop (lines 173-176) does not. When stat() fails at line 176, the unhandled exception kills the daemon thread silently, stopping hot-reload for the rest of the polling cycle.
Wrap the stat() call at line 176 in a try-except block (similar to lines 162-165), and add the same protection at line 127 in the start() method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/core/config_watcher.py` around lines 122 - 127, The loop that
populates self._watched_mtimes for names in _CONFIG_FILES inside start() and the
"new file" detection that assigns self._watched_mtimes[path] =
path.stat().st_mtime in the watch/poll loop must guard against a race where the
file is removed between is_file() and stat(); wrap the path.stat() call in a
try/except FileNotFoundError (mirroring the existing try/except used earlier
around stat in the poll logic) and skip/ignore that file when the exception
occurs (optionally log at debug), ensuring the daemon thread doesn't crash.
| @pytest.fixture | ||
| def workspace(tmp_path: Path): | ||
| repo_path = tmp_path / "test_repo" | ||
| repo_path.mkdir() | ||
| return create_or_load_workspace(repo_path) |
There was a problem hiding this comment.
Make this fixture hermetic to global CodeFRAME defaults.
load_preferences() also merges ~/.codeframe/AGENTS.md, but this fixture uses the runner's real home directory. Any developer/CI machine with global defaults can change the merged preferences and make these assertions nondeterministic. Patch the home directory (or the global-config lookup) to an empty temp location before the tests run. The same setup pattern appears in tests/core/test_config_watcher.py.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/test_config_reload_integration.py` around lines 21 - 25, The
workspace fixture currently uses the real HOME so global ~/.codeframe/AGENTS.md
can bleed into tests; update the fixture to be hermetic by accepting the pytest
monkeypatch fixture (def workspace(tmp_path, monkeypatch):), then set
monkeypatch.setenv("HOME", str(tmp_path)) (and optionally
monkeypatch.setenv("XDG_CONFIG_HOME", str(tmp_path)) to be safe) before calling
create_or_load_workspace(repo_path) so load_preferences() will only see the
empty temp location instead of the developer/CI home; keep the rest of the
fixture (repo_path.mkdir() and return create_or_load_workspace(repo_path)) the
same.
…orkspace (#402) - Wrap newly-created file detection in try/except OSError - Cap reload_timestamps at 100 entries - Cache workspace reference to avoid DB access on every emit - Move import outside loop in batch_status
There was a problem hiding this comment.
♻️ Duplicate comments (3)
codeframe/core/config_watcher.py (3)
127-131:⚠️ Potential issue | 🟠 MajorGuard startup mtime snapshot against TOCTOU race.
At Line [130],
path.stat()can raise afterpath.is_file()if the file is atomically replaced/deleted during startup, which can fail watcher initialization.Suggested fix
for name in _CONFIG_FILES: path = self._workspace_path / name if path.is_file(): - self._watched_mtimes[path] = path.stat().st_mtime + try: + self._watched_mtimes[path] = path.stat().st_mtime + except OSError: + continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/config_watcher.py` around lines 127 - 131, The startup mtime snapshot loop using _CONFIG_FILES currently calls path.is_file() then path.stat(), which can raise if the file is removed/replace between those calls; update the loop in config_watcher.py (the block that populates self._watched_mtimes) to guard the stat with a try/except (catch FileNotFoundError/OSError) and skip the path when stat fails, so you only add entries when path.stat() succeeds; reference _CONFIG_FILES, path.is_file(), path.stat().st_mtime and self._watched_mtimes when making the change.
53-56:⚠️ Potential issue | 🟠 Major
ConfigReloadStatestill leaks mutable cross-thread state.Line [57] returns a live
AgentPreferencesobject, and Lines [53]-[56] expose mutable/public state. External callers can mutate shared state outside the lock.Suggested hardening
+import copy ... class ConfigReloadState: def __init__(self, initial_prefs: AgentPreferences) -> None: self._prefs = initial_prefs self._lock = threading.Lock() - self.last_reload_at: Optional[datetime] = None - self.last_error: Optional[str] = None - self.reload_timestamps: list[str] = [] + self._last_reload_at: Optional[datetime] = None + self._last_error: Optional[str] = None + self._reload_timestamps: list[str] = [] def get_prefs(self) -> AgentPreferences: with self._lock: - return self._prefs + return copy.deepcopy(self._prefs) + + def get_last_reload_at(self) -> Optional[datetime]: + with self._lock: + return self._last_reload_at + + def get_last_error(self) -> Optional[str]: + with self._lock: + return self._last_error + + def get_reload_timestamps(self) -> tuple[str, ...]: + with self._lock: + return tuple(self._reload_timestamps)Also applies to: 57-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/config_watcher.py` around lines 53 - 56, ConfigReloadState currently exposes mutable cross-thread state (attributes last_reload_at, last_error, reload_timestamps and the AgentPreferences object returned live), so callers can mutate shared state outside the lock; fix by making internal state private and only returning immutable copies under the lock: e.g., rename fields to _last_reload_at, _last_error, _reload_timestamps and store reload_timestamps as a tuple (or always return a copy), and change the accessor that returns AgentPreferences (look for the method that returns AgentPreferences) to return a deep/shallow copy or an immutable/frozen representation while holding the same lock that protects state. Ensure all external accessors read state under the lock and never expose the live mutable objects directly.
158-163:⚠️ Potential issue | 🟠 MajorDeleted config files never trigger reload.
Lines [162]-[163] skip deletions without marking a change, so removing a watched config file does not call
_reload(), leaving stale settings active.Suggested fix
for name in _CONFIG_FILES: path = self._workspace_path / name if not path.is_file(): # File might have been created since start if path not in self._watched_mtimes: continue - # File was deleted — skip - continue + # File was deleted — trigger reload and drop stale mtime + del self._watched_mtimes[path] + changed = True + continueAlso applies to: 185-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/core/config_watcher.py` around lines 158 - 163, The watcher currently skips deleted files without marking them changed so removals never trigger _reload(); update the branch that handles "if not path.is_file()" to detect deletions by checking if path is present in self._watched_mtimes, and when it is, remove it from self._watched_mtimes (or mark it as changed) and call the reload path (e.g., invoke self._reload() or enqueue the change) so deleted configs trigger a reload; make the same change for the analogous block that checks path existence around the lines referencing self._watched_mtimes (the second occurrence noted) to ensure deletions are handled consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@codeframe/core/config_watcher.py`:
- Around line 127-131: The startup mtime snapshot loop using _CONFIG_FILES
currently calls path.is_file() then path.stat(), which can raise if the file is
removed/replace between those calls; update the loop in config_watcher.py (the
block that populates self._watched_mtimes) to guard the stat with a try/except
(catch FileNotFoundError/OSError) and skip the path when stat fails, so you only
add entries when path.stat() succeeds; reference _CONFIG_FILES, path.is_file(),
path.stat().st_mtime and self._watched_mtimes when making the change.
- Around line 53-56: ConfigReloadState currently exposes mutable cross-thread
state (attributes last_reload_at, last_error, reload_timestamps and the
AgentPreferences object returned live), so callers can mutate shared state
outside the lock; fix by making internal state private and only returning
immutable copies under the lock: e.g., rename fields to _last_reload_at,
_last_error, _reload_timestamps and store reload_timestamps as a tuple (or
always return a copy), and change the accessor that returns AgentPreferences
(look for the method that returns AgentPreferences) to return a deep/shallow
copy or an immutable/frozen representation while holding the same lock that
protects state. Ensure all external accessors read state under the lock and
never expose the live mutable objects directly.
- Around line 158-163: The watcher currently skips deleted files without marking
them changed so removals never trigger _reload(); update the branch that handles
"if not path.is_file()" to detect deletions by checking if path is present in
self._watched_mtimes, and when it is, remove it from self._watched_mtimes (or
mark it as changed) and call the reload path (e.g., invoke self._reload() or
enqueue the change) so deleted configs trigger a reload; make the same change
for the analogous block that checks path existence around the lines referencing
self._watched_mtimes (the second occurrence noted) to ensure deletions are
handled consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90297da6-5c9c-4f22-aeed-f71694b448e3
📒 Files selected for processing (2)
codeframe/cli/app.pycodeframe/core/config_watcher.py
|
Follow-up to my earlier review — checking what the latest commit ( Resolved from prior review
Still open
Summary The architecture is solid and the test coverage across unit/integration/CLI layers is thorough. One required fix remains (OSError gap in point 1). Everything else is polish. |
1 participant
Summary
Implements #402: Hot-reload CODEFRAME.md/AGENTS.md/CLAUDE.md during long-running batch executions without restarting.
_execute_serialand_execute_parallel, reload checked between task dispatchescf work batch statusshows "Config reloaded at HH:MM:SS" when reloads occurredAcceptance Criteria
cf work batch statusshows "config reloaded at HH:MM:SS" if reload occurredTest Plan
Implementation Notes
threading+os.statpolling (matching existing patterns)resultsJSON column via__config_reloads__keyCloses #402
Summary by CodeRabbit
New Features
Tests