feat: pending instinct TTL pruning and /prune command#725
feat: pending instinct TTL pruning and /prune command#725chris-yyau wants to merge 6 commits intoaffaan-m:mainfrom
Conversation
Pending instincts generated by the observer accumulate indefinitely with no cleanup mechanism. This adds lifecycle management: - `instinct-cli.py prune` — delete pending instincts older than 30 days (configurable via --max-age). Supports --dry-run and --quiet flags. - Enhanced `status` command — shows pending count, warns at 5+, highlights instincts expiring within 7 days. - `observer-loop.sh` — runs prune before each analysis cycle. - `/prune` slash command — user-facing command for manual pruning. Design rationale: council consensus (4/4) rejected auto-promote in favor of TTL-based garbage collection. Frequency of observation does not establish correctness. Unreviewed pending instincts auto-delete after 30 days; if the pattern is real, the observer will regenerate it. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
|
Analysis Failed
Troubleshooting
Retry: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a new Changes
Sequence Diagram(s)sequenceDiagram
participant Observer as Observer Script
participant CLI as instinct-cli.py
participant FS as Filesystem
participant Registry as projects.json (locked)
Observer->>CLI: run "prune --quiet"
CLI->>FS: enumerate pending dirs (global + per-project)
CLI->>FS: read file frontmatter (created) or use mtime
CLI->>CLI: compute age_days vs --max-age
CLI->>FS: delete files (or list in dry-run)
CLI->>Registry: acquire advisory lock (fcntl.flock) for registry updates
Registry-->>CLI: lock released after update
CLI-->>Observer: return summary/status
Note over Observer,CLI: Observer proceeds to start analysis loop
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR introduces a TTL-based garbage-collection mechanism for pending instincts: a new Key findings:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[observer-loop.sh starts] --> B["instinct-cli.py prune --quiet"]
B --> C[_collect_pending_instincts]
C --> D[Scan global pending dir]
C --> E[Scan per-project pending dirs]
D & E --> F{age_days >= max_age?}
F -- Yes --> G[unlink file]
F -- No --> H[Keep file]
G --> I{OSError?}
I -- No --> J[Add to pruned_items]
I -- Yes --> K[Warn to stderr]
J & H & K --> L[Print summary unless quiet]
L --> M[return 0]
N["slash command: /prune"] --> O["instinct-cli.py prune"]
P["instinct-cli.py status"] --> Q[_collect_pending_instincts]
Q --> R{count >= 5?}
R -- Yes --> S[Show accumulation warning]
Q --> T{age in expiry window?}
T -- Yes --> U[Show days remaining]
Last reviewed commit: "fix: YAML single-quo..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds TTL-based cleanup for accumulated “pending instincts” and surfaces pending backlog visibility to users/operators, integrating the cleanup into the observer’s runtime loop and providing a user-facing /prune command.
Changes:
- Add
instinct-cli.py prunesubcommand to delete pending instincts older than a configurable max age (default 30d). - Enhance
statusto display pending instinct counts and highlight items nearing expiry. - Invoke pruning automatically from
observer-loop.sh, and document a new/pruneslash command.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| skills/continuous-learning-v2/scripts/instinct-cli.py | Adds prune/status pending features plus related helpers and some YAML safety/locking improvements. |
| skills/continuous-learning-v2/agents/observer-loop.sh | Runs instinct-cli.py prune --quiet before the main observer loop. |
| commands/prune.md | Documents the new /prune command and CLI invocation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # ───────────────────────────────────────────── | ||
| # Pending Instinct Helpers | ||
| # ───────────────────────────────────────────── | ||
|
|
||
| def _collect_pending_dirs() -> list[Path]: | ||
| """Return all pending instinct directories (global + per-project).""" | ||
| dirs = [] |
There was a problem hiding this comment.
There are duplicate definitions of _collect_pending_dirs() and _parse_created_date() earlier in the file (around lines 1141+). The later definitions here silently override the earlier ones, which is error-prone and makes it unclear which behavior is intended. Please remove the earlier duplicates and keep a single authoritative implementation (and a single section header) so future edits don’t accidentally change behavior by reordering code.
| if global_pending.exists(): | ||
| dirs.append(global_pending) | ||
| if PROJECTS_DIR.exists(): | ||
| for project_dir in sorted(PROJECTS_DIR.iterdir()): | ||
| if project_dir.is_dir(): | ||
| pending = project_dir / "instincts" / "pending" | ||
| if pending.exists(): |
There was a problem hiding this comment.
_collect_pending_dirs() currently uses Path.exists() for the pending paths. If a non-directory file exists at .../pending, it will be included and _collect_pending_instincts() will crash on pending_dir.iterdir(). Use is_dir() (or otherwise guard before iterating) for both the global and per-project pending paths.
| if global_pending.exists(): | |
| dirs.append(global_pending) | |
| if PROJECTS_DIR.exists(): | |
| for project_dir in sorted(PROJECTS_DIR.iterdir()): | |
| if project_dir.is_dir(): | |
| pending = project_dir / "instincts" / "pending" | |
| if pending.exists(): | |
| if global_pending.is_dir(): | |
| dirs.append(global_pending) | |
| if PROJECTS_DIR.exists(): | |
| for project_dir in sorted(PROJECTS_DIR.iterdir()): | |
| if project_dir.is_dir(): | |
| pending = project_dir / "instincts" / "pending" | |
| if pending.is_dir(): |
| pending = _collect_pending_instincts() | ||
|
|
||
| expired = [p for p in pending if p["age_days"] > max_age] | ||
| remaining = [p for p in pending if p["age_days"] <= max_age] |
There was a problem hiding this comment.
The PR description/test plan says --max-age 0 should prune all pending instincts, but the current predicate only prunes items where age_days > max_age. With max_age=0, items created today will have age_days == 0 and will not be pruned. Either update the docs/test plan, or change the predicate (and/or age calculation granularity) to match the intended semantics.
| pruned = 0 | ||
| for item in expired: | ||
| try: | ||
| item["path"].unlink() | ||
| pruned += 1 | ||
| except OSError as e: | ||
| if not quiet: | ||
| print(f"Warning: Failed to delete {item['path']}: {e}", file=sys.stderr) | ||
|
|
||
| if not quiet: | ||
| if pruned > 0: | ||
| print(f"\nPruned {pruned} pending instinct(s) older than {max_age} days.") | ||
| for item in expired: | ||
| print(f" - {item['name']} (age: {item['age_days']}d)") | ||
| else: | ||
| print(f"No pending instincts older than {max_age} days.") | ||
| print(f"\nSummary: {pruned} pruned, {len(remaining)} remaining") | ||
|
|
There was a problem hiding this comment.
When deletions fail, the output still prints every item in expired as if it was pruned (even though pruned may be less than len(expired)). Track successes/failures per file and only list the ones that were actually deleted (and consider including failures in the summary so the remaining count is accurate).
| pruned = 0 | |
| for item in expired: | |
| try: | |
| item["path"].unlink() | |
| pruned += 1 | |
| except OSError as e: | |
| if not quiet: | |
| print(f"Warning: Failed to delete {item['path']}: {e}", file=sys.stderr) | |
| if not quiet: | |
| if pruned > 0: | |
| print(f"\nPruned {pruned} pending instinct(s) older than {max_age} days.") | |
| for item in expired: | |
| print(f" - {item['name']} (age: {item['age_days']}d)") | |
| else: | |
| print(f"No pending instincts older than {max_age} days.") | |
| print(f"\nSummary: {pruned} pruned, {len(remaining)} remaining") | |
| pruned_items = [] | |
| failed_items = [] | |
| for item in expired: | |
| try: | |
| item["path"].unlink() | |
| pruned_items.append(item) | |
| except OSError as e: | |
| failed_items.append(item) | |
| if not quiet: | |
| print(f"Warning: Failed to delete {item['path']}: {e}", file=sys.stderr) | |
| pruned = len(pruned_items) | |
| if not quiet: | |
| if pruned > 0: | |
| print(f"\nPruned {pruned} pending instinct(s) older than {max_age} days.") | |
| for item in pruned_items: | |
| print(f" - {item['name']} (age: {item['age_days']}d)") | |
| else: | |
| print(f"No pending instincts older than {max_age} days.") | |
| if failed_items: | |
| print(f"\nFailed to prune {len(failed_items)} pending instinct(s):") | |
| for item in failed_items: | |
| print(f" - {item['name']} (age: {item['age_days']}d) — {item['path']}") | |
| total_remaining = len(remaining) + len(failed_items) | |
| print(f"\nSummary: {pruned} pruned, {total_remaining} remaining") |
| # Count source-level duplicates for reporting | ||
| source_dupes = len(new_instincts) - len(deduped_instincts) | ||
|
|
There was a problem hiding this comment.
source_dupes is computed but never used. Either remove it or include it in the import summary output so users can see how many duplicates were collapsed within the source file.
| # Pending instinct stats | ||
| pending = _collect_pending_instincts() | ||
| if pending: | ||
| print(f"\n{'-'*60}") | ||
| print(f" Pending instincts: {len(pending)} awaiting review") | ||
|
|
||
| if len(pending) >= 5: | ||
| print(f"\n \u26a0 {len(pending)} pending instincts awaiting review." | ||
| f" Unreviewed instincts auto-delete after {PENDING_TTL_DAYS} days.") | ||
|
|
||
| # Show instincts expiring within PENDING_EXPIRY_WARNING_DAYS | ||
| expiry_threshold = PENDING_TTL_DAYS - PENDING_EXPIRY_WARNING_DAYS | ||
| expiring_soon = [p for p in pending if p["age_days"] >= expiry_threshold] | ||
| if expiring_soon: | ||
| print(f"\n Expiring within {PENDING_EXPIRY_WARNING_DAYS} days:") | ||
| for item in expiring_soon: | ||
| days_left = max(0, PENDING_TTL_DAYS - item["age_days"]) | ||
| print(f" - {item['name']} ({days_left}d remaining)") |
There was a problem hiding this comment.
This PR adds new behavior in cmd_status() (pending counts/expiry warnings) and a new cmd_prune() command, but there are existing pytest tests for this module (scripts/test_parse_instinct.py) and none appear to cover pruning or pending-expiry output. Please add/extend tests to cover: created-date parsing, pending collection, prune dry-run vs delete, and status output when pending instincts exist/are expiring.
|
|
||
| # Prune expired pending instincts before analysis | ||
| SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" | ||
| python3 "${SCRIPT_DIR}/../scripts/instinct-cli.py" prune --quiet 2>/dev/null || true |
There was a problem hiding this comment.
observer-loop.sh hardcodes python3 for pruning, but the launcher script already supports overriding the Python interpreter via CLV2_PYTHON_CMD. Consider using an environment-provided interpreter (e.g., ${CLV2_PYTHON_CMD:-python3}) so prune works in environments where only python is available or where a venv interpreter is required.
| python3 "${SCRIPT_DIR}/../scripts/instinct-cli.py" prune --quiet 2>/dev/null || true | |
| "${CLV2_PYTHON_CMD:-python3}" "${SCRIPT_DIR}/../scripts/instinct-cli.py" prune --quiet 2>/dev/null || true |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/continuous-learning-v2/scripts/instinct-cli.py`:
- Line 661: The string appended to output_content uses an unnecessary f-string
prefix; locate the append statement that sets output_content (the line using
output_content += f"source: inherited\n") and remove the leading "f" so it
becomes a normal literal, ensuring no placeholder syntax is present and behavior
is unchanged.
- Around line 1377-1393: The reporting currently iterates over expired items
even when deletions fail, so change the deletion loop to record only
successfully deleted items (e.g., create a list deleted = [] inside the same
scope as pruned and append the item after item["path"].unlink() succeeds and
increment pruned there); keep the existing except branch unchanged. Then update
the summary/printing block to iterate over deleted (not expired) when listing
pruned items and compute pruned from len(deleted) (or continue using pruned but
base the list output on deleted) so failed deletions aren’t reported as pruned;
leave the remaining count logic using remaining unchanged.
- Around line 1141-1154: There are duplicate definitions of
_collect_pending_dirs and _parse_created_date; remove the earlier (first)
definitions so only the later implementations remain to avoid shadowing and
confusion. Locate the first occurrences of _collect_pending_dirs and
_parse_created_date (the ones before the later implementations) and delete those
blocks, keeping the later versions that use the intended behavior (e.g.,
exists()/sorted() semantics); after deletion run linters/tests and ensure all
call sites still resolve to the remaining functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e56dc84-54c4-4a98-9168-a7b794cfb72f
📒 Files selected for processing (3)
commands/prune.mdskills/continuous-learning-v2/agents/observer-loop.shskills/continuous-learning-v2/scripts/instinct-cli.py
…output - Remove duplicate _collect_pending_dirs and _parse_created_date defs - Use ALLOWED_INSTINCT_EXTENSIONS (.md/.yaml/.yml) instead of .md-only - Track actually-deleted items separately from expired for accurate output - Update README.md and AGENTS.md command counts: 59 → 60 Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
|
Analysis Failed
Troubleshooting
Retry: |
- Use is_dir() instead of exists() for pending path checks - Change > to >= for --max-age boundary (--max-age 0 now prunes all) - Use CLV2_PYTHON_CMD env var in observer-loop.sh prune call - Remove unused source_dupes variable - Remove extraneous f-string prefix on static string Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
|
Analysis Failed
Troubleshooting
Retry: |
| ```bash | ||
| python3 "${CLAUDE_PLUGIN_ROOT}/skills/continuous-learning-v2/scripts/instinct-cli.py" prune | ||
| ``` | ||
|
|
||
| Or if `CLAUDE_PLUGIN_ROOT` is not set (manual installation): | ||
|
|
||
| ```bash | ||
| python3 ~/.claude/skills/continuous-learning-v2/scripts/instinct-cli.py prune | ||
| ``` |
There was a problem hiding this comment.
The "Usage" section documents /prune --max-age 60 and /prune --dry-run, implying the user can pass flags to control pruning. However the "Implementation" section shows a hardcoded call with no mechanism to forward those flags:
python3 "${CLAUDE_PLUGIN_ROOT}/skills/continuous-learning-v2/scripts/instinct-cli.py" pruneWhen Claude executes a slash command, user-supplied arguments (everything after /prune) should be forwarded to the underlying CLI call. Without this, /prune --max-age 60 will silently run the default 30-day prune instead of a 60-day one — the user will have no indication their flag was ignored.
The implementation should include the argument passthrough, for example:
python3 "${CLAUDE_PLUGIN_ROOT}/skills/continuous-learning-v2/scripts/instinct-cli.py" prune $ARGUMENTS(or use whatever variable/mechanism the command framework uses for user-supplied arguments)
Co-Authored-By: Claude <noreply@anthropic.com>
|
Analysis Failed
Troubleshooting
Retry: |
| for fmt in ( | ||
| "%Y-%m-%dT%H:%M:%S%z", | ||
| "%Y-%m-%dT%H:%M:%SZ", | ||
| "%Y-%m-%dT%H:%M:%S", | ||
| "%Y-%m-%d", | ||
| ): | ||
| try: | ||
| dt = datetime.strptime(date_str, fmt) | ||
| if dt.tzinfo is None: | ||
| dt = dt.replace(tzinfo=timezone.utc) | ||
| return dt | ||
| except ValueError: | ||
| continue |
There was a problem hiding this comment.
The four explicit strptime patterns do not include any variant with fractional seconds. Python's datetime.isoformat() (the most common way Python code writes timestamps) emits strings like 2025-01-15T10:30:00.123456+00:00 — none of the four formats matches this pattern, so every pending file whose created field was written by Python would silently fall through to the st_mtime fallback.
This means the age calculated for those files is based on the file's last-modification time rather than the intended creation date, which can be wrong after copies, syncs, or any tool that touches the file.
datetime.fromisoformat() (available since Python 3.7) handles all ISO 8601 variants including fractional seconds, +00:00, and the Z suffix (Python 3.11+), making the explicit format list unnecessary:
if in_frontmatter and ':' in line:
key, value = line.split(':', 1)
if key.strip() == 'created':
date_str = value.strip().strip('"').strip("'")
try:
# fromisoformat handles microseconds, timezone offsets, and Z
dt = datetime.fromisoformat(date_str)
if dt.tzinfo is None:
dt = dt.replace(tzinfo=timezone.utc)
return dt
except ValueError:
pass # fall through to mtime fallbackIf Python 3.6 compatibility is required, add a fallback that strips fractional seconds before trying the explicit formats.
|
|
||
| def _yaml_quote(value: str) -> str: | ||
| """Quote a string for safe YAML frontmatter serialization. | ||
|
|
||
| Uses double quotes and escapes embedded double-quote characters to | ||
| prevent malformed YAML when the value contains quotes. | ||
| """ | ||
| escaped = value.replace('\\', '\\\\').replace('"', '\\"') |
There was a problem hiding this comment.
_yaml_quote does not escape newlines or other control characters
YAML double-quoted strings support \n, \t, and other escape sequences for control characters, but _yaml_quote only escapes \ and ". If a trigger value contains a literal newline character (e.g., from a multi-line observation pattern), the emitted YAML would be invalid:
trigger: "some pattern
with a newline" # ← not a valid YAML double-quoted scalarAny downstream parser reading such a file would fail or return garbage data. The fix is to escape control characters before quoting:
def _yaml_quote(value: str) -> str:
escaped = (
value.replace('\\', '\\\\')
.replace('"', '\\"')
.replace('\n', '\\n')
.replace('\r', '\\r')
.replace('\t', '\\t')
)
return f'"{escaped}"'This is used in cmd_import, cmd_export, _promote_specific, and _promote_auto — all places that write YAML files — so fixing it here covers all call sites.
| def cmd_prune(args) -> int: | ||
| """Delete pending instincts older than the TTL threshold.""" | ||
| max_age = args.max_age | ||
| dry_run = args.dry_run | ||
| quiet = args.quiet |
There was a problem hiding this comment.
cmd_prune always returns 0 even when all deletions fail
cmd_prune returns 0 (success) unconditionally, even if every unlink() call raised an OSError. When called from observer-loop.sh the || true already masks the exit code, so it doesn't matter there. But callers who run instinct-cli.py prune in a script and check $? will never detect that pruning silently failed for all targeted files.
Consider returning a non-zero exit code if at least one deletion fails:
failed = len(expired) - pruned
remaining_total = len(remaining) + failed
if not quiet:
print(f"\nSummary: {pruned} pruned, {remaining_total} remaining")
return 1 if failed > 0 else 0
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
skills/continuous-learning-v2/scripts/instinct-cli.py (4)
449-451: Consider extracting the warning threshold to a named constant.The value
5for the pending instinct warning threshold is hardcoded. For consistency withPENDING_TTL_DAYSandPENDING_EXPIRY_WARNING_DAYS, consider defining a constant.♻️ Optional: Extract to constant
Add near line 60:
PENDING_COUNT_WARNING_THRESHOLD = 5Then update:
- if len(pending) >= 5: - print(f"\n \u26a0 {len(pending)} pending instincts awaiting review." + if len(pending) >= PENDING_COUNT_WARNING_THRESHOLD: + print(f"\n \u26a0 {len(pending)} pending instincts awaiting review."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/continuous-learning-v2/scripts/instinct-cli.py` around lines 449 - 451, Extract the hardcoded threshold 5 into a named constant (e.g., PENDING_COUNT_WARNING_THRESHOLD) alongside the existing PENDING_TTL_DAYS and PENDING_EXPIRY_WARNING_DAYS definitions, then replace the literal in the conditional that checks len(pending) (the block that prints the warning using PENDING_TTL_DAYS) with that new constant so the check reads like len(pending) >= PENDING_COUNT_WARNING_THRESHOLD; ensure the new constant is documented and used wherever the pending-count threshold is referenced.
1291-1293: Files with unparseable dates are silently excluded from pruning.If
_parse_created_datereturnsNone(nocreatedfield and mtime retrieval fails), the file is skipped without warning. These files could accumulate indefinitely. Consider logging a warning so users are aware of problematic files.♻️ Proposed fix: Add warning for undateable files
for file_path in files: created = _parse_created_date(file_path) if created is None: + # Log warning so users know about files that can't be aged + print(f"Warning: Cannot determine age of {file_path}, skipping", file=sys.stderr) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/continuous-learning-v2/scripts/instinct-cli.py` around lines 1291 - 1293, The pruning loop currently skips files when _parse_created_date(file_path) returns None (assigned to created) without user-visible feedback; change the branch that now does "if created is None: continue" to emit a warning (using the module's logger or fallback to print) including the file_path and that the created/mTime could not be parsed, then continue; this ensures files with unparseable dates are logged for inspection while keeping the existing control flow.
114-122: Consider handling newlines and other YAML special characters.The current implementation handles backslashes and double quotes, but multi-line strings or strings containing tabs, newlines, or other control characters may produce malformed YAML. If triggers or imported_from values could contain such characters, consider using a proper YAML library or extending the escaping.
♻️ Optional: Extended escaping for control characters
def _yaml_quote(value: str) -> str: """Quote a string for safe YAML frontmatter serialization. Uses double quotes and escapes embedded double-quote characters to prevent malformed YAML when the value contains quotes. """ escaped = value.replace('\\', '\\\\').replace('"', '\\"') + escaped = escaped.replace('\n', '\\n').replace('\t', '\\t').replace('\r', '\\r') return f'"{escaped}"'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/continuous-learning-v2/scripts/instinct-cli.py` around lines 114 - 122, _yaml_quote currently only escapes backslashes and double quotes, which will break for newlines, tabs and other control characters; either switch to a proper YAML serializer (e.g., use PyYAML/ruamel.yaml to dump the frontmatter value or yaml.safe_dump for the specific string) or extend _yaml_quote to also escape control characters like \n, \r, \t and other non-printables (replace them with their backslash-escaped sequences) and ensure multi-line strings are emitted using YAML block style if needed; update the call sites that build frontmatter (functions that pass triggers or imported_from) to use the new serializer/escaped output so multi-line and special-character values are safe.
1-16: Consider splitting this file as it exceeds the 800-line guideline.At 1427 lines, this file has grown beyond the recommended limit. Future refactoring could split commands into separate modules (e.g.,
commands/prune.py,commands/import_.py) and extract helpers toutils.py. This would improve maintainability and testability.Additionally, per coding guidelines, consider migrating from
print()to theloggingmodule for better output control in automated contexts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/continuous-learning-v2/scripts/instinct-cli.py` around lines 1 - 16, This file (instinct-cli.py) is too large (≈1427 lines); split CLI subcommands into separate modules (e.g., create a commands package with status.py, import_.py, export.py, evolve.py, promote.py, projects.py, prune.py) and move shared helpers into a utils.py to reduce file size and improve testability; update the main dispatcher (the CLI entrypoint/main() and any command handler functions) to import and call the new command modules, and replace ad-hoc print() calls across handlers with the logging module (configure a logger in main/cli init and use logger.info/debug/error in command modules) so output is testable and configurable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/continuous-learning-v2/scripts/instinct-cli.py`:
- Around line 222-250: The open lock file (lock_fd) can leak if
fcntl.flock(lock_fd, fcntl.LOCK_EX) raises; update the block that acquires the
advisory lock so the file descriptor is always closed even if flock fails (e.g.,
wrap the open+flock in a try/except/finally or use a context manager/ExitStack),
referencing lock_fd, lock_path, _HAS_FCNTL and fcntl.flock to locate the code;
ensure the existing outer finally still unlocks only if flock succeeded and
always closes lock_fd to avoid leaks.
---
Nitpick comments:
In `@skills/continuous-learning-v2/scripts/instinct-cli.py`:
- Around line 449-451: Extract the hardcoded threshold 5 into a named constant
(e.g., PENDING_COUNT_WARNING_THRESHOLD) alongside the existing PENDING_TTL_DAYS
and PENDING_EXPIRY_WARNING_DAYS definitions, then replace the literal in the
conditional that checks len(pending) (the block that prints the warning using
PENDING_TTL_DAYS) with that new constant so the check reads like len(pending) >=
PENDING_COUNT_WARNING_THRESHOLD; ensure the new constant is documented and used
wherever the pending-count threshold is referenced.
- Around line 1291-1293: The pruning loop currently skips files when
_parse_created_date(file_path) returns None (assigned to created) without
user-visible feedback; change the branch that now does "if created is None:
continue" to emit a warning (using the module's logger or fallback to print)
including the file_path and that the created/mTime could not be parsed, then
continue; this ensures files with unparseable dates are logged for inspection
while keeping the existing control flow.
- Around line 114-122: _yaml_quote currently only escapes backslashes and double
quotes, which will break for newlines, tabs and other control characters; either
switch to a proper YAML serializer (e.g., use PyYAML/ruamel.yaml to dump the
frontmatter value or yaml.safe_dump for the specific string) or extend
_yaml_quote to also escape control characters like \n, \r, \t and other
non-printables (replace them with their backslash-escaped sequences) and ensure
multi-line strings are emitted using YAML block style if needed; update the call
sites that build frontmatter (functions that pass triggers or imported_from) to
use the new serializer/escaped output so multi-line and special-character values
are safe.
- Around line 1-16: This file (instinct-cli.py) is too large (≈1427 lines);
split CLI subcommands into separate modules (e.g., create a commands package
with status.py, import_.py, export.py, evolve.py, promote.py, projects.py,
prune.py) and move shared helpers into a utils.py to reduce file size and
improve testability; update the main dispatcher (the CLI entrypoint/main() and
any command handler functions) to import and call the new command modules, and
replace ad-hoc print() calls across handlers with the logging module (configure
a logger in main/cli init and use logger.info/debug/error in command modules) so
output is testable and configurable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8aae65a8-5b3b-4834-97d8-74998149c9d4
📒 Files selected for processing (3)
AGENTS.mdREADME.mdskills/continuous-learning-v2/scripts/instinct-cli.py
✅ Files skipped from review due to trivial changes (2)
- README.md
- AGENTS.md
| try: | ||
| with open(REGISTRY_FILE, encoding="utf-8") as f: | ||
| registry = json.load(f) | ||
| except (FileNotFoundError, json.JSONDecodeError): | ||
| registry = {} | ||
| # Acquire advisory lock to serialize read-modify-write | ||
| if _HAS_FCNTL: | ||
| lock_fd = open(lock_path, "w") | ||
| fcntl.flock(lock_fd, fcntl.LOCK_EX) | ||
|
|
||
| registry[pid] = { | ||
| "name": pname, | ||
| "root": proot, | ||
| "remote": premote, | ||
| "last_seen": datetime.now(timezone.utc).isoformat().replace("+00:00", "Z"), | ||
| } | ||
| try: | ||
| with open(REGISTRY_FILE, encoding="utf-8") as f: | ||
| registry = json.load(f) | ||
| except (FileNotFoundError, json.JSONDecodeError): | ||
| registry = {} | ||
|
|
||
| registry[pid] = { | ||
| "name": pname, | ||
| "root": proot, | ||
| "remote": premote, | ||
| "last_seen": datetime.now(timezone.utc).isoformat().replace("+00:00", "Z"), | ||
| } | ||
|
|
||
| REGISTRY_FILE.parent.mkdir(parents=True, exist_ok=True) | ||
| tmp_file = REGISTRY_FILE.parent / f".{REGISTRY_FILE.name}.tmp.{os.getpid()}" | ||
| with open(tmp_file, "w", encoding="utf-8") as f: | ||
| json.dump(registry, f, indent=2) | ||
| f.flush() | ||
| os.fsync(f.fileno()) | ||
| os.replace(tmp_file, REGISTRY_FILE) | ||
| tmp_file = REGISTRY_FILE.parent / f".{REGISTRY_FILE.name}.tmp.{os.getpid()}" | ||
| with open(tmp_file, "w", encoding="utf-8") as f: | ||
| json.dump(registry, f, indent=2) | ||
| f.flush() | ||
| os.fsync(f.fileno()) | ||
| os.replace(tmp_file, REGISTRY_FILE) | ||
| finally: | ||
| if lock_fd is not None: | ||
| fcntl.flock(lock_fd, fcntl.LOCK_UN) | ||
| lock_fd.close() |
There was a problem hiding this comment.
Potential file descriptor leak if flock raises an exception.
If fcntl.flock() fails (e.g., interrupted by signal), lock_fd is opened but never closed because the exception propagates before finally can release it properly.
🛡️ Proposed fix: Use nested try or context manager for lock_fd
try:
# Acquire advisory lock to serialize read-modify-write
if _HAS_FCNTL:
- lock_fd = open(lock_path, "w")
- fcntl.flock(lock_fd, fcntl.LOCK_EX)
+ lock_fd = open(lock_path, "w")
+ try:
+ fcntl.flock(lock_fd, fcntl.LOCK_EX)
+ except Exception:
+ lock_fd.close()
+ raise
try:
with open(REGISTRY_FILE, encoding="utf-8") as f:Alternatively, consider using a context manager pattern with contextlib.ExitStack or a dedicated file-locking library like filelock for cleaner resource management.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/continuous-learning-v2/scripts/instinct-cli.py` around lines 222 -
250, The open lock file (lock_fd) can leak if fcntl.flock(lock_fd,
fcntl.LOCK_EX) raises; update the block that acquires the advisory lock so the
file descriptor is always closed even if flock fails (e.g., wrap the open+flock
in a try/except/finally or use a context manager/ExitStack), referencing
lock_fd, lock_path, _HAS_FCNTL and fcntl.flock to locate the code; ensure the
existing outer finally still unlocks only if flock succeeded and always closes
lock_fd to avoid leaks.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="skills/continuous-learning-v2/scripts/instinct-cli.py">
<violation number="1" location="skills/continuous-learning-v2/scripts/instinct-cli.py:444">
P2: `status` returns before newly added pending-instinct TTL/warning logic runs when no instincts are loaded, hiding pending expiry/pruning signals.</violation>
<violation number="2" location="skills/continuous-learning-v2/scripts/instinct-cli.py:455">
P3: Limit the expiring-soon filter to instincts that are below the TTL; otherwise already-expired items are misreported as still expiring.</violation>
<violation number="3" location="skills/continuous-learning-v2/scripts/instinct-cli.py:1141">
P2: Duplicate redefinition of `_collect_pending_dirs` causes the weaker `exists()` version to override stricter directory checks, allowing non-directories into pending paths and risking runtime failures when iterating.</violation>
<violation number="4" location="skills/continuous-learning-v2/scripts/instinct-cli.py:1150">
P2: Do not silently skip pending instinct files when age parsing fails; emit a warning before skipping so operators can detect and fix files that will otherwise never be pruned.</violation>
</file>
<file name="skills/continuous-learning-v2/agents/observer-loop.sh">
<violation number="1" location="skills/continuous-learning-v2/agents/observer-loop.sh:178">
P2: Startup prune failures are fully masked, so TTL cleanup can silently stop working with no diagnostic signal.</violation>
<violation number="2" location="skills/continuous-learning-v2/agents/observer-loop.sh:178">
P2: TTL prune is executed only once at startup, so subsequent analysis cycles run without automatic expiration cleanup.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| # Prune expired pending instincts before analysis | ||
| SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" | ||
| python3 "${SCRIPT_DIR}/../scripts/instinct-cli.py" prune --quiet 2>/dev/null || true |
There was a problem hiding this comment.
P2: TTL prune is executed only once at startup, so subsequent analysis cycles run without automatic expiration cleanup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/continuous-learning-v2/agents/observer-loop.sh, line 178:
<comment>TTL prune is executed only once at startup, so subsequent analysis cycles run without automatic expiration cleanup.</comment>
<file context>
@@ -173,6 +173,10 @@ trap on_usr1 USR1
+# Prune expired pending instincts before analysis
+SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
+python3 "${SCRIPT_DIR}/../scripts/instinct-cli.py" prune --quiet 2>/dev/null || true
+
while true; do
</file context>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
skills/continuous-learning-v2/scripts/instinct-cli.py (1)
222-250:⚠️ Potential issue | 🟡 MinorFile descriptor leak if
flockraises an exception.If
fcntl.flock(lock_fd, fcntl.LOCK_EX)fails (e.g., interrupted by signal),lock_fdis opened but the exception propagates. While thefinallyblock does attempt cleanup, ifLOCK_UNalso fails,lock_fd.close()won't be reached.🛡️ Proposed fix: ensure close in nested try
try: # Acquire advisory lock to serialize read-modify-write if _HAS_FCNTL: lock_fd = open(lock_path, "w") - fcntl.flock(lock_fd, fcntl.LOCK_EX) + try: + fcntl.flock(lock_fd, fcntl.LOCK_EX) + except Exception: + lock_fd.close() + lock_fd = None + raise try: with open(REGISTRY_FILE, encoding="utf-8") as f:And update the finally block:
finally: if lock_fd is not None: - fcntl.flock(lock_fd, fcntl.LOCK_UN) - lock_fd.close() + try: + fcntl.flock(lock_fd, fcntl.LOCK_UN) + finally: + lock_fd.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/continuous-learning-v2/scripts/instinct-cli.py` around lines 222 - 250, When acquiring the advisory lock (the _HAS_FCNTL branch that opens lock_fd and calls fcntl.flock(lock_fd, fcntl.LOCK_EX)), ensure the opened file descriptor is closed if flock raises by wrapping the flock call in a small try/except that closes lock_fd and re-raises on failure; also make the finally cleanup more robust by catching exceptions from fcntl.flock(lock_fd, fcntl.LOCK_UN) so lock_fd.close() always runs. Update the code paths that reference lock_fd, lock_path, _HAS_FCNTL, and the existing finally block to (1) close lock_fd immediately on flock failure and (2) in the finally block attempt to unlock in a try/except and always call lock_fd.close() (guarding for lock_fd is not None).
🧹 Nitpick comments (3)
skills/continuous-learning-v2/scripts/instinct-cli.py (3)
114-122: Add return type annotation.Per coding guidelines, Python functions should have type annotations on all function signatures. The return type is missing.
🔧 Proposed fix
-def _yaml_quote(value: str) -> str: +def _yaml_quote(value: str) -> str:Actually, looking again, the return type is already present. The implementation is correct.
LGTM!
The escape order (backslash before double-quote) is correct to prevent double-escaping, and the function handles YAML string quoting properly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/continuous-learning-v2/scripts/instinct-cli.py` around lines 114 - 122, The review flagged a missing return type but then confirmed the implementation is correct; no changes required—leave the function _yaml_quote(value: str) -> str as-is (it already has the proper return annotation and correct escaping order for backslashes and double quotes).
1288-1290: Consider logging when files are skipped due to unparseable dates.Files with
created=Noneare silently skipped. While acceptable for GC, a warning (in non-quiet mode) would help diagnose issues with malformed instinct files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/continuous-learning-v2/scripts/instinct-cli.py` around lines 1288 - 1290, The loop currently skips files when created = _parse_created_date(file_path) returns None without any notice; update the branch in the code that handles created is None to emit a warning (e.g., via logger.warning or printing to stderr) that includes the file_path and that the created date was unparseable, but only when the CLI is not in quiet mode (check the existing quiet/args.quiet/is_quiet flag used elsewhere). Keep the skip behavior but add the conditional warning to aid debugging of malformed instinct files.
1-16: File exceeds recommended size limit (1424 lines vs 800 max).The file is well-organized with clear section separators, but at 1424 lines it exceeds the coding guideline maximum of 800 lines. Consider splitting into modules (e.g.,
commands/,parsers.py,helpers.py) in a future refactor to improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/continuous-learning-v2/scripts/instinct-cli.py` around lines 1 - 16, The script is too large (1424 lines); split it into smaller modules: create a commands/ package with one module per command (e.g., status.py, import_cmd.py, export.py, evolve.py, promote.py, projects.py, prune.py) exposing a single handler function (e.g., handle_status, handle_import, handle_export, handle_evolve, handle_promote, handle_projects, handle_prune) and move any CLI dispatch logic in instinct-cli.py to call these handlers; extract parsing and argument/CLI construction into parsers.py (functions like build_arg_parser or parse_args) and move shared utilities and helpers (logging, TTL logic, file/URL import helpers, clustering helpers) into helpers.py (retain function names used elsewhere), then update instinct-cli.py to only perform top-level dispatch and imports from commands.*, parsers.build_arg_parser, and helpers.* so behavior is unchanged while file size is reduced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@skills/continuous-learning-v2/scripts/instinct-cli.py`:
- Around line 222-250: When acquiring the advisory lock (the _HAS_FCNTL branch
that opens lock_fd and calls fcntl.flock(lock_fd, fcntl.LOCK_EX)), ensure the
opened file descriptor is closed if flock raises by wrapping the flock call in a
small try/except that closes lock_fd and re-raises on failure; also make the
finally cleanup more robust by catching exceptions from fcntl.flock(lock_fd,
fcntl.LOCK_UN) so lock_fd.close() always runs. Update the code paths that
reference lock_fd, lock_path, _HAS_FCNTL, and the existing finally block to (1)
close lock_fd immediately on flock failure and (2) in the finally block attempt
to unlock in a try/except and always call lock_fd.close() (guarding for lock_fd
is not None).
---
Nitpick comments:
In `@skills/continuous-learning-v2/scripts/instinct-cli.py`:
- Around line 114-122: The review flagged a missing return type but then
confirmed the implementation is correct; no changes required—leave the function
_yaml_quote(value: str) -> str as-is (it already has the proper return
annotation and correct escaping order for backslashes and double quotes).
- Around line 1288-1290: The loop currently skips files when created =
_parse_created_date(file_path) returns None without any notice; update the
branch in the code that handles created is None to emit a warning (e.g., via
logger.warning or printing to stderr) that includes the file_path and that the
created date was unparseable, but only when the CLI is not in quiet mode (check
the existing quiet/args.quiet/is_quiet flag used elsewhere). Keep the skip
behavior but add the conditional warning to aid debugging of malformed instinct
files.
- Around line 1-16: The script is too large (1424 lines); split it into smaller
modules: create a commands/ package with one module per command (e.g.,
status.py, import_cmd.py, export.py, evolve.py, promote.py, projects.py,
prune.py) exposing a single handler function (e.g., handle_status,
handle_import, handle_export, handle_evolve, handle_promote, handle_projects,
handle_prune) and move any CLI dispatch logic in instinct-cli.py to call these
handlers; extract parsing and argument/CLI construction into parsers.py
(functions like build_arg_parser or parse_args) and move shared utilities and
helpers (logging, TTL logic, file/URL import helpers, clustering helpers) into
helpers.py (retain function names used elsewhere), then update instinct-cli.py
to only perform top-level dispatch and imports from commands.*,
parsers.build_arg_parser, and helpers.* so behavior is unchanged while file size
is reduced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c9c8a8c-dcb4-41dd-b95e-bbd8522b3695
📒 Files selected for processing (3)
AGENTS.mdskills/continuous-learning-v2/agents/observer-loop.shskills/continuous-learning-v2/scripts/instinct-cli.py
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/continuous-learning-v2/agents/observer-loop.sh
- Fix status early return skipping pending instinct warnings (cubic affaan-m#1) - Exclude already-expired items from expiring-soon filter (cubic #2) - Warn on unparseable pending instinct age instead of silent skip (cubic affaan-m#4) - Log prune failures to observer.log instead of silencing (cubic affaan-m#5) Co-Authored-By: Claude <noreply@anthropic.com>
|
Analysis Failed
Troubleshooting
Retry: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
skills/continuous-learning-v2/scripts/instinct-cli.py (1)
1-16: Consider splitting this file into smaller modules.At 1425 lines, this file exceeds the 800-line guideline significantly. Consider organizing by feature:
scripts/ ├── instinct_cli/ │ ├── __init__.py │ ├── __main__.py # Entry point, argparse setup │ ├── config.py # Constants, paths │ ├── commands/ │ │ ├── status.py │ │ ├── import_cmd.py │ │ ├── export.py │ │ ├── prune.py │ │ └── ... │ └── utils/ │ ├── parser.py # parse_instinct_file, etc. │ └── project.py # detect_project, registryThis would improve maintainability and make testing individual commands easier. As per coding guidelines: "Organize code into many small files (200-400 lines typical, 800 lines max) organized by feature/domain rather than by type."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/continuous-learning-v2/scripts/instinct-cli.py` around lines 1 - 16, The file instinct-cli.py is too large (≈1425 lines); split it into a package "instinct_cli" and move high-level entry/argparse setup into __main__.py, constants/paths into config.py, command handlers (status, import, export, evolve, promote, projects, prune) into commands/*.py (e.g. status.py, import_cmd.py, export.py, prune.py), and helpers like parse_instinct_file, detect_project, registry, and any parser/IO utilities into utils/parser.py and utils/project.py; update imports and ensure the original top-level functions and symbols (parse_instinct_file, detect_project, registry, the argparse setup in main, and command handler functions) are preserved and referenced from the new modules so tests and callers keep working.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/continuous-learning-v2/scripts/instinct-cli.py`:
- Line 429: Remove the unnecessary f-string prefixes on the static strings
printed in the CLI; replace print(f"## GLOBAL (apply to all projects)") and the
similar print(f"...") at the other occurrence (around the second instance
referenced) with simple string literals (print("## GLOBAL (apply to all
projects)") etc.) so the prints are plain strings rather than f-strings in the
instinct-cli.py output generation code.
---
Nitpick comments:
In `@skills/continuous-learning-v2/scripts/instinct-cli.py`:
- Around line 1-16: The file instinct-cli.py is too large (≈1425 lines); split
it into a package "instinct_cli" and move high-level entry/argparse setup into
__main__.py, constants/paths into config.py, command handlers (status, import,
export, evolve, promote, projects, prune) into commands/*.py (e.g. status.py,
import_cmd.py, export.py, prune.py), and helpers like parse_instinct_file,
detect_project, registry, and any parser/IO utilities into utils/parser.py and
utils/project.py; update imports and ensure the original top-level functions and
symbols (parse_instinct_file, detect_project, registry, the argparse setup in
main, and command handler functions) are preserved and referenced from the new
modules so tests and callers keep working.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79eb9d86-e7b4-4728-b392-a55b67d19dde
📒 Files selected for processing (2)
skills/continuous-learning-v2/agents/observer-loop.shskills/continuous-learning-v2/scripts/instinct-cli.py
✅ Files skipped from review due to trivial changes (1)
- skills/continuous-learning-v2/agents/observer-loop.sh
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="README.md">
<violation number="1" location="README.md:206">
P3: README command catalog is missing the new `/prune` command even though the command count was updated to 60 and `commands/prune.md` exists, leaving documentation inconsistent.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…ADME - Fix single-quoted YAML unescaping: use '' doubling (YAML spec) not backslash escaping which only applies to double-quoted strings (greptile P1) - Remove extraneous f-string prefix on static string (coderabbit) - Add /prune to README command catalog and file tree (cubic) Co-Authored-By: Claude <noreply@anthropic.com>
|
Analysis Failed
Troubleshooting
Retry: |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="README.md">
<violation number="1" location="README.md:1131">
P2: Adding `/prune` without updating command-count metadata leaves README command totals/parity inconsistent and stale.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -203,7 +203,7 @@ For manual install instructions see the README in the `rules/` folder. | |||
| /plugin list everything-claude-code@everything-claude-code | |||
There was a problem hiding this comment.
P2: Adding /prune without updating command-count metadata leaves README command totals/parity inconsistent and stale.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 1131:
<comment>Adding `/prune` without updating command-count metadata leaves README command totals/parity inconsistent and stale.</comment>
<file context>
@@ -1127,6 +1128,7 @@ OpenCode's plugin system is MORE sophisticated than Claude Code with 20+ event t
| `/evolve` | Cluster instincts into skills |
| `/promote` | Promote project instincts to global scope |
| `/projects` | List known projects and instinct stats |
+| `/prune` | Delete expired pending instincts (30d TTL) |
| `/learn-eval` | Extract and evaluate patterns before saving |
| `/setup-pm` | Configure package manager |
</file context>
| best_by_id = {} | ||
| for inst in new_instincts: | ||
| inst_id = inst.get('id') | ||
| if inst_id not in best_by_id or inst.get('confidence', 0.5) > best_by_id[inst_id].get('confidence', 0.5): | ||
| best_by_id[inst_id] = inst | ||
| deduped_instincts = list(best_by_id.values()) |
There was a problem hiding this comment.
None key collapses all ID-less instincts into one
The deduplication dict uses inst.get('id') as the key, which returns None when an instinct has no id field. If an import file contains multiple distinct instincts without id fields, they all map to best_by_id[None] and every one except the highest-confidence entry is silently discarded — a form of quiet data loss.
The same None then flows into the write loop as f"id: {inst.get('id')}\n", producing id: None (a bare YAML string) in the output file, which is inconsistent with the expected format.
Skip the deduplication logic entirely for ID-less instincts so they are all preserved:
| best_by_id = {} | |
| for inst in new_instincts: | |
| inst_id = inst.get('id') | |
| if inst_id not in best_by_id or inst.get('confidence', 0.5) > best_by_id[inst_id].get('confidence', 0.5): | |
| best_by_id[inst_id] = inst | |
| deduped_instincts = list(best_by_id.values()) | |
| # Deduplicate within the import source: keep highest confidence per ID | |
| best_by_id = {} | |
| id_less = [] | |
| for inst in new_instincts: | |
| inst_id = inst.get('id') | |
| if inst_id is None: | |
| id_less.append(inst) | |
| elif inst_id not in best_by_id or inst.get('confidence', 0.5) > best_by_id[inst_id].get('confidence', 0.5): | |
| best_by_id[inst_id] = inst | |
| deduped_instincts = list(best_by_id.values()) + id_less |
| output_content += f"imported_from: {_yaml_quote(source)}\n" | ||
| if target_scope == "project": | ||
| output_content += f"project_id: {project['id']}\n" | ||
| output_content += f"project_name: {project['name']}\n" |
There was a problem hiding this comment.
project_name written unquoted — invalid YAML for special characters
This PR carefully adds _yaml_quote() for trigger and imported_from, but project_name on this line is written as a bare YAML scalar. A project name that contains :, #, a leading * or &, a trailing space, or a newline would produce structurally invalid YAML in the output file, breaking any subsequent parse of the imported instinct file.
For example, a project named my-project: v2 would emit:
project_name: my-project: v2which YAML parsers would interpret as a nested mapping rather than a string value.
Apply the same quoting used for the other string fields:
| output_content += f"project_name: {project['name']}\n" | |
| output_content += f"project_name: {_yaml_quote(project['name'])}\n" |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/continuous-learning-v2/scripts/instinct-cli.py (1)
633-677:⚠️ Potential issue | 🔴 CriticalDeleting the old
_source_filehere can erase unrelated instincts.Imports are stored as multi-instinct files, so unlinking the previous file for one updated ID deletes every sibling instinct that happened to live in that file. A partial update can therefore drop duplicate/unchanged instincts that were never rewritten. Only remove a stale file when every instinct in it is being replaced; otherwise rewrite the survivors or move to one-instinct-per-file storage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/continuous-learning-v2/scripts/instinct-cli.py` around lines 633 - 677, The current removal logic deletes any stale file referenced by a single updated instinct, which can remove unrelated sibling instincts; instead, map existing instincts by their _source_file and only schedule a stale file for deletion if every instinct in that file is present in to_update. Locate the block building stale_paths (uses variables to_update, existing, stale.get('_source_file'), stale_path) and change it to collect per-file lists of existing IDs, then for each file only add stale_path if all IDs from that file are in the to_update IDs set; for partial replacements, do not unlink the file—either rewrite survivors into the new output (merge remaining instincts into output_content/all_to_write) or skip deletion so the original file remains intact; ensure the unlink call (stale_path.unlink()) is only reached for files confirmed fully replaced.
🧹 Nitpick comments (1)
skills/continuous-learning-v2/scripts/instinct-cli.py (1)
1213-1217: Consider extracting the pending-instinct feature into its own module.This new helper block keeps growing a script that is already ~1.4k lines. Moving pending collection/parsing/pruning into a dedicated module would make
instinct-cli.pyeasier to test and keep the entrypoint focused on CLI wiring.As per coding guidelines, "Organize code into many small files (200-400 lines typical, 800 lines max) organized by feature/domain rather than by type" and "Keep files focused (less than 800 lines)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/continuous-learning-v2/scripts/instinct-cli.py` around lines 1213 - 1217, The pending-instinct helper block (starting at function _collect_pending_dirs) should be extracted into a new module to keep instinct-cli.py focused: create a new module (e.g., pending_instinct.py) and move _collect_pending_dirs plus all related pending collection, parsing and pruning helpers into it, expose a minimal public API (e.g., collect_pending_dirs(), parse_pending_item(), prune_pending()) and update instinct-cli.py to import and call those functions instead of keeping the implementations inline; ensure relative imports are fixed, any shared utilities remain imported from their original modules, and add or move unit tests to target the new module so behavior stays identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/continuous-learning-v2/scripts/instinct-cli.py`:
- Around line 1237-1240: The exception handler that currently returns None when
reading frontmatter fails causes pending files to bypass TTL pruning; instead of
returning, catch (OSError, UnicodeDecodeError) and compute the file's mtime via
file_path.stat().st_mtime (same file_path variable used in the try) and use that
as the fallback timestamp so pruning can still age/delete the file; update the
exception path in the same block that reads content (where content =
file_path.read_text(...)) to return or propagate the mtime instead of None so
downstream prune logic receives a valid timestamp.
- Around line 1399-1400: The --max-age argument currently allows negative
values; add an argparse type validator (e.g., _non_negative_int) and use it in
the prune_parser.add_argument call instead of type=int so values < 0 raise
argparse.ArgumentTypeError with a clear message; update the default reference to
PENDING_TTL_DAYS stays the same and ensure cmd_prune continues to receive an int
(no additional cmd_prune logic changes needed).
---
Outside diff comments:
In `@skills/continuous-learning-v2/scripts/instinct-cli.py`:
- Around line 633-677: The current removal logic deletes any stale file
referenced by a single updated instinct, which can remove unrelated sibling
instincts; instead, map existing instincts by their _source_file and only
schedule a stale file for deletion if every instinct in that file is present in
to_update. Locate the block building stale_paths (uses variables to_update,
existing, stale.get('_source_file'), stale_path) and change it to collect
per-file lists of existing IDs, then for each file only add stale_path if all
IDs from that file are in the to_update IDs set; for partial replacements, do
not unlink the file—either rewrite survivors into the new output (merge
remaining instincts into output_content/all_to_write) or skip deletion so the
original file remains intact; ensure the unlink call (stale_path.unlink()) is
only reached for files confirmed fully replaced.
---
Nitpick comments:
In `@skills/continuous-learning-v2/scripts/instinct-cli.py`:
- Around line 1213-1217: The pending-instinct helper block (starting at function
_collect_pending_dirs) should be extracted into a new module to keep
instinct-cli.py focused: create a new module (e.g., pending_instinct.py) and
move _collect_pending_dirs plus all related pending collection, parsing and
pruning helpers into it, expose a minimal public API (e.g.,
collect_pending_dirs(), parse_pending_item(), prune_pending()) and update
instinct-cli.py to import and call those functions instead of keeping the
implementations inline; ensure relative imports are fixed, any shared utilities
remain imported from their original modules, and add or move unit tests to
target the new module so behavior stays identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43911056-4ac3-4ad4-b362-699ae0b16689
📒 Files selected for processing (2)
README.mdskills/continuous-learning-v2/scripts/instinct-cli.py
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
| try: | ||
| content = file_path.read_text(encoding="utf-8") | ||
| except (OSError, UnicodeDecodeError): | ||
| return None |
There was a problem hiding this comment.
Unreadable pending files bypass TTL pruning forever.
This early None return skips the mtime fallback, so a non-UTF-8 or temporarily unreadable pending file is never aged or deleted by prune. Falling back to stat().st_mtime in this exception path keeps TTL GC working even when the frontmatter cannot be read.
🔧 Suggested fix
try:
content = file_path.read_text(encoding="utf-8")
except (OSError, UnicodeDecodeError):
- return None
+ try:
+ return datetime.fromtimestamp(file_path.stat().st_mtime, tz=timezone.utc)
+ except OSError:
+ return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/continuous-learning-v2/scripts/instinct-cli.py` around lines 1237 -
1240, The exception handler that currently returns None when reading frontmatter
fails causes pending files to bypass TTL pruning; instead of returning, catch
(OSError, UnicodeDecodeError) and compute the file's mtime via
file_path.stat().st_mtime (same file_path variable used in the try) and use that
as the fallback timestamp so pruning can still age/delete the file; update the
exception path in the same block that reads content (where content =
file_path.read_text(...)) to return or propagate the mtime instead of None so
downstream prune logic receives a valid timestamp.
| prune_parser.add_argument('--max-age', type=int, default=PENDING_TTL_DAYS, | ||
| help=f'Max age in days before pruning (default: {PENDING_TTL_DAYS})') |
There was a problem hiding this comment.
Reject negative --max-age values.
argparse currently accepts --max-age -1, and age_days >= -1 will treat almost every pending file as expired. This destructive CLI option should be validated as non-negative before cmd_prune runs.
🔧 Suggested fix
- prune_parser.add_argument('--max-age', type=int, default=PENDING_TTL_DAYS,
+ prune_parser.add_argument('--max-age', type=_non_negative_int, default=PENDING_TTL_DAYS,
help=f'Max age in days before pruning (default: {PENDING_TTL_DAYS})')def _non_negative_int(value: str) -> int:
parsed = int(value)
if parsed < 0:
raise argparse.ArgumentTypeError("--max-age must be >= 0")
return parsedAs per coding guidelines, "Always validate all user input before processing at system boundaries" and "Fail fast with clear error messages when validation fails".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/continuous-learning-v2/scripts/instinct-cli.py` around lines 1399 -
1400, The --max-age argument currently allows negative values; add an argparse
type validator (e.g., _non_negative_int) and use it in the
prune_parser.add_argument call instead of type=int so values < 0 raise
argparse.ArgumentTypeError with a clear message; update the default reference to
PENDING_TTL_DAYS stays the same and ensure cmd_prune continues to receive an int
(no additional cmd_prune logic changes needed).
Summary
instinct-cli.py prunesubcommand — auto-deletes pending instincts older than 30 days (configurable--max-age)statuscommand — shows pending instinct count, warns at 5+, highlights instincts expiring within 7 days/pruneslash command for user-facing manual pruningobserver-loop.shbefore each analysis cycleProblem
Pending instincts generated by the observer accumulate indefinitely with no cleanup mechanism. There is no TTL, no cap, and no notification. Users must manually run
/instinct-statusand/promote— but without a reminder, pending instincts pile up forever.Design Rationale
Auto-promote was considered and rejected — frequency of observation does not establish correctness. A pattern appearing in multiple projects proves it exists, not that it's good. TTL-based garbage collection is simpler and safer: if a pruned pattern is real, the observer will regenerate it.
Changes
scripts/instinct-cli.pyprunesubcommand,_collect_pending_dirs(),_parse_created_date(),_collect_pending_instincts()helpers, enhancecmd_status()with pending warningsagents/observer-loop.shprune --quietbefore main analysis loopcommands/prune.md/pruneslash commandTest plan
python3 instinct-cli.py prune --dry-run— verify it finds pending instincts without deletingpython3 instinct-cli.py prune— verify deletion of instincts older than 30 dayspython3 instinct-cli.py prune --max-age 0— verify all pending instincts would be prunedpython3 instinct-cli.py status— verify pending count shown, expiry warnings displayed🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation