skills/continuous-learning-v2/hooks/observe.sh (1)

17-17: Unconditional debug logging on every hook call will create noisy, unbounded log growth.

These lines run on the hot path and append every invocation. Gate this behind a debug flag (and ideally rotate/size-cap logs).

Proposed fix

+DEBUG_HOOK_LOG="${CLV2_DEBUG_HOOK_LOG:-0}"
+HOOK_LOG_FILE="${HOME}/claude-hooks.log"
+log_debug() {
+  [ "$DEBUG_HOOK_LOG" = "1" ] || return 0
+  printf '%s\n' "$1" >> "$HOOK_LOG_FILE"
+}
-echo "From observe.sh: Hook fired $(date)" >> ~/claude-hooks.log
+log_debug "From observe.sh: Hook fired $(date)"
@@
-echo "From observe.sh: CONFIG_DIR: $CONFIG_DIR" >> ~/claude-hooks.log
-echo "From observe.sh: OBSERVATIONS_FILE: $OBSERVATIONS_FILE" >> ~/claude-hooks.log
-echo "From observe.sh: CLAUDE_PROJECT_DIR: $CLAUDE_PROJECT_DIR" >> ~/claude-hooks.log
-echo "From observe.sh: PYTHON_CMD: $PYTHON_CMD" >> ~/claude-hooks.log
+log_debug "From observe.sh: CONFIG_DIR: $CONFIG_DIR"
+log_debug "From observe.sh: OBSERVATIONS_FILE: $OBSERVATIONS_FILE"
+log_debug "From observe.sh: CLAUDE_PROJECT_DIR: $CLAUDE_PROJECT_DIR"
+log_debug "From observe.sh: PYTHON_CMD: $PYTHON_CMD"
@@
-echo "date: $(date) From observe.sh: PROJECT_DIR: $PROJECT_DIR" >> ~/claude-hooks.log
-echo "date: $(date) From observe.sh: CONFIG_DIR: $CONFIG_DIR" >> ~/claude-hooks.log
+log_debug "date: $(date) From observe.sh: PROJECT_DIR: $PROJECT_DIR"
+log_debug "date: $(date) From observe.sh: CONFIG_DIR: $CONFIG_DIR"

Also applies to: 95-99, 240-242

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@skills/continuous-learning-v2/hooks/observe.sh` at line 17, The script
currently appends an unconditional debug line (echo "From observe.sh: Hook fired
$(date)" >> ~/claude-hooks.log) on every invocation, causing unbounded log
growth; wrap this and the other similar occurrences (around lines 95-99 and
240-242) in a conditional that checks a debug flag (e.g. CLAUDE_HOOKS_DEBUG or
DEBUG_HOOKS) and only writes the log when that env var is set/truthy, and
consider switching to a rotating/size-limited logging mechanism (or use system
logger) instead of blind appends; update the echo statements in observe.sh so
they are executed only when the debug flag is enabled.

skills/continuous-learning-v2/hooks/observe.sh (1)

279-284: Config file existence check doesn't cover CLV2_CONFIG override.

Line 279 verifies $CONFIG_FILE (${SKILL_ROOT}/config.json) exists, but line 282 reads from CLV2_CONFIG if set—which might be a different file that was never validated. Also, open() without a context manager is inconsistent with the pattern used in start-observer.sh:79-92.

Suggested fix

   CONFIG_FILE="${SKILL_ROOT}/config.json"
-  if [ -f "$CONFIG_FILE" ] && [ -n "$PYTHON_CMD" ]; then
+  EFFECTIVE_CONFIG="${CLV2_CONFIG:-$CONFIG_FILE}"
+  if [ -f "$EFFECTIVE_CONFIG" ] && [ -n "$PYTHON_CMD" ]; then
     _enabled=$("$PYTHON_CMD" -c "
 import json, os
-cfg = json.load(open(os.environ.get('CLV2_CONFIG', '$CONFIG_FILE')))
+with open('$EFFECTIVE_CONFIG') as f:
+    cfg = json.load(f)
 print(str(cfg.get('observer', {}).get('enabled', False)).lower())
 " 2>/dev/null || echo "false")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@skills/continuous-learning-v2/hooks/observe.sh` around lines 279 - 284, The
existence check currently only verifies CONFIG_FILE but the Python snippet reads
from CLV2_CONFIG when set, risking reading an unvalidated file; update the logic
that sets _enabled to first determine the actual config path (use
${CLV2_CONFIG:-$CONFIG_FILE}) and verify that file exists before invoking
PYTHON_CMD, and modify the Python one-liner to open the file using a context
manager (with open(... ) as f) and load JSON from that file; keep the existing
fallback to "false" and references to PYTHON_CMD and _enabled unchanged.

skills/continuous-learning-v2/hooks/observe.sh (1)

298-307: ⚠️ Potential issue | 🟡 Minor

Race condition is fixed, but stale PID files still prevent observer restart.

The flock implementation properly addresses the concurrency race. However, lines 299 and 303 only check for PID file existence, not whether the process is actually running. If an observer crashes without cleaning up its PID file, lazy-start will be skipped forever.

Consider adding stale PID cleanup inside the critical section:

🛡️ Proposed fix to clean stale PIDs before starting

 if [ "$OBSERVER_ENABLED" = "true" ] && [ ! -f "${PROJECT_DIR}/.observer.pid" ] && [ ! -f "${CONFIG_DIR}/.observer.pid" ]; then
   (
     flock -n 9 || exit 0
     # Double-check PID files after acquiring lock
-    if [ ! -f "${PROJECT_DIR}/.observer.pid" ] && [ ! -f "${CONFIG_DIR}/.observer.pid" ]; then
+    # Clean up stale PID files first
+    for _pid_file in "${PROJECT_DIR}/.observer.pid" "${CONFIG_DIR}/.observer.pid"; do
+      if [ -f "$_pid_file" ]; then
+        _existing_pid="$(cat "$_pid_file" 2>/dev/null || true)"
+        if [ -n "$_existing_pid" ] && ! kill -0 "$_existing_pid" 2>/dev/null; then
+          rm -f "$_pid_file"
+        fi
+      fi
+    done
+    # Now check if we should start
+    if [ ! -f "${PROJECT_DIR}/.observer.pid" ] && [ ! -f "${CONFIG_DIR}/.observer.pid" ]; then
       "${SKILL_ROOT}/agents/start-observer.sh" start 2>/dev/null || true
     fi
   ) 9>"$LAZY_START_LOCK"
 fi

Note: The initial check at line 299 must remain as-is (checking file existence) since it determines whether to attempt lock acquisition at all. The stale cleanup happens only after acquiring the lock.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@skills/continuous-learning-v2/hooks/observe.sh` around lines 298 - 307, The
current lazy-start uses flock but only checks for PID file existence; inside the
critical section (the block using 9>"$LAZY_START_LOCK") add logic to detect and
remove stale PID files before launching the observer: after acquiring the lock
and before calling "${SKILL_ROOT}/agents/start-observer.sh", read PID(s) from
"${PROJECT_DIR}/.observer.pid" and "${CONFIG_DIR}/.observer.pid" (if present)
and use a safe liveness check (e.g., kill -0 $PID) to determine if the PID is
dead; remove the stale PID file(s) if the process is not running, then re-check
existence and only call start-observer.sh if no live PID remains—leave the outer
pre-check that gates lock acquisition unchanged.

skills/continuous-learning-v2/hooks/observe.sh (2)

292-303: ⚠️ Potential issue | 🟠 Major

disabled sentinel check still diverges from CLV2_CONFIG path resolution.

In this block, EFFECTIVE_CONFIG is used for enabled-state reading, but the disabled check is still tied to ${CONFIG_DIR}/disabled (hardcoded base dir). If CLV2_CONFIG points elsewhere, disable behavior can still diverge from config behavior.

Proposed fix

-if [ -f "${CONFIG_DIR}/disabled" ]; then
+EFFECTIVE_CONFIG="${CLV2_CONFIG:-${SKILL_ROOT}/config.json}"
+EFFECTIVE_CONFIG_DIR="$(dirname "$EFFECTIVE_CONFIG")"
+if [ -f "${EFFECTIVE_CONFIG_DIR}/disabled" ]; then
   OBSERVER_ENABLED=false
 else
   OBSERVER_ENABLED=false
-  CONFIG_FILE="${SKILL_ROOT}/config.json"
-  # Allow CLV2_CONFIG override
-  if [ -n "${CLV2_CONFIG:-}" ]; then
-    CONFIG_FILE="$CLV2_CONFIG"
-  fi
-  # Use effective config path for both existence check and reading
-  EFFECTIVE_CONFIG="$CONFIG_FILE"
+  # Use effective config path for both existence check and reading
   if [ -f "$EFFECTIVE_CONFIG" ] && [ -n "$PYTHON_CMD" ]; then

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@skills/continuous-learning-v2/hooks/observe.sh` around lines 292 - 303, The
current disabled-sentinel check uses ${CONFIG_DIR}/disabled while the script
resolves CONFIG_FILE/EFFECTIVE_CONFIG (including CLV2_CONFIG override) later,
causing divergence; move the EFFECTIVE_CONFIG/CONFIG_FILE resolution above the
disabled check and then test for a sentinel in the same directory as
EFFECTIVE_CONFIG (e.g., check "$(dirname "$EFFECTIVE_CONFIG")/disabled"), and
set OBSERVER_ENABLED=false if that sentinel exists; update references to
OBSERVER_ENABLED, CONFIG_FILE, CLV2_CONFIG, and EFFECTIVE_CONFIG accordingly so
the disable behavior follows the resolved config path.

---

331-333: ⚠️ Potential issue | 🟠 Major

Lock is released before observer startup is confirmed, so duplicate starts remain possible.

Line 332 and Line 342 run start-observer.sh in background inside the lock. The lock can be released before .observer.pid is created, allowing a concurrent hook to start another observer.

Proposed fix

-          nohup "${SKILL_ROOT}/agents/start-observer.sh" start >/dev/null 2>&1 &
+          "${SKILL_ROOT}/agents/start-observer.sh" start >/dev/null 2>&1 || true
@@
-          nohup "${SKILL_ROOT}/agents/start-observer.sh" start >/dev/null 2>&1 &
+          "${SKILL_ROOT}/agents/start-observer.sh" start >/dev/null 2>&1 || true

Also applies to: 341-343

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@skills/continuous-learning-v2/hooks/observe.sh` around lines 331 - 333, The
lock in observe.sh is released before the observer process startup is confirmed,
allowing a race that can spawn duplicate observers; modify the block that
currently runs nohup "${SKILL_ROOT}/agents/start-observer.sh" start >/dev/null
2>&1 & so that either (a) start-observer.sh is invoked synchronously while
holding the lock until it creates .observer.pid, or (b) after launching the
background process you poll for the creation of ${PROJECT_DIR}/.observer.pid and
${CONFIG_DIR}/.observer.pid with a short timeout before releasing the lock;
ensure the lock is only released once the pid file exists and reference the
exact invocation of start-observer.sh and the .observer.pid files to locate and
update the code path.

skills/continuous-learning-v2/hooks/observe.sh (3)

292-303: ⚠️ Potential issue | 🟠 Major

Propagate CLV2_CONFIG consistently across disable checks, PID lookups, and startup.

Lines 296-302 only switch the observer.enabled read. CONFIG_DIR still stays on ${HOME}/.claude/homunculus, so the disabled sentinel and global PID checks keep using the default directory, and skills/continuous-learning-v2/agents/start-observer.sh still resolves its own default config file. With a custom CLV2_CONFIG, lazy-start can read one config but signal/start a different observer.

♻️ Suggested direction

-  CONFIG_FILE="${SKILL_ROOT}/config.json"
-  # Allow CLV2_CONFIG override
-  if [ -n "${CLV2_CONFIG:-}" ]; then
-    CONFIG_FILE="$CLV2_CONFIG"
-  fi
-  # Use effective config path for both existence check and reading
-  EFFECTIVE_CONFIG="$CONFIG_FILE"
+  DEFAULT_CONFIG_FILE="${SKILL_ROOT}/config.json"
+  EFFECTIVE_CONFIG="${CLV2_CONFIG:-$DEFAULT_CONFIG_FILE}"
+  CONFIG_DIR="$(dirname "$EFFECTIVE_CONFIG")"

Apply the same effective config resolution to the earlier disabled guard and to skills/continuous-learning-v2/agents/start-observer.sh.

Also applies to: 316-323, 332-345

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@skills/continuous-learning-v2/hooks/observe.sh` around lines 292 - 303, The
script computes EFFECTIVE_CONFIG too late so checks for the disabled sentinel
and PID/start logic still use the default CONFIG_DIR and CONFIG_FILE; move or
duplicate the CLV2_CONFIG override and EFFECTIVE_CONFIG resolution earlier and
use EFFECTIVE_CONFIG (not CONFIG_DIR or the original CONFIG_FILE) for the
disabled file check, PID lookups, OBSERVER_ENABLED decisions, and when invoking
skills/continuous-learning-v2/agents/start-observer.sh; also update
start-observer.sh to accept/resolve CLV2_CONFIG or an explicit config path so
the started observer uses the same EFFECTIVE_CONFIG value.

---

331-345: ⚠️ Potential issue | 🟠 Major

Keep the startup lock until the observer PID is visible.

Line 332 and Line 345 background start-observer.sh, but skills/continuous-learning-v2/agents/start-observer.sh:195-211 already daemonizes and returns before skills/continuous-learning-v2/agents/observer-loop.sh:137-138 writes the PID file. That leaves a window where the lock is gone and the next hook can start a second observer.

🔒 Suggested fix

-          nohup "${SKILL_ROOT}/agents/start-observer.sh" start >/dev/null 2>&1 &
+          "${SKILL_ROOT}/agents/start-observer.sh" start >/dev/null 2>&1 || true
+          for _ in 1 2 3 4 5; do
+            _CHECK_OBSERVER_RUNNING "${PROJECT_DIR}/.observer.pid" && break
+            _CHECK_OBSERVER_RUNNING "${CONFIG_DIR}/.observer.pid" && break
+            sleep 1
+          done

Apply the same change in the macOS fallback branch.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@skills/continuous-learning-v2/hooks/observe.sh` around lines 331 - 345, The
macOS fallback branch starts start-observer.sh in background and releases
LAZY_START_LOCK too early; modify the lockfile branch (inside the subshell where
lockfile -r 1 -l 30 "$LAZY_START_LOCK" is used) to mirror the Linux branch
behavior: after launching nohup "${SKILL_ROOT}/agents/start-observer.sh" start
>/dev/null 2>&1 &, do a short polling loop (with a reasonable timeout) that
checks for either "${PROJECT_DIR}/.observer.pid" or
"${CONFIG_DIR}/.observer.pid" (reuse _CHECK_OBSERVER_RUNNING or plain -f checks)
and only allow the subshell to exit (and thus remove the LAZY_START_LOCK via the
existing EXIT trap) once one of those PID files is present or the timeout
elapses; keep the existing trap and ensure the lock is still held during the
wait so a second observer cannot be started.

---

278-289: ⚠️ Potential issue | 🔴 Critical

Reject unreadable or malformed PID files before calling kill.

Line 282 and Line 358 can fail under set -e if another hook removes the file after the -f check, and the current [ -n "$pid" ] gate still lets values like -1, 0, or abc reach kill. Treat unreadable or non-numeric PIDs as stale in both paths.

🛡️ Suggested fix

 _CHECK_OBSERVER_RUNNING() {
   local pid_file="$1"
   if [ -f "$pid_file" ]; then
     local pid
-    pid=$(cat "$pid_file" 2>/dev/null)
-    if [ -n "$pid" ] && kill -0 "$pid" 2>/dev/null; then
+    pid=$(cat "$pid_file" 2>/dev/null || true)
+    case "$pid" in
+      ''|*[!0-9]*|0|1)
+        rm -f "$pid_file" 2>/dev/null || true
+        return 1
+        ;;
+    esac
+    if kill -0 "$pid" 2>/dev/null; then
       return 0  # Process is alive
     fi
     # Stale PID file - remove it
     rm -f "$pid_file" 2>/dev/null || true
   fi
@@
 for pid_file in "${PROJECT_DIR}/.observer.pid" "${CONFIG_DIR}/.observer.pid"; do
   if [ -f "$pid_file" ]; then
-    observer_pid=$(cat "$pid_file")
+    observer_pid=$(cat "$pid_file" 2>/dev/null || true)
+    case "$observer_pid" in
+      ''|*[!0-9]*|0|1)
+        rm -f "$pid_file" 2>/dev/null || true
+        continue
+        ;;
+    esac
     # Deduplicate: skip if already signaled this pass
     case "$signaled_pids" in
       *" $observer_pid "*) continue ;;
     esac

Also applies to: 356-365

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@skills/continuous-learning-v2/hooks/observe.sh` around lines 278 - 289, The
PID handling in _CHECK_OBSERVER_RUNNING allows unreadable or malformed values to
reach kill and can fail under set -e if the file is removed between the -f check
and reading; replace the current cat/kill flow with a safe read and numeric
validation: use read -r pid < "$pid_file" 2>/dev/null (or a subshell read) and
if the read fails treat the PID file as stale and rm -f it, then validate pid
with a regex like ^[0-9]+$ and ensure pid is greater than 0 before calling kill
-0; if validation fails remove the pid file and return 1. Apply the same robust
read+numeric check logic to the other similar block referenced (the logic around
lines handling PID file at the other path / function).