mirror of
https://github.com/affaan-m/ECC.git
synced 2026-09-08 07:37:48 +02:00
fix(continuous-learning-v2): warn when the observer never survives a hook invocation (#2489) (#2606)
* fix(continuous-learning-v2): warn when the observer never survives a hook invocation (#2489) The observer is lazy-started from a hook process that exits immediately afterwards. start-observer.sh's liveness check runs inside that still-living process tree, so it always sees a healthy observer and prints "Observer started (PID: N)". On native Windows (Git Bash/MSYS2) the reap happens later, when the hook's Job Object closes, so no self-check placed in start-observer.sh can ever observe the failure. The next hook invocation is the only place the death is visible, and _CHECK_OBSERVER_RUNNING already found it there -- then discarded it, deleting the stale PID file and restarting silently, once per tool call, forever. Users were left with an observer-start.log full of success lines and an observer that never completed a single analysis cycle. Record the "well-formed PID that is no longer alive" case, count consecutive non-survivals in ${PROJECT_DIR}/.observer-nosurvive-count, and log one explanatory warning when the streak reaches ECC_OBSERVER_NOSURVIVE_WARN_AFTER (default 3). Warning fires on equality so a persistent failure logs once per streak rather than once per tool call; finding the observer alive resets the streak. The Windows-specific explanation is gated on uname so Linux/macOS users are pointed at observer.log instead of a wrong diagnosis. Counting happens in the caller, not inside _CHECK_OBSERVER_RUNNING, because that function is invoked once per PID file and again under the start lock. The PowerShell backgrounding rewrite is deliberately not included: it cannot be exercised on a non-Windows machine, and untested process-spawning code is a worse outcome than an accurate diagnostic. * docs(continuous-learning-v2): state observer platform support and the new warn threshold The observer's Windows limitation was only discoverable by hitting it. Record it next to observer.enabled, where it is read before the flag is set, and document ECC_OBSERVER_NOSURVIVE_WARN_AFTER so the knob added alongside the warning does not repeat the undocumented-env-var problem tracked in #2573. zh-TW is intentionally left alone: translation parity is not enforced here and the repo rejects blind translation imports without translator review. * fix(continuous-learning-v2): serialize the non-survival streak under the lazy-start lock observe.sh runs on every tool call, so the streak read-modify-write could race between concurrent invocations -- losing an increment or logging the warning twice. That is the same class of bug the signal counter hit in #2296, and this repo's rule is to never fall back to an unlocked read-modify-write. Rather than add a second lock, move the increment into _START_OBSERVER_LOGGED. All three of its call sites already run inside the lazy-start lock (flock / lockfile / mkdir), so the update is serialized with no new machinery. Counting at the restart instead of at detection also means N racing hooks record one death rather than N. The reset stays in the caller: it is an idempotent unlink, not a read-modify-write, so it needs no lock. Adds a regression case pinning the increment inside _START_OBSERVER_LOGGED and asserting all three call sites remain locked. * fix(continuous-learning-v2): harden the non-survival threshold and warning output Three review findings on the #2489 diagnostic: - An all-zero threshold silently disabled it. `00` passes a digits-only check but compares as zero, and the streak only grows, so the warning could never fire. Normalize with base-10 arithmetic and fall back to the default for anything below 1. Base 10 is forced explicitly because a leading zero would otherwise be read as octal, and `08` is an arithmetic error that would abort the hook under `set -e`. The same normalization now guards the streak read. - An unwritable log silently swallowed the diagnostic. Build the message once and fall back to stderr when the append fails. This cannot spam: the block runs once per streak, not once per tool call. The counter write keeps its `|| true` -- observe.sh runs on every tool call and the repo rule is that hooks exit 0 on non-critical errors, so a full disk must not break tool use. - The live-PID test fixture used process.pid, which is 1 in a container and is deliberately rejected by _CHECK_OBSERVER_RUNNING; the reset case would then fail for the wrong reason. Use a spawned child and clean it up. Adds a regression case for the all-zero threshold. Verified on bash 3.2 (the macOS CI runner shell) as well as bash 5. * fix(continuous-learning-v2): warn only on a persisted streak increment If the counter write fails, the file stays below the threshold, so every later hook invocation rereads it, re-increments in memory, hits the equality check and warns again -- turning the once-per-streak diagnostic into once-per-tool- call spam. That is worse in exactly the case the stderr fallback added in the previous commit was meant to cover, since a disk that cannot take the log usually cannot take the counter either. Gate the warning on the write succeeding. The write stays non-fatal: it runs as an `if` condition, so `set -e` is satisfied and an unwritable counter costs a delayed diagnostic rather than a broken tool call. Tests: an unwritable counter must stay silent across repeated invocations while the hook still exits 0, and a leading-zero threshold ("08") must be read as decimal -- "00" alone did not exercise the base-10 conversion, since it is zero either way. --------- Co-authored-by: haelyra <49814733+haelyra@users.noreply.github.com>
This commit is contained in:
Gaurav Dubey
@@ -238,6 +238,22 @@ Edit `config.json` to control the background observer:
|
||||
|
||||
Other behavior (observation capture, instinct thresholds, project scoping, promotion criteria) is configured via code defaults in `instinct-cli.py` and `observe.sh`.
|
||||
|
||||
### Observer platform support
|
||||
|
||||
The background observer requires WSL2, Linux, or macOS. On native Windows
|
||||
(Git Bash / MSYS2) it starts and reports success, but the process is killed
|
||||
when the spawning hook exits and its Job Object closes, so no analysis ever
|
||||
runs — setting `observer.enabled: true` there is effectively a no-op
|
||||
(see issue #2489).
|
||||
|
||||
`observe.sh` detects this on the following hook invocation and writes an
|
||||
explanatory warning to `observer-start.log` once the observer has failed to
|
||||
survive several times in a row.
|
||||
|
||||
| Env var | Default | Description |
|
||||
|---------|---------|-------------|
|
||||
| `ECC_OBSERVER_NOSURVIVE_WARN_AFTER` | `3` | Consecutive non-survivals before the warning is logged |
|
||||
|
||||
## File Structure
|
||||
|
||||
```
|
||||
|
||||
@@ -375,6 +375,14 @@ _REMOVE_FILE_IF_PRESENT() {
|
||||
_START_OBSERVER_LOGGED() {
|
||||
local bootstrap_log="${PROJECT_DIR}/observer-start.log"
|
||||
mkdir -p "$PROJECT_DIR"
|
||||
# Every call site below sits inside the lazy-start lock (flock / lockfile /
|
||||
# mkdir), so the streak read-modify-write in _NOTE_OBSERVER_NOSURVIVE is
|
||||
# serialized here without a second lock -- concurrent hook invocations cannot
|
||||
# lose an increment or double-log the warning. Counting at the restart (rather
|
||||
# than at detection) also means N racing hooks record one death, not N.
|
||||
if [ "${OBSERVER_DIED:-false}" = "true" ]; then
|
||||
_NOTE_OBSERVER_NOSURVIVE
|
||||
fi
|
||||
"${SKILL_ROOT}/agents/start-observer.sh" start >> "$bootstrap_log" 2>&1 || true
|
||||
}
|
||||
|
||||
@@ -393,12 +401,82 @@ _CHECK_OBSERVER_RUNNING() {
|
||||
if kill -0 "$pid" 2>/dev/null; then
|
||||
return 0 # Process is alive
|
||||
fi
|
||||
# Stale PID file - remove it
|
||||
# Stale PID file - remove it. A well-formed PID that is no longer alive
|
||||
# means an observer we launched has since died, which is the only evidence
|
||||
# of non-survival any process ever sees (#2489). Record it; the caller
|
||||
# decides whether the streak is long enough to warn about.
|
||||
OBSERVER_DIED=true
|
||||
_REMOVE_FILE_IF_PRESENT "$pid_file"
|
||||
fi
|
||||
return 1 # No PID file or process dead
|
||||
}
|
||||
|
||||
# The observer is lazy-started from a hook process that exits immediately after.
|
||||
# start-observer.sh's own liveness check runs inside that still-living process
|
||||
# tree, so it always sees a healthy observer and reports success -- on native
|
||||
# Windows the reap happens later, when the hook's Job Object closes. The next
|
||||
# hook invocation is therefore the only place the death is observable, and
|
||||
# before #2489 it silently deleted the stale PID and restarted, once per tool
|
||||
# call, forever. Warn once per streak so this is signal rather than noise.
|
||||
_NOTE_OBSERVER_NOSURVIVE() {
|
||||
local streak_file="${PROJECT_DIR}/.observer-nosurvive-count"
|
||||
local log_file="${PROJECT_DIR}/observer-start.log"
|
||||
local warn_after="${ECC_OBSERVER_NOSURVIVE_WARN_AFTER:-3}"
|
||||
local streak
|
||||
streak=$(cat "$streak_file" 2>/dev/null || echo 0)
|
||||
# Force base 10 after the digit check: a stray leading zero would otherwise
|
||||
# make bash read the value as octal, and `08` is an arithmetic error that
|
||||
# would abort the whole hook under `set -e`.
|
||||
case "$streak" in ''|*[!0-9]*) streak=0 ;; *) streak=$((10#$streak)) ;; esac
|
||||
# Reject every all-zero spelling, not just the literal `0`: `00` passes a
|
||||
# digits-only check but compares as zero, and since the streak only grows the
|
||||
# threshold could never be reached -- silently disabling the diagnostic.
|
||||
case "$warn_after" in ''|*[!0-9]*) warn_after=3 ;; *) warn_after=$((10#$warn_after)) ;; esac
|
||||
if [ "$warn_after" -lt 1 ]; then warn_after=3; fi
|
||||
streak=$((streak + 1))
|
||||
|
||||
# Warn only on a persisted increment, and only on equality. Both conditions
|
||||
# are what keep this to one warning per streak:
|
||||
# - `-eq` rather than `-ge` stops it repeating once the threshold is passed.
|
||||
# - Requiring the write to succeed stops it repeating when the write fails:
|
||||
# a stuck counter file would otherwise be reread at `warn_after - 1` on
|
||||
# every tool call, re-incremented in memory, and warn every time.
|
||||
# A failed write is still never fatal -- observe.sh runs on every tool call
|
||||
# and the repo rule is that hooks exit 0 on non-critical errors, so a full
|
||||
# disk must not break tool execution. The `if` context keeps `set -e` happy.
|
||||
if printf '%s\n' "$streak" > "$streak_file" 2>/dev/null &&
|
||||
[ "$streak" -eq "$warn_after" ]; then
|
||||
local platform_hint
|
||||
local uname_lower
|
||||
uname_lower=$(uname -s 2>/dev/null | tr '[:upper:]' '[:lower:]')
|
||||
case "$uname_lower" in
|
||||
*mingw*|*msys*|*cygwin*)
|
||||
platform_hint='[observe] On native Windows (Git Bash/MSYS2) this is expected: the background launch does not detach the observer from the hook process Job Object, so it is killed when the hook exits. Run under WSL2, Linux or macOS. See issue #2489.'
|
||||
;;
|
||||
*)
|
||||
platform_hint="[observe] Check ${PROJECT_DIR}/observer.log for the reason the observer exited."
|
||||
;;
|
||||
esac
|
||||
|
||||
local message
|
||||
printf -v message '%s\n%s\n%s\n%s' \
|
||||
"[observe] Observer did not survive to the next hook invocation ${streak} times in a row." \
|
||||
"[observe] Startup reports success, but the process is gone by the following tool call, so no analysis ever runs." \
|
||||
"$platform_hint" \
|
||||
"[observe] Set ECC_OBSERVER_NOSURVIVE_WARN_AFTER to change this threshold (currently ${warn_after})."
|
||||
# An unwritable log must not silently swallow the diagnostic, so fall back
|
||||
# to stderr. Safe from spam: this block runs once per streak, not per call.
|
||||
if ! printf '%s\n' "$message" >> "$log_file" 2>/dev/null; then
|
||||
printf '%s\n' "$message" >&2 2>/dev/null || true
|
||||
fi
|
||||
fi
|
||||
return 0
|
||||
}
|
||||
|
||||
_RESET_OBSERVER_NOSURVIVE_STREAK() {
|
||||
_REMOVE_FILE_IF_PRESENT "${PROJECT_DIR}/.observer-nosurvive-count"
|
||||
}
|
||||
|
||||
if [ -f "${CONFIG_DIR}/disabled" ]; then
|
||||
OBSERVER_ENABLED=false
|
||||
else
|
||||
@@ -427,9 +505,21 @@ fi
|
||||
|
||||
# Check both project-scoped AND global PID files (with stale PID recovery)
|
||||
if [ "$OBSERVER_ENABLED" = "true" ]; then
|
||||
# Clean up stale PID files first
|
||||
_CHECK_OBSERVER_RUNNING "${PROJECT_DIR}/.observer.pid" || true
|
||||
_CHECK_OBSERVER_RUNNING "${CONFIG_DIR}/.observer.pid" || true
|
||||
# Clean up stale PID files first.
|
||||
# `if` context (not `|| true`) so `set -e` stays satisfied while we still
|
||||
# capture whether either PID file pointed at a live observer.
|
||||
OBSERVER_ALIVE=false
|
||||
OBSERVER_DIED=false
|
||||
if _CHECK_OBSERVER_RUNNING "${PROJECT_DIR}/.observer.pid"; then OBSERVER_ALIVE=true; fi
|
||||
if _CHECK_OBSERVER_RUNNING "${CONFIG_DIR}/.observer.pid"; then OBSERVER_ALIVE=true; fi
|
||||
|
||||
# A live observer clears the streak so a later one-off crash does not inherit
|
||||
# an old count. This is an idempotent unlink, not a read-modify-write, so it
|
||||
# needs no lock. The matching increment runs inside the lazy-start lock, in
|
||||
# _START_OBSERVER_LOGGED.
|
||||
if [ "$OBSERVER_ALIVE" = "true" ]; then
|
||||
_RESET_OBSERVER_NOSURVIVE_STREAK
|
||||
fi
|
||||
|
||||
# Check if observer is now running after cleanup
|
||||
if [ ! -f "${PROJECT_DIR}/.observer.pid" ] && [ ! -f "${CONFIG_DIR}/.observer.pid" ]; then
|
||||
|
||||
Reference in New Issue
Block a user