From 4f1d5f00f0175e257ddc4a47746453edecb27017 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Sun, 26 Apr 2026 23:16:43 -0700 Subject: [PATCH 01/13] Add Codex issue digest skill (#19779) Problem: Maintainers need a shared way to run Codex GitHub issue digests without copying large prompts or relying on manual GitHub page summaries. Solution: Add a reusable codex-issue-digest skill with a deterministic GitHub collector, owner/all-label windows, reaction-aware activity metrics, scaled attention markers, and focused tests. --- .codex/skills/codex-issue-digest/SKILL.md | 102 ++ .../codex-issue-digest/agents/openai.yaml | 4 + .../scripts/collect_issue_digest.py | 988 ++++++++++++++++++ .../scripts/test_collect_issue_digest.py | 614 +++++++++++ 4 files changed, 1708 insertions(+) create mode 100644 .codex/skills/codex-issue-digest/SKILL.md create mode 100644 .codex/skills/codex-issue-digest/agents/openai.yaml create mode 100755 .codex/skills/codex-issue-digest/scripts/collect_issue_digest.py create mode 100644 .codex/skills/codex-issue-digest/scripts/test_collect_issue_digest.py diff --git a/.codex/skills/codex-issue-digest/SKILL.md b/.codex/skills/codex-issue-digest/SKILL.md new file mode 100644 index 0000000000..b531748f8c --- /dev/null +++ b/.codex/skills/codex-issue-digest/SKILL.md @@ -0,0 +1,102 @@ +--- +name: codex-issue-digest +description: Run a GitHub issue digest for openai/codex by feature-area labels, all areas, and configurable time windows. Use when asked to summarize recent Codex bug reports or enhancement requests, especially for owner-specific labels such as tui, exec, app, or similar areas. +--- + +# Codex Issue Digest + +## Objective + +Produce a concise, insight-oriented digest of `openai/codex` issues for the requested feature-area labels over the previous 24 hours by default. Honor a different duration when the user asks for one, for example "past week" or "48 hours". + +Include only issues that currently have `bug` or `enhancement` plus at least one requested owner label. If the user asks for all areas or all labels, collect `bug`/`enhancement` issues across all labels. + +## Inputs + +- Feature-area labels, for example `tui exec` +- `all areas` / `all labels` to scan all current feature labels +- Optional repo override, default `openai/codex` +- Optional time window, default previous 24 hours; examples: `48h`, `7d`, `1w`, `past week` + +## Workflow + +1. Run the collector from a current Codex repo checkout: + +```bash +python3 .codex/skills/codex-issue-digest/scripts/collect_issue_digest.py --labels tui exec --window-hours 24 +``` + +Use `--window "past week"` or `--window-hours 168` when the user asks for a non-default duration. Use `--all-labels` when the user says all areas or all labels. + +2. Use the JSON as the source of truth. It includes new issues, new issue comments, new reactions/upvotes, current labels, current reaction counts, model-ready `summary_inputs`, and detailed `digest_rows`. +3. Start the report with `## Summary`, then `## Details`. +4. In `## Summary`, write skim-first headlines: + - Lead with the most important fact or judgment. Do not start with aggregate counts unless the aggregate itself is the story. + - Make the first 1-3 bullets answer "what should owners pay attention to right now?" + - Bold only the critical insight phrase in each high-priority bullet, for example `**GPT-5.5 context is the dominant pressure point**`. + - Keep summary bullets short enough to scan in about 20 seconds. + - Put broad stats near the end of the summary, after the owner-relevant takeaways. + - Say clearly when there is nothing significant to act on. + - Call out any areas or themes receiving lots of user attention. + - Cluster and name themes yourself from `summary_inputs`; the collector intentionally does not hard-code issue categories. + - Use a cluster only when the issues genuinely share the same product problem. If several issues merely share a broad platform or label, describe them individually. + - Do not omit a repeated theme just because its individual issues fall below the details table cutoff. Several similar reports should be called out as a repeated customer concern. + - For single-issue rows, summarize the concern directly instead of calling it a cluster. + - Use inline numbered issue links from each relevant row's `ref_markdown`. +5. In `## Details`, include a compact table only when useful: + - Prefer rows from `digest_rows`; include a `Refs` column using each row's `ref_markdown`. + - Keep the table short; omit low-signal rows when the summary already covers them. + - Use compact columns such as marker, area, type, description, interactions, and refs. + - The `Description` cell should be a short owner-readable phrase. Use row `description`, title, body excerpts, and recent comments, but do not mechanically copy the raw GitHub issue title when it contains incidental details. + - A clear quiet/no-concern sentence when there is no meaningful signal. +6. Use the JSON `attention_marker` exactly. It is empty for normal rows, `🔥` for elevated rows, and `🔥🔥` for very high-attention rows. The actual cutoffs are in `attention_thresholds`. +7. Use inline numbered references where a row or bullet points to issues, for example `Compaction bugs [1](https://github.com/openai/codex/issues/123), [2](https://github.com/openai/codex/issues/456)`. Do not add a separate footnotes section. +8. Label `interactions` as `Interactions`; it counts posts/comments/reactions during the requested window, not unique people. +9. Mention the collector `script_version`, repo checkout `git_head`, and time window in the digest footer or final line. + +## Reaction Handling + +The collector uses GitHub reactions endpoints, which include `created_at`, to count reactions created during the digest window for hydrated issues. It reports both in-window reaction counts and current reaction totals. Treat current reaction totals as standing engagement, and treat `new_reactions` / `new_upvotes` as windowed activity. + +By default, the collector fetches issue comments with `since=` and caps the number of comment pages per issue. This keeps very long historical threads from dominating a digest run and focuses the report on recent posts. Use `--fetch-all-comments` only when exhaustive comment history is more important than runtime. + +GitHub issue search is still seeded by issue `updated_at`, so a purely reaction-only issue may be missed if reactions do not bump `updated_at`. Covering every reaction-only case would require either a persisted snapshot store or a broader scan of labeled issues. + +## Attention Markers + +The collector scales attention markers by the requested time window. The baseline is 10 human user interactions for `🔥` and 20 for `🔥🔥` over 24 hours; longer or shorter windows scale those cutoffs linearly and round up. For example, a one-week report uses 70 and 140 interactions. Human user interactions are human-authored new issue posts, human-authored new comments, and human reactions created during the window, including upvotes. Bot posts and bot reactions are excluded. In prose, explain this as high user interaction rather than naming the emoji. + +## Freshness + +The automation should run from a repo checkout that contains this skill. For shared daily use, prefer one of these patterns: + +- Run the automation in a checkout that is refreshed before the automation starts, for example with `git pull --ff-only`. +- If the automation cannot safely mutate the checkout, have it report the current `git_head` from the collector output so readers know which skill/script version produced the digest. + +## Sample Owner Prompt + +```text +Use $codex-issue-digest to run the Codex issue digest for labels tui and exec over the previous 24 hours. +``` + +```text +Use $codex-issue-digest to run the Codex issue digest for all areas over the past week. +``` + +## Validation + +Dry run the collector against recent issues: + +```bash +python3 .codex/skills/codex-issue-digest/scripts/collect_issue_digest.py --labels tui exec --window-hours 24 +``` + +```bash +python3 .codex/skills/codex-issue-digest/scripts/collect_issue_digest.py --all-labels --window "past week" --limit-issues 10 +``` + +Run the focused script tests: + +```bash +pytest .codex/skills/codex-issue-digest/scripts/test_collect_issue_digest.py +``` diff --git a/.codex/skills/codex-issue-digest/agents/openai.yaml b/.codex/skills/codex-issue-digest/agents/openai.yaml new file mode 100644 index 0000000000..706ce5e11b --- /dev/null +++ b/.codex/skills/codex-issue-digest/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + display_name: "Codex Issue Digest" + short_description: "Summarize Codex issues by labels or all areas" + default_prompt: "Use $codex-issue-digest to run the Codex issue digest for labels tui and exec over the previous 24 hours." diff --git a/.codex/skills/codex-issue-digest/scripts/collect_issue_digest.py b/.codex/skills/codex-issue-digest/scripts/collect_issue_digest.py new file mode 100755 index 0000000000..e211af08f8 --- /dev/null +++ b/.codex/skills/codex-issue-digest/scripts/collect_issue_digest.py @@ -0,0 +1,988 @@ +#!/usr/bin/env python3 +"""Collect recent openai/codex issue activity for owner-focused digests.""" + +import argparse +import json +import math +import re +import subprocess +import sys +from datetime import datetime, timedelta, timezone +from pathlib import Path +from urllib.parse import quote + +SCRIPT_VERSION = 2 +QUALIFYING_KIND_LABELS = ("bug", "enhancement") +REACTION_KEYS = ("+1", "-1", "laugh", "hooray", "confused", "heart", "rocket", "eyes") +BASE_ATTENTION_WINDOW_HOURS = 24.0 +ONE_ATTENTION_INTERACTION_THRESHOLD = 10 +TWO_ATTENTION_INTERACTION_THRESHOLD = 20 +ALL_LABEL_PHRASES = {"all", "all areas", "all labels", "all-areas", "all-labels", "*"} + + +class GhCommandError(RuntimeError): + pass + + +def parse_args(): + parser = argparse.ArgumentParser( + description="Collect recent GitHub issue activity for a Codex owner digest." + ) + parser.add_argument( + "--repo", default="openai/codex", help="OWNER/REPO, default openai/codex" + ) + parser.add_argument( + "--labels", + nargs="+", + default=[], + help="Feature-area labels owned by the digest recipient, for example: tui exec", + ) + parser.add_argument( + "--all-labels", + action="store_true", + help="Collect bug/enhancement issues across all feature-area labels", + ) + parser.add_argument( + "--window", + help='Lookback duration such as "24h", "7d", "1w", or "past week"', + ) + parser.add_argument( + "--window-hours", type=float, default=24.0, help="Lookback window" + ) + parser.add_argument( + "--since", help="UTC ISO timestamp override for the window start" + ) + parser.add_argument("--until", help="UTC ISO timestamp override for the window end") + parser.add_argument( + "--limit-issues", + type=int, + default=200, + help="Maximum candidate issues to hydrate after search", + ) + parser.add_argument( + "--body-chars", type=int, default=1200, help="Issue body excerpt length" + ) + parser.add_argument( + "--comment-chars", type=int, default=900, help="Comment excerpt length" + ) + parser.add_argument( + "--max-comment-pages", + type=int, + default=3, + help=( + "Maximum pages of issue comments to hydrate per issue after applying the " + "window filter. Use 0 with --fetch-all-comments for no page cap." + ), + ) + parser.add_argument( + "--fetch-all-comments", + action="store_true", + help="Hydrate complete issue comment histories instead of only window-updated comments.", + ) + return parser.parse_args() + + +def parse_timestamp(value, arg_name): + if value is None: + return None + normalized = value.strip() + if not normalized: + return None + if normalized.endswith("Z"): + normalized = f"{normalized[:-1]}+00:00" + try: + parsed = datetime.fromisoformat(normalized) + except ValueError as err: + raise ValueError(f"{arg_name} must be an ISO timestamp") from err + if parsed.tzinfo is None: + parsed = parsed.replace(tzinfo=timezone.utc) + return parsed.astimezone(timezone.utc) + + +def format_timestamp(value): + return ( + value.astimezone(timezone.utc) + .replace(microsecond=0) + .isoformat() + .replace("+00:00", "Z") + ) + + +def resolve_window(args): + until = parse_timestamp(args.until, "--until") or datetime.now(timezone.utc) + since = parse_timestamp(args.since, "--since") + if since is None: + hours = parse_duration_hours(getattr(args, "window", None)) + if hours is None: + hours = getattr(args, "window_hours", 24.0) + if hours <= 0: + raise ValueError("window duration must be > 0") + since = until - timedelta(hours=hours) + if since >= until: + raise ValueError("--since must be before --until") + return since, until + + +def parse_duration_hours(value): + if value is None: + return None + text = value.strip().casefold().replace("_", " ") + if not text: + return None + text = re.sub(r"^(past|last)\s+", "", text) + aliases = { + "day": 24.0, + "24h": 24.0, + "week": 168.0, + "7d": 168.0, + } + if text in aliases: + return aliases[text] + match = re.fullmatch(r"(\d+(?:\.\d+)?)\s*(h|hr|hrs|hour|hours)", text) + if match: + return float(match.group(1)) + match = re.fullmatch(r"(\d+(?:\.\d+)?)\s*(d|day|days)", text) + if match: + return float(match.group(1)) * 24.0 + match = re.fullmatch(r"(\d+(?:\.\d+)?)\s*(w|week|weeks)", text) + if match: + return float(match.group(1)) * 168.0 + raise ValueError(f"Unsupported duration: {value}") + + +def normalize_requested_labels(labels, all_labels=False): + out = [] + seen = set() + for raw in labels: + for piece in raw.split(","): + label = piece.strip() + if not label: + continue + key = label.casefold() + if key not in seen: + out.append(label) + seen.add(key) + phrase = " ".join(label.casefold() for label in out) + if all_labels or phrase in ALL_LABEL_PHRASES: + return [], True + if not out: + raise ValueError( + "At least one feature-area label is required, or use --all-labels" + ) + return out, False + + +def quote_label(label): + if re.fullmatch(r"[A-Za-z0-9_.:-]+", label): + return f"label:{label}" + escaped = label.replace('"', '\\"') + return f'label:"{escaped}"' + + +def build_search_queries( + repo, owner_labels, since, kind_labels=QUALIFYING_KIND_LABELS, all_labels=False +): + since_date = since.date().isoformat() + queries = [] + if all_labels: + for kind_label in kind_labels: + queries.append( + " ".join( + [ + f"repo:{repo}", + "is:issue", + f"updated:>={since_date}", + quote_label(kind_label), + ] + ) + ) + return queries + for owner_label in owner_labels: + for kind_label in kind_labels: + queries.append( + " ".join( + [ + f"repo:{repo}", + "is:issue", + f"updated:>={since_date}", + quote_label(owner_label), + quote_label(kind_label), + ] + ) + ) + return queries + + +def _format_gh_error(cmd, err): + stdout = (err.stdout or "").strip() + stderr = (err.stderr or "").strip() + parts = [f"GitHub CLI command failed: {' '.join(cmd)}"] + if stdout: + parts.append(f"stdout: {stdout}") + if stderr: + parts.append(f"stderr: {stderr}") + return "\n".join(parts) + + +def gh_json(args): + cmd = ["gh", *args] + try: + proc = subprocess.run(cmd, check=True, capture_output=True, text=True) + except FileNotFoundError as err: + raise GhCommandError("`gh` command not found") from err + except subprocess.CalledProcessError as err: + raise GhCommandError(_format_gh_error(cmd, err)) from err + raw = proc.stdout.strip() + if not raw: + return None + try: + return json.loads(raw) + except json.JSONDecodeError as err: + raise GhCommandError( + f"Failed to parse JSON from gh output for {' '.join(args)}" + ) from err + + +def gh_text(args): + cmd = ["gh", *args] + try: + proc = subprocess.run(cmd, check=True, capture_output=True, text=True) + except (FileNotFoundError, subprocess.CalledProcessError): + return "" + return proc.stdout.strip() + + +def git_head(): + try: + proc = subprocess.run( + ["git", "rev-parse", "--short=12", "HEAD"], + check=True, + capture_output=True, + text=True, + ) + except (FileNotFoundError, subprocess.CalledProcessError): + return None + return proc.stdout.strip() or None + + +def skill_relative_path(): + try: + return str(Path(__file__).resolve().relative_to(Path.cwd().resolve())) + except ValueError: + return str(Path(__file__).resolve()) + + +def gh_api_list_paginated(endpoint, per_page=100, max_pages=None, with_metadata=False): + items = [] + page = 1 + truncated = False + while True: + sep = "&" if "?" in endpoint else "?" + page_endpoint = f"{endpoint}{sep}per_page={per_page}&page={page}" + payload = gh_json(["api", page_endpoint]) + if payload is None: + break + if not isinstance(payload, list): + raise GhCommandError(f"Unexpected paginated payload from gh api {endpoint}") + items.extend(payload) + if len(payload) < per_page: + break + if max_pages is not None and page >= max_pages: + truncated = True + break + page += 1 + if with_metadata: + return { + "items": items, + "truncated": truncated, + "pages": page, + "max_pages": max_pages, + } + return items + + +def search_issue_numbers(queries, limit): + numbers = {} + for query in queries: + page = 1 + while True: + payload = gh_json( + [ + "api", + "search/issues", + "-X", + "GET", + "-f", + f"q={query}", + "-f", + "per_page=100", + "-f", + f"page={page}", + ] + ) + if not isinstance(payload, dict): + raise GhCommandError("Unexpected payload from GitHub issue search") + items = payload.get("items") or [] + if not isinstance(items, list): + raise GhCommandError("Expected search `items` to be a list") + for item in items: + if not isinstance(item, dict): + continue + number = item.get("number") + if isinstance(number, int): + numbers[number] = str(item.get("updated_at") or "") + if len(items) < 100 or len(numbers) >= limit: + break + page += 1 + ordered = sorted( + numbers, key=lambda number: (numbers[number], number), reverse=True + ) + return ordered[:limit] + + +def fetch_issue(repo, number): + payload = gh_json(["api", f"repos/{repo}/issues/{number}"]) + if not isinstance(payload, dict): + raise GhCommandError(f"Unexpected issue payload for #{number}") + return payload + + +def fetch_comments(repo, number, since=None, max_pages=None): + endpoint = f"repos/{repo}/issues/{number}/comments" + if since is not None: + endpoint = f"{endpoint}?since={quote(format_timestamp(since), safe='')}" + return gh_api_list_paginated( + endpoint, + max_pages=max_pages, + with_metadata=True, + ) + + +def fetch_reactions_for_item(endpoint, item): + if reaction_summary(item)["total"] <= 0: + return [] + return gh_api_list_paginated(endpoint) + + +def fetch_comment_reactions(repo, comments): + reactions_by_comment_id = {} + for comment in comments: + comment_id = comment.get("id") + if comment_id in (None, ""): + continue + endpoint = f"repos/{repo}/issues/comments/{comment_id}/reactions" + reactions_by_comment_id[comment_id] = fetch_reactions_for_item( + endpoint, comment + ) + return reactions_by_comment_id + + +def extract_login(user_obj): + if isinstance(user_obj, dict): + return str(user_obj.get("login") or "") + return "" + + +def is_bot_login(login): + return bool(login) and login.lower().endswith("[bot]") + + +def is_human_user(user_obj): + login = extract_login(user_obj) + return bool(login) and not is_bot_login(login) + + +def label_names(issue): + labels = [] + for label in issue.get("labels") or []: + if isinstance(label, dict) and label.get("name"): + labels.append(str(label["name"])) + return sorted(labels, key=str.casefold) + + +def matching_labels(labels, requested): + labels_by_key = {label.casefold(): label for label in labels} + return [label for label in requested if label.casefold() in labels_by_key] + + +def area_labels(labels): + kind_keys = {label.casefold() for label in QUALIFYING_KIND_LABELS} + return [label for label in labels if label.casefold() not in kind_keys] + + +def attention_thresholds_for_window(window_hours): + if window_hours <= 0: + raise ValueError("window_hours must be > 0") + window_hours = round(window_hours, 6) + scale = window_hours / BASE_ATTENTION_WINDOW_HOURS + elevated = max(1, math.ceil(ONE_ATTENTION_INTERACTION_THRESHOLD * scale)) + very_high = max( + elevated + 1, math.ceil(TWO_ATTENTION_INTERACTION_THRESHOLD * scale) + ) + return { + "base_window_hours": BASE_ATTENTION_WINDOW_HOURS, + "window_hours": round(window_hours, 3), + "scale": round(scale, 3), + "elevated": elevated, + "very_high": very_high, + } + + +def attention_level_for(user_interactions, attention_thresholds=None): + thresholds = attention_thresholds or attention_thresholds_for_window( + BASE_ATTENTION_WINDOW_HOURS + ) + if user_interactions >= thresholds["very_high"]: + return 2 + if user_interactions >= thresholds["elevated"]: + return 1 + return 0 + + +def attention_marker_for(user_interactions, attention_thresholds=None): + return "🔥" * attention_level_for(user_interactions, attention_thresholds) + + +def reaction_summary(item): + reactions = item.get("reactions") + if not isinstance(reactions, dict): + return {"total": 0, "counts": {}} + counts = {} + for key in REACTION_KEYS: + value = reactions.get(key, 0) + if isinstance(value, int) and value: + counts[key] = value + total = reactions.get("total_count") + if not isinstance(total, int): + total = sum(counts.values()) + return {"total": total, "counts": counts} + + +def reaction_event_summary(reactions, since, until): + counts = {} + total = 0 + for reaction in reactions or []: + if not isinstance(reaction, dict): + continue + if not is_in_window(str(reaction.get("created_at") or ""), since, until): + continue + if not is_human_user(reaction.get("user")): + continue + content = str(reaction.get("content") or "") + if not content: + continue + counts[content] = counts.get(content, 0) + 1 + total += 1 + return { + "total": total, + "counts": counts, + "upvotes": counts.get("+1", 0), + } + + +def compact_text(value, limit): + text = re.sub(r"\s+", " ", str(value or "")).strip() + if limit <= 0: + return "" + if len(text) <= limit: + return text + return f"{text[: max(limit - 1, 0)].rstrip()}..." + + +def clean_title_for_description(title): + cleaned = re.sub(r"\s+", " ", str(title or "")).strip() + cleaned = re.sub( + r"^(codex(?: desktop| app|\.app| cli)?|desktop|windows codex app)\s*[:,-]\s*", + "", + cleaned, + flags=re.IGNORECASE, + ) + cleaned = re.sub(r"^on windows,\s*", "Windows: ", cleaned, flags=re.IGNORECASE) + cleaned = cleaned.strip(" -:;") + return compact_text(cleaned, 80) or "Issue needs owner review" + + +def issue_description(issue): + return clean_title_for_description(issue.get("title")) + + +def is_in_window(timestamp, since, until): + parsed = parse_timestamp(timestamp, "timestamp") + if parsed is None: + return False + return since <= parsed < until + + +def summarize_comment( + comment, comment_chars, reaction_events=None, since=None, until=None +): + reactions = reaction_summary(comment) + new_reactions = ( + reaction_event_summary(reaction_events, since, until) + if since is not None and until is not None + else {"total": 0, "counts": {}, "upvotes": 0} + ) + human_user_interaction = is_human_user(comment.get("user")) + return { + "id": comment.get("id"), + "author": extract_login(comment.get("user")), + "author_association": str(comment.get("author_association") or ""), + "created_at": str(comment.get("created_at") or ""), + "updated_at": str(comment.get("updated_at") or ""), + "url": str(comment.get("html_url") or ""), + "human_user_interaction": human_user_interaction, + "reactions": reactions["counts"], + "reaction_total": reactions["total"], + "new_reactions": new_reactions["total"], + "new_upvotes": new_reactions["upvotes"], + "new_reaction_counts": new_reactions["counts"], + "body_excerpt": compact_text(comment.get("body"), comment_chars), + } + + +def summarize_issue( + issue, + comments, + requested_labels, + since, + until, + body_chars, + comment_chars, + issue_reaction_events=None, + comment_reactions_by_id=None, + all_labels=False, + comments_hydration=None, + attention_thresholds=None, +): + labels = label_names(issue) + labels_by_key = {label.casefold() for label in labels} + kind_labels = [ + label for label in QUALIFYING_KIND_LABELS if label.casefold() in labels_by_key + ] + if all_labels: + owner_labels = area_labels(labels) or ["unlabeled"] + else: + owner_labels = matching_labels(labels, requested_labels) + if not kind_labels or not owner_labels: + return None + + updated_at = str(issue.get("updated_at") or "") + if not is_in_window(updated_at, since, until): + return None + + new_issue = is_in_window(str(issue.get("created_at") or ""), since, until) + comment_reactions_by_id = comment_reactions_by_id or {} + new_comments = [ + summarize_comment( + comment, + comment_chars, + reaction_events=comment_reactions_by_id.get(comment.get("id")), + since=since, + until=until, + ) + for comment in comments + if is_in_window(str(comment.get("created_at") or ""), since, until) + ] + new_comments.sort(key=lambda item: (item["created_at"], str(item["id"]))) + + issue_reactions = reaction_summary(issue) + issue_reaction_events_summary = reaction_event_summary( + issue_reaction_events, since, until + ) + comment_reaction_events_summary = reaction_event_summary( + [ + reaction + for reactions in comment_reactions_by_id.values() + for reaction in reactions + ], + since, + until, + ) + new_reactions = ( + issue_reaction_events_summary["total"] + + comment_reaction_events_summary["total"] + ) + new_upvotes = ( + issue_reaction_events_summary["upvotes"] + + comment_reaction_events_summary["upvotes"] + ) + all_comment_reaction_total = sum( + reaction_summary(comment)["total"] for comment in comments + ) + new_comment_reaction_total = sum( + comment["reaction_total"] for comment in new_comments + ) + new_issue_user_interaction = new_issue and is_human_user(issue.get("user")) + new_comment_user_interactions = sum( + 1 for comment in new_comments if comment["human_user_interaction"] + ) + user_interactions = ( + int(new_issue_user_interaction) + new_comment_user_interactions + new_reactions + ) + attention_level = attention_level_for(user_interactions, attention_thresholds) + attention_marker = attention_marker_for(user_interactions, attention_thresholds) + updated_without_visible_new_post = ( + not new_issue and not new_comments and new_reactions == 0 + ) + + engagement_score = ( + len(new_comments) * 3 + + new_reactions + + issue_reactions["total"] + + new_comment_reaction_total + + min(int(issue.get("comments") or len(comments) or 0), 10) + ) + + return { + "number": issue.get("number"), + "title": str(issue.get("title") or ""), + "description": issue_description(issue), + "url": str(issue.get("html_url") or ""), + "state": str(issue.get("state") or ""), + "author": extract_login(issue.get("user")), + "author_association": str(issue.get("author_association") or ""), + "created_at": str(issue.get("created_at") or ""), + "updated_at": updated_at, + "labels": labels, + "kind_labels": kind_labels, + "owner_labels": owner_labels, + "comments_total": int(issue.get("comments") or len(comments) or 0), + "comments_hydration": comments_hydration + or { + "fetched": len(comments), + "since": None, + "truncated": False, + "max_pages": None, + }, + "issue_reactions": issue_reactions["counts"], + "issue_reaction_total": issue_reactions["total"], + "comment_reaction_total": all_comment_reaction_total, + "new_comment_reaction_total": new_comment_reaction_total, + "new_issue_reactions": issue_reaction_events_summary["total"], + "new_issue_upvotes": issue_reaction_events_summary["upvotes"], + "new_comment_reactions": comment_reaction_events_summary["total"], + "new_comment_upvotes": comment_reaction_events_summary["upvotes"], + "new_reactions": new_reactions, + "new_upvotes": new_upvotes, + "user_interactions": user_interactions, + "attention": attention_level > 0, + "attention_level": attention_level, + "attention_marker": attention_marker, + "engagement_score": engagement_score, + "activity": { + "new_issue": new_issue, + "new_comments": len(new_comments), + "new_human_comments": new_comment_user_interactions, + "new_reactions": new_reactions, + "new_upvotes": new_upvotes, + "updated_without_visible_new_post": updated_without_visible_new_post, + }, + "body_excerpt": compact_text(issue.get("body"), body_chars), + "new_comments": new_comments, + } + + +def count_by_label(issues, labels): + out = {} + for label in labels: + matching = [issue for issue in issues if label in issue["owner_labels"]] + out[label] = { + "issues": len(matching), + "new_issues": sum( + 1 for issue in matching if issue["activity"]["new_issue"] + ), + "new_comments": sum( + issue["activity"]["new_comments"] for issue in matching + ), + } + return out + + +def count_by_kind(issues): + out = {} + for kind in QUALIFYING_KIND_LABELS: + matching = [issue for issue in issues if kind in issue["kind_labels"]] + out[kind] = { + "issues": len(matching), + "new_issues": sum( + 1 for issue in matching if issue["activity"]["new_issue"] + ), + "new_comments": sum( + issue["activity"]["new_comments"] for issue in matching + ), + } + return out + + +def hot_items(issues, limit=8): + ranked = sorted( + issues, + key=lambda issue: ( + issue["attention"], + issue["attention_level"], + issue["user_interactions"], + issue["engagement_score"], + issue["activity"]["new_comments"], + issue["issue_reaction_total"] + issue["comment_reaction_total"], + issue["updated_at"], + ), + reverse=True, + ) + return [ + { + "number": issue["number"], + "title": issue["title"], + "url": issue["url"], + "owner_labels": issue["owner_labels"], + "kind_labels": issue["kind_labels"], + "attention": issue["attention"], + "attention_level": issue["attention_level"], + "attention_marker": issue["attention_marker"], + "user_interactions": issue["user_interactions"], + "new_reactions": issue["new_reactions"], + "new_upvotes": issue["new_upvotes"], + "engagement_score": issue["engagement_score"], + "new_comments": issue["activity"]["new_comments"], + "reaction_total": issue["issue_reaction_total"] + + issue["comment_reaction_total"], + } + for issue in ranked[:limit] + if issue["engagement_score"] > 0 + ] + + +def ranked_digest_issues(issues): + return sorted( + issues, + key=lambda issue: ( + issue["attention"], + issue["attention_level"], + issue["user_interactions"], + issue["engagement_score"], + issue["activity"]["new_comments"], + issue["updated_at"], + ), + reverse=True, + ) + + +def digest_rows(issues, limit=10, ref_map=None): + ranked = ranked_digest_issues(issues) + if ref_map is None: + ref_map = {issue["number"]: ref for ref, issue in enumerate(ranked, start=1)} + rows = [] + for issue in ranked[:limit]: + ref = ref_map[issue["number"]] + reaction_total = issue["issue_reaction_total"] + issue["comment_reaction_total"] + rows.append( + { + "ref": ref, + "ref_markdown": f"[{ref}]({issue['url']})", + "marker": issue["attention_marker"], + "attention_marker": issue["attention_marker"], + "number": issue["number"], + "description": issue["description"], + "title": issue["title"], + "url": issue["url"], + "area": ", ".join(issue["owner_labels"]), + "kind": ", ".join(issue["kind_labels"]), + "state": issue["state"], + "interactions": issue["user_interactions"], + "user_interactions": issue["user_interactions"], + "new_reactions": issue["new_reactions"], + "new_upvotes": issue["new_upvotes"], + "current_reactions": reaction_total, + } + ) + return rows + + +def issue_ref_markdown(issue, ref_map): + ref = ref_map[issue["number"]] + return f"[{ref}]({issue['url']})" + + +def summary_inputs(issues, limit=80, ref_map=None): + ranked = ranked_digest_issues(issues) + if ref_map is None: + ref_map = {issue["number"]: ref for ref, issue in enumerate(ranked, start=1)} + rows = [] + for issue in ranked[:limit]: + rows.append( + { + "ref": ref_map[issue["number"]], + "ref_markdown": issue_ref_markdown(issue, ref_map), + "number": issue["number"], + "title": issue["title"], + "description": issue["description"], + "url": issue["url"], + "labels": issue["labels"], + "owner_labels": issue["owner_labels"], + "kind_labels": issue["kind_labels"], + "state": issue.get("state", ""), + "attention_marker": issue.get("attention_marker", ""), + "interactions": issue["user_interactions"], + "new_comments": issue["activity"].get("new_comments", 0), + "new_reactions": issue.get("new_reactions", 0), + "new_upvotes": issue.get("new_upvotes", 0), + "current_reactions": issue.get("issue_reaction_total", 0) + + issue.get("comment_reaction_total", 0), + } + ) + return rows + + +def collect_digest(args): + since, until = resolve_window(args) + window_hours = (until - since).total_seconds() / 3600 + attention_thresholds = attention_thresholds_for_window(window_hours) + requested_labels, all_labels = normalize_requested_labels( + args.labels, all_labels=args.all_labels + ) + queries = build_search_queries( + args.repo, requested_labels, since, all_labels=all_labels + ) + numbers = search_issue_numbers(queries, args.limit_issues) + gh_version_output = gh_text(["--version"]) + + issues = [] + max_comment_pages = None if args.max_comment_pages <= 0 else args.max_comment_pages + for number in numbers: + issue = fetch_issue(args.repo, number) + comments_since = None if args.fetch_all_comments else since + comments_payload = fetch_comments( + args.repo, + number, + since=comments_since, + max_pages=max_comment_pages, + ) + comments = comments_payload["items"] + issue_reaction_events = fetch_reactions_for_item( + f"repos/{args.repo}/issues/{number}/reactions", issue + ) + comment_reactions_by_id = fetch_comment_reactions(args.repo, comments) + comments_hydration = { + "fetched": len(comments), + "total": int(issue.get("comments") or len(comments) or 0), + "since": format_timestamp(comments_since) if comments_since else None, + "truncated": comments_payload["truncated"], + "max_pages": comments_payload["max_pages"], + "fetch_all_comments": args.fetch_all_comments, + } + summary = summarize_issue( + issue, + comments, + requested_labels, + since, + until, + args.body_chars, + args.comment_chars, + issue_reaction_events=issue_reaction_events, + comment_reactions_by_id=comment_reactions_by_id, + all_labels=all_labels, + comments_hydration=comments_hydration, + attention_thresholds=attention_thresholds, + ) + if summary is not None: + issues.append(summary) + + issues.sort( + key=lambda issue: (issue["updated_at"], int(issue["number"] or 0)), reverse=True + ) + totals = { + "candidate_issues": len(numbers), + "included_issues": len(issues), + "new_issues": sum(1 for issue in issues if issue["activity"]["new_issue"]), + "issues_with_new_comments": sum( + 1 for issue in issues if issue["activity"]["new_comments"] > 0 + ), + "new_comments": sum(issue["activity"]["new_comments"] for issue in issues), + "comments_fetched": sum( + issue["comments_hydration"]["fetched"] for issue in issues + ), + "issues_with_truncated_comment_hydration": sum( + 1 for issue in issues if issue["comments_hydration"]["truncated"] + ), + "updated_without_visible_new_post": sum( + 1 + for issue in issues + if issue["activity"]["updated_without_visible_new_post"] + ), + "issue_reactions_current_total": sum( + issue["issue_reaction_total"] for issue in issues + ), + "comment_reactions_current_total": sum( + issue["comment_reaction_total"] for issue in issues + ), + "new_reactions": sum(issue["new_reactions"] for issue in issues), + "new_upvotes": sum(issue["new_upvotes"] for issue in issues), + "user_interactions": sum(issue["user_interactions"] for issue in issues), + } + ranked = ranked_digest_issues(issues) + ref_map = {issue["number"]: ref for ref, issue in enumerate(ranked, start=1)} + filter_label = "all" if all_labels else requested_labels + + return { + "generated_at": format_timestamp(datetime.now(timezone.utc)), + "source": { + "repo": args.repo, + "skill": "codex-issue-digest", + "collector": skill_relative_path(), + "script_version": SCRIPT_VERSION, + "git_head": git_head(), + "gh_version": gh_version_output.splitlines()[0] + if gh_version_output + else None, + }, + "window": { + "since": format_timestamp(since), + "until": format_timestamp(until), + "hours": round(window_hours, 3), + }, + "attention_thresholds": attention_thresholds, + "filters": { + "owner_labels": filter_label, + "all_labels": all_labels, + "kind_labels": list(QUALIFYING_KIND_LABELS), + }, + "collection_notes": [ + "Issues are selected when they currently have bug or enhancement plus at least one requested owner label and were updated during the window.", + "By default, issue comments are fetched with since=window_start and a max page cap to avoid long historical threads; use --fetch-all-comments when exhaustive comment history is needed.", + "New issue comments are filtered by comment creation time within the window from the fetched comment set.", + "Reaction events are counted by GitHub reaction created_at timestamps for hydrated issues and fetched comments.", + "Current reaction totals are standing engagement signals; new_reactions and new_upvotes are windowed activity.", + "The collector does not assign semantic clusters; use summary_inputs as model-ready evidence for report-time clustering.", + "Pure reaction-only issues may be missed if GitHub issue search does not surface them via updated_at.", + "Issues updated during the window without a new issue body or new comment are retained because label/status edits can still be useful owner signals.", + ], + "totals": totals, + "by_owner_label": count_by_label( + issues, + sorted( + {area for issue in issues for area in issue["owner_labels"]}, + key=str.casefold, + ) + if all_labels + else requested_labels, + ), + "by_kind_label": count_by_kind(issues), + "hot_items": hot_items(issues), + "summary_inputs": summary_inputs(issues, ref_map=ref_map), + "digest_rows": digest_rows(issues, ref_map=ref_map), + "issues": issues, + } + + +def main(): + args = parse_args() + try: + digest = collect_digest(args) + except (GhCommandError, RuntimeError, ValueError) as err: + sys.stderr.write(f"collect_issue_digest.py error: {err}\n") + return 1 + sys.stdout.write(json.dumps(digest, indent=2, sort_keys=True) + "\n") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.codex/skills/codex-issue-digest/scripts/test_collect_issue_digest.py b/.codex/skills/codex-issue-digest/scripts/test_collect_issue_digest.py new file mode 100644 index 0000000000..1c283ea2f6 --- /dev/null +++ b/.codex/skills/codex-issue-digest/scripts/test_collect_issue_digest.py @@ -0,0 +1,614 @@ +import importlib.util +from datetime import timezone +from pathlib import Path + + +MODULE_PATH = Path(__file__).with_name("collect_issue_digest.py") +MODULE_SPEC = importlib.util.spec_from_file_location( + "collect_issue_digest", MODULE_PATH +) +collect_issue_digest = importlib.util.module_from_spec(MODULE_SPEC) +assert MODULE_SPEC.loader is not None +MODULE_SPEC.loader.exec_module(collect_issue_digest) + + +def test_build_search_queries_uses_each_owner_and_kind_label(): + since = collect_issue_digest.parse_timestamp("2026-04-25T12:34:56Z", "--since") + + queries = collect_issue_digest.build_search_queries( + "openai/codex", ["tui", "exec"], since + ) + + assert queries == [ + "repo:openai/codex is:issue updated:>=2026-04-25 label:tui label:bug", + "repo:openai/codex is:issue updated:>=2026-04-25 label:tui label:enhancement", + "repo:openai/codex is:issue updated:>=2026-04-25 label:exec label:bug", + "repo:openai/codex is:issue updated:>=2026-04-25 label:exec label:enhancement", + ] + + +def test_build_search_queries_can_scan_all_labels(): + since = collect_issue_digest.parse_timestamp("2026-04-25T12:34:56Z", "--since") + + queries = collect_issue_digest.build_search_queries( + "openai/codex", [], since, all_labels=True + ) + + assert queries == [ + "repo:openai/codex is:issue updated:>=2026-04-25 label:bug", + "repo:openai/codex is:issue updated:>=2026-04-25 label:enhancement", + ] + + +def test_normalize_requested_labels_accepts_all_area_phrases(): + assert collect_issue_digest.normalize_requested_labels(["all", "areas"]) == ( + [], + True, + ) + assert collect_issue_digest.normalize_requested_labels(["all-labels"]) == ( + [], + True, + ) + + +def test_summarize_issue_keeps_new_comments_and_reaction_signals(): + since = collect_issue_digest.parse_timestamp("2026-04-25T00:00:00Z", "--since") + until = collect_issue_digest.parse_timestamp("2026-04-26T00:00:00Z", "--until") + issue = { + "number": 123, + "title": "TUI does not redraw", + "html_url": "https://github.com/openai/codex/issues/123", + "state": "open", + "created_at": "2026-04-24T20:00:00Z", + "updated_at": "2026-04-25T10:00:00Z", + "user": {"login": "alice"}, + "author_association": "NONE", + "comments": 2, + "body": "The terminal freezes after resize.", + "labels": [{"name": "bug"}, {"name": "tui"}], + "reactions": {"total_count": 3, "+1": 2, "rocket": 1}, + } + comments = [ + { + "id": 1, + "created_at": "2026-04-25T11:00:00Z", + "updated_at": "2026-04-25T11:00:00Z", + "html_url": "https://github.com/openai/codex/issues/123#issuecomment-1", + "user": {"login": "bob"}, + "author_association": "MEMBER", + "body": "I can reproduce this on main.", + "reactions": {"total_count": 4, "heart": 1, "+1": 3}, + }, + { + "id": 2, + "created_at": "2026-04-24T11:00:00Z", + "updated_at": "2026-04-24T11:00:00Z", + "html_url": "https://github.com/openai/codex/issues/123#issuecomment-2", + "user": {"login": "carol"}, + "author_association": "NONE", + "body": "Older comment.", + "reactions": {"total_count": 1, "eyes": 1}, + }, + ] + + summary = collect_issue_digest.summarize_issue( + issue, + comments, + ["tui", "exec"], + since, + until, + body_chars=200, + comment_chars=200, + ) + + assert summary == { + "number": 123, + "title": "TUI does not redraw", + "description": "TUI does not redraw", + "url": "https://github.com/openai/codex/issues/123", + "state": "open", + "author": "alice", + "author_association": "NONE", + "created_at": "2026-04-24T20:00:00Z", + "updated_at": "2026-04-25T10:00:00Z", + "labels": ["bug", "tui"], + "kind_labels": ["bug"], + "owner_labels": ["tui"], + "comments_total": 2, + "comments_hydration": { + "fetched": 2, + "since": None, + "truncated": False, + "max_pages": None, + }, + "issue_reactions": {"+1": 2, "rocket": 1}, + "issue_reaction_total": 3, + "comment_reaction_total": 5, + "new_comment_reaction_total": 4, + "new_issue_reactions": 0, + "new_issue_upvotes": 0, + "new_comment_reactions": 0, + "new_comment_upvotes": 0, + "new_reactions": 0, + "new_upvotes": 0, + "user_interactions": 1, + "attention": False, + "attention_level": 0, + "attention_marker": "", + "engagement_score": 12, + "activity": { + "new_issue": False, + "new_comments": 1, + "new_human_comments": 1, + "new_reactions": 0, + "new_upvotes": 0, + "updated_without_visible_new_post": False, + }, + "body_excerpt": "The terminal freezes after resize.", + "new_comments": [ + { + "id": 1, + "author": "bob", + "author_association": "MEMBER", + "created_at": "2026-04-25T11:00:00Z", + "updated_at": "2026-04-25T11:00:00Z", + "url": "https://github.com/openai/codex/issues/123#issuecomment-1", + "human_user_interaction": True, + "reactions": {"+1": 3, "heart": 1}, + "reaction_total": 4, + "new_reactions": 0, + "new_upvotes": 0, + "new_reaction_counts": {}, + "body_excerpt": "I can reproduce this on main.", + } + ], + } + + +def test_summarize_issue_filters_non_owner_or_non_kind_labels(): + since = collect_issue_digest.parse_timestamp("2026-04-25T00:00:00Z", "--since") + until = collect_issue_digest.parse_timestamp("2026-04-26T00:00:00Z", "--until") + base_issue = { + "number": 1, + "title": "Question", + "created_at": "2026-04-25T01:00:00Z", + "updated_at": "2026-04-25T01:00:00Z", + "labels": [{"name": "question"}, {"name": "tui"}], + } + + assert ( + collect_issue_digest.summarize_issue( + base_issue, + [], + ["tui"], + since, + until, + body_chars=100, + comment_chars=100, + ) + is None + ) + + issue_without_owner = dict(base_issue) + issue_without_owner["labels"] = [{"name": "bug"}, {"name": "app"}] + + assert ( + collect_issue_digest.summarize_issue( + issue_without_owner, + [], + ["tui"], + since, + until, + body_chars=100, + comment_chars=100, + ) + is None + ) + + +def test_resolve_window_defaults_to_previous_hours(): + class Args: + since = None + until = "2026-04-26T12:00:00Z" + window_hours = 24 + + since, until = collect_issue_digest.resolve_window(Args()) + + assert since.isoformat() == "2026-04-25T12:00:00+00:00" + assert until.tzinfo == timezone.utc + + +def test_parse_duration_hours_accepts_common_phrases(): + assert collect_issue_digest.parse_duration_hours("past week") == 168 + assert collect_issue_digest.parse_duration_hours("48h") == 48 + assert collect_issue_digest.parse_duration_hours("2 days") == 48 + assert collect_issue_digest.parse_duration_hours("1w") == 168 + + +def test_attention_thresholds_scale_by_window_length(): + one_day = collect_issue_digest.attention_thresholds_for_window(24) + assert one_day["elevated"] == 10 + assert one_day["very_high"] == 20 + + half_day = collect_issue_digest.attention_thresholds_for_window(12) + assert half_day["elevated"] == 5 + assert half_day["very_high"] == 10 + + week = collect_issue_digest.attention_thresholds_for_window(168) + assert week["elevated"] == 70 + assert week["very_high"] == 140 + assert collect_issue_digest.attention_marker_for(69, week) == "" + assert collect_issue_digest.attention_marker_for(107, week) == "🔥" + assert collect_issue_digest.attention_marker_for(140, week) == "🔥🔥" + + +def test_fetch_comments_uses_since_filter_and_page_cap(monkeypatch): + calls = [] + + def fake_gh_json(args): + calls.append(args) + return [{"id": idx} for idx in range(100)] + + monkeypatch.setattr(collect_issue_digest, "gh_json", fake_gh_json) + since = collect_issue_digest.parse_timestamp("2026-04-25T00:00:00Z", "--since") + + payload = collect_issue_digest.fetch_comments( + "openai/codex", 123, since=since, max_pages=1 + ) + + assert len(payload["items"]) == 100 + assert payload["truncated"] is True + assert payload["max_pages"] == 1 + assert calls == [ + [ + "api", + "repos/openai/codex/issues/123/comments?since=2026-04-25T00%3A00%3A00Z&per_page=100&page=1", + ] + ] + + +def test_issue_description_prefers_title_over_body_noise(): + issue = { + "title": "Codex.app GUI: MCP child processes not reaped after task completion", + "body": "A later crash mention should not override the title-level symptom.", + "labels": [{"name": "app"}, {"name": "bug"}], + } + + description = collect_issue_digest.issue_description(issue) + assert "MCP child processes" in description + assert "crash" not in description.casefold() + + +def test_attention_markers_count_human_user_interactions(): + since = collect_issue_digest.parse_timestamp("2026-04-25T00:00:00Z", "--since") + until = collect_issue_digest.parse_timestamp("2026-04-26T00:00:00Z", "--until") + issue = { + "number": 456, + "title": "Agent context is exploding", + "html_url": "https://github.com/openai/codex/issues/456", + "state": "open", + "created_at": "2026-04-25T01:00:00Z", + "updated_at": "2026-04-25T12:00:00Z", + "user": {"login": "alice"}, + "labels": [{"name": "bug"}, {"name": "agent"}], + } + comments = [ + { + "id": idx, + "created_at": "2026-04-25T02:00:00Z", + "updated_at": "2026-04-25T02:00:00Z", + "user": {"login": f"user-{idx}"}, + "body": "same here", + } + for idx in range(9) + ] + comments.append( + { + "id": 99, + "created_at": "2026-04-25T02:00:00Z", + "updated_at": "2026-04-25T02:00:00Z", + "user": {"login": "github-actions[bot]"}, + "body": "duplicate bot note", + } + ) + + summary = collect_issue_digest.summarize_issue( + issue, + comments, + ["agent"], + since, + until, + body_chars=100, + comment_chars=100, + ) + + assert summary["user_interactions"] == 10 + assert summary["activity"]["new_human_comments"] == 9 + assert summary["attention"] is True + assert summary["attention_level"] == 1 + assert summary["attention_marker"] == "🔥" + + issue["created_at"] = "2026-04-24T01:00:00Z" + comments.extend( + { + "id": idx, + "created_at": "2026-04-25T03:00:00Z", + "updated_at": "2026-04-25T03:00:00Z", + "user": {"login": f"extra-user-{idx}"}, + "body": "also seeing this", + } + for idx in range(11) + ) + + summary = collect_issue_digest.summarize_issue( + issue, + comments, + ["agent"], + since, + until, + body_chars=100, + comment_chars=100, + ) + + assert summary["user_interactions"] == 20 + assert summary["attention_level"] == 2 + assert summary["attention_marker"] == "🔥🔥" + + +def test_reactions_count_toward_attention_markers(): + since = collect_issue_digest.parse_timestamp("2026-04-25T00:00:00Z", "--since") + until = collect_issue_digest.parse_timestamp("2026-04-26T00:00:00Z", "--until") + issue = { + "number": 789, + "title": "Support 1M token context", + "html_url": "https://github.com/openai/codex/issues/789", + "state": "open", + "created_at": "2026-04-24T01:00:00Z", + "updated_at": "2026-04-25T12:00:00Z", + "user": {"login": "alice"}, + "labels": [{"name": "enhancement"}, {"name": "context"}], + "reactions": {"total_count": 20, "+1": 20}, + } + comments = [ + { + "id": 1, + "created_at": "2026-04-25T02:00:00Z", + "updated_at": "2026-04-25T02:00:00Z", + "user": {"login": "commenter"}, + "body": "please", + "reactions": {"total_count": 2, "+1": 2}, + } + ] + issue_reactions = [ + { + "content": "+1", + "created_at": "2026-04-25T03:00:00Z", + "user": {"login": f"reactor-{idx}"}, + } + for idx in range(18) + ] + comment_reactions_by_id = { + 1: [ + { + "content": "heart", + "created_at": "2026-04-25T04:00:00Z", + "user": {"login": "human-reactor"}, + }, + { + "content": "+1", + "created_at": "2026-04-25T04:00:00Z", + "user": {"login": "github-actions[bot]"}, + }, + ] + } + + summary = collect_issue_digest.summarize_issue( + issue, + comments, + ["context"], + since, + until, + body_chars=100, + comment_chars=100, + issue_reaction_events=issue_reactions, + comment_reactions_by_id=comment_reactions_by_id, + ) + + assert summary["new_reactions"] == 19 + assert summary["new_upvotes"] == 18 + assert summary["user_interactions"] == 20 + assert summary["attention_level"] == 2 + assert summary["attention_marker"] == "🔥🔥" + assert summary["new_comments"][0]["new_reactions"] == 1 + assert summary["new_comments"][0]["new_upvotes"] == 0 + + +def test_digest_rows_are_table_ready_with_concise_descriptions(): + rows = collect_issue_digest.digest_rows( + [ + { + "number": 1, + "title": "Quiet bug", + "description": "Quiet bug", + "url": "https://github.com/openai/codex/issues/1", + "owner_labels": ["context"], + "kind_labels": ["bug"], + "state": "open", + "attention": False, + "attention_level": 0, + "attention_marker": "", + "user_interactions": 1, + "new_reactions": 0, + "new_upvotes": 0, + "engagement_score": 3, + "issue_reaction_total": 0, + "comment_reaction_total": 0, + "updated_at": "2026-04-25T01:00:00Z", + "activity": { + "new_issue": True, + "new_comments": 0, + "new_reactions": 0, + "updated_without_visible_new_post": False, + }, + }, + { + "number": 2, + "title": "Busy bug", + "description": "High-volume bug report", + "url": "https://github.com/openai/codex/issues/2", + "owner_labels": ["agent"], + "kind_labels": ["bug"], + "state": "open", + "attention": True, + "attention_level": 1, + "attention_marker": "🔥", + "user_interactions": 17, + "new_reactions": 3, + "new_upvotes": 2, + "engagement_score": 20, + "issue_reaction_total": 5, + "comment_reaction_total": 2, + "updated_at": "2026-04-25T02:00:00Z", + "activity": { + "new_issue": False, + "new_comments": 16, + "new_reactions": 3, + "updated_without_visible_new_post": False, + }, + }, + ] + ) + + assert rows[0] == { + "ref": 1, + "ref_markdown": "[1](https://github.com/openai/codex/issues/2)", + "marker": "🔥", + "attention_marker": "🔥", + "number": 2, + "description": "High-volume bug report", + "title": "Busy bug", + "url": "https://github.com/openai/codex/issues/2", + "area": "agent", + "kind": "bug", + "state": "open", + "interactions": 17, + "user_interactions": 17, + "new_reactions": 3, + "new_upvotes": 2, + "current_reactions": 7, + } + + +def test_summary_inputs_are_model_ready_without_preclustering(): + issues = [ + { + "number": 20, + "title": "Windows app Browser Use external navigation fails", + "description": "Browser Use navigation or app-server failure", + "url": "https://github.com/openai/codex/issues/20", + "labels": ["app", "bug"], + "owner_labels": ["app"], + "kind_labels": ["bug"], + "attention": False, + "attention_level": 0, + "attention_marker": "", + "user_interactions": 3, + "new_reactions": 1, + "engagement_score": 8, + "updated_at": "2026-04-25T04:00:00Z", + "activity": {"new_comments": 2}, + }, + { + "number": 21, + "title": "On Windows, cmake output waits until timeout", + "description": "Windows command timeout/capture problem", + "url": "https://github.com/openai/codex/issues/21", + "labels": ["app", "bug"], + "owner_labels": ["app"], + "kind_labels": ["bug"], + "attention": False, + "attention_level": 0, + "attention_marker": "", + "user_interactions": 3, + "new_reactions": 0, + "engagement_score": 7, + "updated_at": "2026-04-25T03:00:00Z", + "activity": {"new_comments": 3}, + }, + { + "number": 22, + "title": "Windows computer use tool fails to click buttons", + "description": "Computer-use workflow failure", + "url": "https://github.com/openai/codex/issues/22", + "labels": ["app", "bug"], + "owner_labels": ["app"], + "kind_labels": ["bug"], + "attention": False, + "attention_level": 0, + "attention_marker": "", + "user_interactions": 3, + "new_reactions": 0, + "engagement_score": 6, + "updated_at": "2026-04-25T02:00:00Z", + "activity": {"new_comments": 3}, + }, + ] + + rows = collect_issue_digest.summary_inputs(issues, ref_map={20: 1, 21: 2, 22: 3}) + + assert rows == [ + { + "ref": 1, + "ref_markdown": "[1](https://github.com/openai/codex/issues/20)", + "number": 20, + "title": "Windows app Browser Use external navigation fails", + "description": "Browser Use navigation or app-server failure", + "url": "https://github.com/openai/codex/issues/20", + "labels": ["app", "bug"], + "owner_labels": ["app"], + "kind_labels": ["bug"], + "state": "", + "attention_marker": "", + "interactions": 3, + "new_comments": 2, + "new_reactions": 1, + "new_upvotes": 0, + "current_reactions": 0, + }, + { + "ref": 2, + "ref_markdown": "[2](https://github.com/openai/codex/issues/21)", + "number": 21, + "title": "On Windows, cmake output waits until timeout", + "description": "Windows command timeout/capture problem", + "url": "https://github.com/openai/codex/issues/21", + "labels": ["app", "bug"], + "owner_labels": ["app"], + "kind_labels": ["bug"], + "state": "", + "attention_marker": "", + "interactions": 3, + "new_comments": 3, + "new_reactions": 0, + "new_upvotes": 0, + "current_reactions": 0, + }, + { + "ref": 3, + "ref_markdown": "[3](https://github.com/openai/codex/issues/22)", + "number": 22, + "title": "Windows computer use tool fails to click buttons", + "description": "Computer-use workflow failure", + "url": "https://github.com/openai/codex/issues/22", + "labels": ["app", "bug"], + "owner_labels": ["app"], + "kind_labels": ["bug"], + "state": "", + "attention_marker": "", + "interactions": 3, + "new_comments": 3, + "new_reactions": 0, + "new_upvotes": 0, + "current_reactions": 0, + }, + ] From f8c527e5298f2cd047a12624133b24de1bf3829d Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 27 Apr 2026 13:31:56 +0200 Subject: [PATCH 02/13] multi_agent_v2: move thread cap into feature config (#19792) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Why `features.multi_agent_v2.max_concurrent_threads_per_session` is meant to be the MultiAgentV2-specific session thread cap: it counts the root thread and all open subagent threads. The previous implementation kept this surface tied to `agents.max_threads`, which made it a global subagent-only cap and allowed the legacy setting to coexist with MultiAgentV2. ## What Changed - Added `max_concurrent_threads_per_session` to `[features.multi_agent_v2]` with default `4`. - Removed the `[agents] max_concurrent_threads_per_session` alias to `agents.max_threads`. - When MultiAgentV2 is enabled, reject `agents.max_threads` and derive the existing internal subagent slot limit as `max_concurrent_threads_per_session - 1`. - Regenerated `core/config.schema.json` and added coverage for the new config semantics. ## Result ``` ➜ codex git:(jif/clean-multi-agent-v2-config) codex -c features.multi_agent_v2.enabled=true -c features.multi_agent_v2.max_concurrent_threads_per_session=3 ╭────────────────────────────────────────────────────╮ │ >_ OpenAI Codex (v0.0.0) │ │ │ │ model: gpt-5.5 xhigh fast /model to change │ │ directory: ~/code/codex │ ╰────────────────────────────────────────────────────╯ Tip: Update Required - This version will no longer be supported starting May 8th. Please upgrade to the latest version (https://github.com/openai/codex/releases/latest) using your preferred package manager. › Can you try to spawn 4 agents • I’ll try to start four lightweight agents at once and report exactly what the runtime accepts. • Spawned Russell [no-apps] (gpt-5.5 xhigh) └ Spawn probe 1: reply briefly that you started, then wait for further instructions. Do not do any repo work. • Spawned Descartes [no-apps] (gpt-5.5 xhigh) └ Spawn probe 2: reply briefly that you started, then wait for further instructions. Do not do any repo work. • Agent spawn failed └ Spawn probe 3: reply briefly that you started, then wait for further instructions. Do not do any repo work. • Agent spawn failed └ Spawn probe 4: reply briefly that you started, then wait for further instructions. Do not do any repo work. • The runtime accepted the first two and rejected the next two with agent thread limit reached. I’m checking whether the two accepted probes have returned cleanly, then I’ll close them if needed. ``` --------- Co-authored-by: Codex --- codex-rs/config/src/key_aliases.rs | 17 ++-- codex-rs/core/config.schema.json | 5 ++ codex-rs/core/src/config/config_tests.rs | 80 +++++++++++++++++++ codex-rs/core/src/config/mod.rs | 41 ++++++++-- codex-rs/core/src/session/turn_context.rs | 18 ++++- .../core/src/tools/handlers/agent_jobs.rs | 5 ++ codex-rs/features/src/feature_configs.rs | 3 + codex-rs/features/src/tests.rs | 3 + codex-rs/protocol/src/error.rs | 2 +- 9 files changed, 152 insertions(+), 22 deletions(-) diff --git a/codex-rs/config/src/key_aliases.rs b/codex-rs/config/src/key_aliases.rs index 8d417e269f..07cb44fa6d 100644 --- a/codex-rs/config/src/key_aliases.rs +++ b/codex-rs/config/src/key_aliases.rs @@ -8,18 +8,11 @@ struct ConfigKeyAlias { canonical_key: &'static str, } -const CONFIG_KEY_ALIASES: &[ConfigKeyAlias] = &[ - ConfigKeyAlias { - table_path: &["memories"], - legacy_key: "no_memories_if_mcp_or_web_search", - canonical_key: "disable_on_external_context", - }, - ConfigKeyAlias { - table_path: &["agents"], - legacy_key: "max_concurrent_threads_per_session", - canonical_key: "max_threads", - }, -]; +const CONFIG_KEY_ALIASES: &[ConfigKeyAlias] = &[ConfigKeyAlias { + table_path: &["memories"], + legacy_key: "no_memories_if_mcp_or_web_search", + canonical_key: "disable_on_external_context", +}]; pub(crate) fn normalize_key_aliases(path: &[String], table: &mut TomlMap) { for alias in CONFIG_KEY_ALIASES { diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 3fbbfaf6eb..5727a4bdcf 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -1310,6 +1310,11 @@ "hide_spawn_agent_metadata": { "type": "boolean" }, + "max_concurrent_threads_per_session": { + "format": "uint", + "minimum": 1.0, + "type": "integer" + }, "usage_hint_enabled": { "type": "boolean" }, diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index d0ea8980bf..995e4299c6 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -7348,6 +7348,7 @@ async fn multi_agent_v2_config_from_feature_table() -> std::io::Result<()> { codex_home.path().join(CONFIG_TOML_FILE), r#"[features.multi_agent_v2] enabled = true +max_concurrent_threads_per_session = 5 usage_hint_enabled = false usage_hint_text = "Custom delegation guidance." hide_spawn_agent_metadata = true @@ -7361,6 +7362,8 @@ hide_spawn_agent_metadata = true .await?; assert!(config.features.enabled(Feature::MultiAgentV2)); + assert_eq!(config.multi_agent_v2.max_concurrent_threads_per_session, 5); + assert_eq!(config.agent_max_threads, Some(4)); assert!(!config.multi_agent_v2.usage_hint_enabled); assert_eq!( config.multi_agent_v2.usage_hint_text.as_deref(), @@ -7379,11 +7382,13 @@ async fn profile_multi_agent_v2_config_overrides_base() -> std::io::Result<()> { r#"profile = "no_hint" [features.multi_agent_v2] +max_concurrent_threads_per_session = 4 usage_hint_enabled = true usage_hint_text = "base hint" hide_spawn_agent_metadata = true [profiles.no_hint.features.multi_agent_v2] +max_concurrent_threads_per_session = 6 usage_hint_enabled = false usage_hint_text = "profile hint" hide_spawn_agent_metadata = false @@ -7396,6 +7401,7 @@ hide_spawn_agent_metadata = false .build() .await?; + assert_eq!(config.multi_agent_v2.max_concurrent_threads_per_session, 6); assert!(!config.multi_agent_v2.usage_hint_enabled); assert_eq!( config.multi_agent_v2.usage_hint_text.as_deref(), @@ -7406,6 +7412,80 @@ hide_spawn_agent_metadata = false Ok(()) } +#[tokio::test] +async fn multi_agent_v2_default_session_thread_cap_counts_root() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + std::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#"[features.multi_agent_v2] +enabled = true +"#, + )?; + + let config = ConfigBuilder::without_managed_config_for_tests() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(codex_home.path().to_path_buf())) + .build() + .await?; + + assert_eq!(config.multi_agent_v2.max_concurrent_threads_per_session, 4); + assert_eq!(config.agent_max_threads, Some(3)); + + Ok(()) +} + +#[tokio::test] +async fn multi_agent_v2_rejects_agents_max_threads() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + std::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#"[features.multi_agent_v2] +enabled = true + +[agents] +max_threads = 3 +"#, + )?; + + let err = ConfigBuilder::without_managed_config_for_tests() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(codex_home.path().to_path_buf())) + .build() + .await + .expect_err("agents.max_threads should conflict with multi_agent_v2"); + + assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput); + assert_eq!( + err.to_string(), + "agents.max_threads cannot be set when multi_agent_v2 is enabled" + ); + + Ok(()) +} + +#[tokio::test] +async fn multi_agent_v2_session_thread_cap_one_disallows_subagents() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + std::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#"[features.multi_agent_v2] +enabled = true +max_concurrent_threads_per_session = 1 +"#, + )?; + + let config = ConfigBuilder::without_managed_config_for_tests() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(codex_home.path().to_path_buf())) + .build() + .await?; + + assert_eq!(config.multi_agent_v2.max_concurrent_threads_per_session, 1); + assert_eq!(config.agent_max_threads, Some(0)); + + Ok(()) +} + #[tokio::test] async fn feature_requirements_normalize_runtime_feature_mutations() -> std::io::Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 9635034dcc..ba21b2ea10 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -131,6 +131,7 @@ pub use codex_git_utils::GhostSnapshotConfig; /// the context window. pub(crate) const AGENTS_MD_MAX_BYTES: usize = 32 * 1024; // 32 KiB pub(crate) const DEFAULT_AGENT_MAX_THREADS: Option = Some(6); +pub(crate) const DEFAULT_MULTI_AGENT_V2_MAX_CONCURRENT_THREADS_PER_SESSION: usize = 4; pub(crate) const DEFAULT_AGENT_MAX_DEPTH: i32 = 1; pub(crate) const DEFAULT_AGENT_JOB_MAX_RUNTIME_SECONDS: Option = None; const LOCAL_DEV_BUILD_VERSION: &str = "0.0.0"; @@ -704,6 +705,7 @@ pub struct Config { #[derive(Debug, Clone, PartialEq, Eq)] pub struct MultiAgentV2Config { + pub max_concurrent_threads_per_session: usize, pub usage_hint_enabled: bool, pub usage_hint_text: Option, pub hide_spawn_agent_metadata: bool, @@ -712,6 +714,8 @@ pub struct MultiAgentV2Config { impl Default for MultiAgentV2Config { fn default() -> Self { Self { + max_concurrent_threads_per_session: + DEFAULT_MULTI_AGENT_V2_MAX_CONCURRENT_THREADS_PER_SESSION, usage_hint_enabled: true, usage_hint_text: None, hide_spawn_agent_metadata: false, @@ -1579,6 +1583,10 @@ fn resolve_multi_agent_v2_config( let profile = multi_agent_v2_toml_config(config_profile.features.as_ref()); let default = MultiAgentV2Config::default(); + let max_concurrent_threads_per_session = profile + .and_then(|config| config.max_concurrent_threads_per_session) + .or_else(|| base.and_then(|config| config.max_concurrent_threads_per_session)) + .unwrap_or(default.max_concurrent_threads_per_session); let usage_hint_enabled = profile .and_then(|config| config.usage_hint_enabled) .or_else(|| base.and_then(|config| config.usage_hint_enabled)) @@ -1594,6 +1602,7 @@ fn resolve_multi_agent_v2_config( .unwrap_or(default.hide_spawn_agent_metadata); MultiAgentV2Config { + max_concurrent_threads_per_session, usage_hint_enabled, usage_hint_text, hide_spawn_agent_metadata, @@ -2078,17 +2087,35 @@ impl Config { let history = cfg.history.unwrap_or_default(); - let agent_max_threads = cfg - .agents - .as_ref() - .and_then(|agents| agents.max_threads) - .or(DEFAULT_AGENT_MAX_THREADS); - if agent_max_threads == Some(0) { + if multi_agent_v2.max_concurrent_threads_per_session == 0 { return Err(std::io::Error::new( std::io::ErrorKind::InvalidInput, - "agents.max_threads must be at least 1", + "features.multi_agent_v2.max_concurrent_threads_per_session must be at least 1", )); } + let agent_max_threads_from_config = cfg.agents.as_ref().and_then(|agents| agents.max_threads); + let agent_max_threads = if features.enabled(Feature::MultiAgentV2) { + if agent_max_threads_from_config.is_some() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "agents.max_threads cannot be set when multi_agent_v2 is enabled", + )); + } + Some( + multi_agent_v2 + .max_concurrent_threads_per_session + .saturating_sub(1), + ) + } else { + let agent_max_threads = agent_max_threads_from_config.or(DEFAULT_AGENT_MAX_THREADS); + if agent_max_threads == Some(0) { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "agents.max_threads must be at least 1", + )); + } + agent_max_threads + }; let agent_max_depth = cfg .agents .as_ref() diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index 383d80292c..24777f62e9 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -194,7 +194,12 @@ impl TurnContext { .with_spawn_agent_usage_hint_text(config.multi_agent_v2.usage_hint_text.clone()) .with_hide_spawn_agent_metadata(config.multi_agent_v2.hide_spawn_agent_metadata) .with_goal_tools_allowed(self.tools_config.goal_tools) - .with_max_concurrent_threads_per_session(config.agent_max_threads) + .with_max_concurrent_threads_per_session( + config + .features + .enabled(Feature::MultiAgentV2) + .then_some(config.multi_agent_v2.max_concurrent_threads_per_session), + ) .with_agent_type_description(crate::agent::role::spawn_tool_spec::build( &config.agent_roles, )); @@ -459,7 +464,16 @@ impl Session { .with_spawn_agent_usage_hint_text(per_turn_config.multi_agent_v2.usage_hint_text.clone()) .with_hide_spawn_agent_metadata(per_turn_config.multi_agent_v2.hide_spawn_agent_metadata) .with_goal_tools_allowed(goal_tools_supported) - .with_max_concurrent_threads_per_session(per_turn_config.agent_max_threads) + .with_max_concurrent_threads_per_session( + per_turn_config + .features + .enabled(Feature::MultiAgentV2) + .then_some( + per_turn_config + .multi_agent_v2 + .max_concurrent_threads_per_session, + ), + ) .with_agent_type_description(crate::agent::role::spawn_tool_spec::build( &per_turn_config.agent_roles, )); diff --git a/codex-rs/core/src/tools/handlers/agent_jobs.rs b/codex-rs/core/src/tools/handlers/agent_jobs.rs index adf777fff7..bb5b82190a 100644 --- a/codex-rs/core/src/tools/handlers/agent_jobs.rs +++ b/codex-rs/core/src/tools/handlers/agent_jobs.rs @@ -534,6 +534,11 @@ async fn build_runner_options( "agent depth limit reached; this session cannot spawn more subagents".to_string(), )); } + if turn.config.agent_max_threads == Some(0) { + return Err(FunctionCallError::RespondToModel( + "agent thread limit reached; this session cannot spawn more subagents".to_string(), + )); + } let max_concurrency = normalize_concurrency(requested_concurrency, turn.config.agent_max_threads); let base_instructions = session.get_base_instructions().await; diff --git a/codex-rs/features/src/feature_configs.rs b/codex-rs/features/src/feature_configs.rs index 0db4e4e82e..bead1ce037 100644 --- a/codex-rs/features/src/feature_configs.rs +++ b/codex-rs/features/src/feature_configs.rs @@ -9,6 +9,9 @@ pub struct MultiAgentV2ConfigToml { #[serde(skip_serializing_if = "Option::is_none")] pub enabled: Option, #[serde(skip_serializing_if = "Option::is_none")] + #[schemars(range(min = 1))] + pub max_concurrent_threads_per_session: Option, + #[serde(skip_serializing_if = "Option::is_none")] pub usage_hint_enabled: Option, #[serde(skip_serializing_if = "Option::is_none")] pub usage_hint_text: Option, diff --git a/codex-rs/features/src/tests.rs b/codex-rs/features/src/tests.rs index e410159b7f..ca05d72d2d 100644 --- a/codex-rs/features/src/tests.rs +++ b/codex-rs/features/src/tests.rs @@ -396,6 +396,7 @@ fn multi_agent_v2_feature_config_deserializes_table() { r#" [multi_agent_v2] enabled = true +max_concurrent_threads_per_session = 4 usage_hint_enabled = false usage_hint_text = "Custom delegation guidance." hide_spawn_agent_metadata = true @@ -411,6 +412,7 @@ hide_spawn_agent_metadata = true features.multi_agent_v2, Some(crate::FeatureToml::Config(crate::MultiAgentV2ConfigToml { enabled: Some(true), + max_concurrent_threads_per_session: Some(4), usage_hint_enabled: Some(false), usage_hint_text: Some("Custom delegation guidance.".to_string()), hide_spawn_agent_metadata: Some(true), @@ -442,6 +444,7 @@ usage_hint_enabled = false features_toml.multi_agent_v2, Some(crate::FeatureToml::Config(crate::MultiAgentV2ConfigToml { enabled: None, + max_concurrent_threads_per_session: None, usage_hint_enabled: Some(false), usage_hint_text: None, hide_spawn_agent_metadata: None, diff --git a/codex-rs/protocol/src/error.rs b/codex-rs/protocol/src/error.rs index b99a994705..207fd94ca2 100644 --- a/codex-rs/protocol/src/error.rs +++ b/codex-rs/protocol/src/error.rs @@ -82,7 +82,7 @@ pub enum CodexErr { ContextWindowExceeded, #[error("no thread with id: {0}")] ThreadNotFound(ThreadId), - #[error("agent thread limit reached (max {max_threads})")] + #[error("agent thread limit reached")] AgentLimitReached { max_threads: usize }, #[error("session configured event was not the first event in the stream")] SessionConfiguredNotFirstEvent, From 01ab25dbb5ffa5868266df0a7b870a601e19a2cd Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 27 Apr 2026 14:32:44 +0200 Subject: [PATCH 03/13] feat: use git-backed workspace diffs for memory consolidation (#18982) ## Why This PR make the `morpheus` agent (memory phase 2) use a git diff to start it's consolidation. The workflow is the following: 1. The agent acquire a lock 2. If `.codex/memories` does not exist or is not a git root, initialize everything (and make a first empty commit) 3. Update `raw_memories.md` and `rollout_summaries/` as before. Basically we select max N phase 1 memories based on a given policy 4. We use git (`gix`) to get a diff between the current state of `.codex/memories` and the last commit. 5. Dump the diff in `phase2_workspace_diff.md` 6. Spawn `morpheus` and point it to `phase2_workspace_diff.md` 7. Wait for `morpheus` to be done 8. Re-create a new `.git` and make one single commit on it. We do this because we don't want to preserve history through `.git` and this is cheap anyway 9. We release the lock On top of this, we keep the retry policies etc etc The goals of this new workflow are: * Better support of any memory extensions such as `chronicle` * Allow the user to manually edit memories and this will be considered by the phase 2 agent As a follow-up we will need to add support for user's edition while `morpheus` is running ## What Changed - Added memory workspace helpers that prepare the git baseline, compute the diff, write `phase2_workspace_diff.md`, and reset the baseline after successful consolidation. - Updated Phase 2 to sync current inputs into `raw_memories.md` and `rollout_summaries/`, prune old extension resources, skip clean workspaces, and run the consolidation subagent only when the workspace has changes. - Tightened Phase 2 job ownership around long-running consolidation with heartbeats and an ownership check before resetting the baseline. - Simplified the prompt and state APIs so DB watermarks are bookkeeping, while workspace dirtiness decides whether consolidation work exists. - Updated the memory pipeline README and tests for workspace diffs, extension-resource cleanup, pollution-driven forgetting, selection ranking, and baseline persistence. ## Verification - Added/updated coverage in `core/src/memories/tests.rs`, `core/src/memories/workspace_tests.rs`, `state/src/runtime/memories.rs`, and `core/tests/suite/memories.rs`. --------- Co-authored-by: Codex --- codex-rs/core/src/memories/README.md | 60 +-- codex-rs/core/src/memories/extensions.rs | 188 +-------- .../core/src/memories/extensions_tests.rs | 81 ++++ codex-rs/core/src/memories/mod.rs | 8 +- codex-rs/core/src/memories/phase2.rs | 183 +++++---- codex-rs/core/src/memories/prompts.rs | 122 +----- codex-rs/core/src/memories/prompts_tests.rs | 36 +- codex-rs/core/src/memories/storage.rs | 18 - codex-rs/core/src/memories/tests.rs | 360 +++++++++++++++--- codex-rs/core/src/memories/workspace.rs | 124 ++++++ codex-rs/core/src/memories/workspace_tests.rs | 78 ++++ .../core/templates/memories/consolidation.md | 70 ++-- codex-rs/core/tests/suite/memories.rs | 344 +++-------------- codex-rs/git-utils/README.md | 8 +- codex-rs/git-utils/src/baseline.rs | 74 +++- codex-rs/git-utils/src/lib.rs | 1 + codex-rs/state/src/lib.rs | 2 - codex-rs/state/src/model/memories.rs | 33 +- codex-rs/state/src/model/mod.rs | 3 - codex-rs/state/src/runtime/memories.rs | 341 +++++++---------- codex-rs/tui/src/app/tests.rs | 3 +- 21 files changed, 1079 insertions(+), 1058 deletions(-) create mode 100644 codex-rs/core/src/memories/extensions_tests.rs create mode 100644 codex-rs/core/src/memories/workspace.rs create mode 100644 codex-rs/core/src/memories/workspace_tests.rs diff --git a/codex-rs/core/src/memories/README.md b/codex-rs/core/src/memories/README.md index a1d365435b..8a885fd864 100644 --- a/codex-rs/core/src/memories/README.md +++ b/codex-rs/core/src/memories/README.md @@ -70,7 +70,8 @@ Phase 2 consolidates the latest stage-1 outputs into the filesystem memory artif What it does: -- claims a single global phase-2 job (so only one consolidation runs at a time) +- claims a single global phase-2 lock before touching the memories root (so only one consolidation + inspects or mutates the workspace at a time) - loads a bounded set of stage-1 outputs from the state DB using phase-2 selection rules: - ignores memories whose `last_usage` falls outside the configured @@ -82,53 +83,58 @@ What it does: - computes a completion watermark from the claimed watermark + newest input timestamps - syncs local memory artifacts under the memories root: - `raw_memories.md` (merged raw memories, latest first) - - `rollout_summaries/` (one summary file per retained rollout) -- prunes stale rollout summaries that are no longer retained -- finds old resource files from memory extensions under - `memories_extensions//resources/` for extension directories that - have an `instructions.md`, using the memory module retention window -- if there are no Phase 1 inputs or old extension resources, marks the job - successful and exits + - `rollout_summaries/` (one summary file per selected rollout) +- keeps the memories root itself as a git-baseline directory, initialized under + `~/.codex/memories/.git` by `codex-git-utils` +- prunes stale rollout summaries that are no longer selected +- prunes memory extension resource files older than the extension retention + window, so cleanup appears in the workspace diff +- writes `phase2_workspace_diff.md` in the memories root with the git-style diff + from the previous successful Phase 2 baseline to the current worktree +- if the memory workspace has no changes after artifact sync/pruning, marks the + job successful and exits -If there is input, it then: +If the memory workspace has changes, it then: - spawns an internal consolidation sub-agent -- builds the Phase 2 prompt with a diff of the current Phase 1 input - selection versus the last successful Phase 2 selection (`added`, - `retained`, `removed`) -- includes old extension resource paths in the prompt diff +- builds the Phase 2 prompt with the path to the generated workspace diff +- points the agent at `phase2_workspace_diff.md` for the detailed diff context - runs it with no approvals, no network, and local write access only - disables collab for that agent (to prevent recursive delegation) - watches the agent status and heartbeats the global job lease while it runs +- resets the memory git baseline after the agent completes successfully; the + generated diff file is removed before this reset so deleted content is not + kept in the prompt artifact or unreachable git objects - marks the phase-2 job success/failure in the state DB when the agent finishes -- prunes old extension resource files after the consolidation agent completes - and the successful Phase 2 job is recorded -Selection diff behavior: +Selection and workspace-diff behavior: - successful Phase 2 runs mark the exact stage-1 snapshots they consumed with `selected_for_phase2 = 1` and persist the matching `selected_for_phase2_source_updated_at` - Phase 1 upserts preserve the previous `selected_for_phase2` baseline until the next successful Phase 2 run rewrites it -- the next Phase 2 run compares the current top-N stage-1 inputs against that - prior snapshot selection to label inputs as `added` or `retained`; a - refreshed thread stays `added` until Phase 2 successfully selects its newer - snapshot -- rows that were previously selected but still exist outside the current top-N - selection are surfaced as `removed` -- before the agent starts, local `rollout_summaries/` and `raw_memories.md` - keep the union of the current selection and the previous successful - selection, so removed-thread evidence stays available during forgetting +- Phase 2 loads only the current top-N selected stage-1 inputs, syncs + `rollout_summaries/` and `raw_memories.md` directly to that selection, then + lets the git-style workspace diff surface additions, modifications, and + deletions against the previous successful memory baseline +- when the selected input set is empty, stale `rollout_summaries/` files are + removed and `raw_memories.md` is rewritten to the empty-input placeholder; + consolidated outputs such as `MEMORY.md`, `memory_summary.md`, and `skills/` + are left for the agent to update Watermark behavior: -- The global phase-2 job claim includes an input watermark representing the latest input timestamp known when the job was claimed. +- The global phase-2 lock does not use DB watermarks as a dirty check; git + workspace dirtiness decides whether an agent needs to run. +- The global phase-2 job row still tracks an input watermark as bookkeeping + for the latest DB input timestamp known when the job was claimed. - Phase 2 recomputes a `new_watermark` using the max of: - the claimed watermark - the newest `source_updated_at` timestamp in the stage-1 inputs it actually loaded - On success, Phase 2 stores that completion watermark in the DB. -- This lets later phase-2 runs know whether new stage-1 data arrived since the last successful consolidation (dirty vs not dirty), while also avoiding moving the watermark backwards. +- This avoids moving the recorded completion watermark backwards, but does not + decide whether Phase 2 has work. In practice, this phase is responsible for refreshing the on-disk memory workspace and producing/updating the higher-level consolidated memory outputs. diff --git a/codex-rs/core/src/memories/extensions.rs b/codex-rs/core/src/memories/extensions.rs index 458197609f..f2586c5323 100644 --- a/codex-rs/core/src/memories/extensions.rs +++ b/codex-rs/core/src/memories/extensions.rs @@ -4,46 +4,27 @@ use chrono::Duration; use chrono::NaiveDateTime; use chrono::Utc; use std::path::Path; -use std::path::PathBuf; use tracing::warn; const FILENAME_TS_FORMAT: &str = "%Y-%m-%dT%H-%M-%S"; pub(super) const EXTENSION_RESOURCE_RETENTION_DAYS: i64 = 7; -#[derive(Debug, Clone, PartialEq, Eq)] -pub(super) struct RemovedExtensionResource { - pub(super) extension: String, - pub(super) resource_path: String, +pub(super) async fn prune_old_extension_resources(memory_root: &Path) { + prune_old_extension_resources_with_now(memory_root, Utc::now()).await } -#[derive(Debug, Clone, PartialEq, Eq)] -pub(super) struct PendingExtensionResourceRemoval { - pub(super) removed: RemovedExtensionResource, - path: PathBuf, -} - -pub(super) async fn find_old_extension_resources( - memory_root: &Path, -) -> Vec { - find_old_extension_resources_with_now(memory_root, Utc::now()).await -} - -async fn find_old_extension_resources_with_now( - memory_root: &Path, - now: DateTime, -) -> Vec { - let mut pending = Vec::new(); +async fn prune_old_extension_resources_with_now(memory_root: &Path, now: DateTime) { let cutoff = now - Duration::days(EXTENSION_RESOURCE_RETENTION_DAYS); let extensions_root = memory_extensions_root(memory_root); let mut extensions = match tokio::fs::read_dir(&extensions_root).await { Ok(extensions) => extensions, - Err(err) if err.kind() == std::io::ErrorKind::NotFound => return pending, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return, Err(err) => { warn!( "failed reading memory extensions root {}: {err}", extensions_root.display() ); - return pending; + return; } }; @@ -52,19 +33,10 @@ async fn find_old_extension_resources_with_now( let Ok(file_type) = extension_entry.file_type().await else { continue; }; - if !file_type.is_dir() { - continue; - } - let Some(extension) = extension_path - .file_name() - .and_then(|name| name.to_str()) - .map(ToOwned::to_owned) - else { - continue; - }; - if !tokio::fs::try_exists(extension_path.join("instructions.md")) - .await - .unwrap_or(false) + if !file_type.is_dir() + || !tokio::fs::try_exists(extension_path.join("instructions.md")) + .await + .unwrap_or(false) { continue; } @@ -106,34 +78,14 @@ async fn find_old_extension_resources_with_now( continue; } - pending.push(PendingExtensionResourceRemoval { - removed: RemovedExtensionResource { - extension: extension.clone(), - resource_path: format!("resources/{file_name}"), - }, - path: resource_file_path, - }); - } - } - - pending.sort_by(|left, right| { - left.removed - .extension - .cmp(&right.removed.extension) - .then_with(|| left.removed.resource_path.cmp(&right.removed.resource_path)) - }); - pending -} - -pub(super) async fn remove_extension_resources(resources: &[PendingExtensionResourceRemoval]) { - for resource in resources { - if let Err(err) = tokio::fs::remove_file(&resource.path).await - && err.kind() != std::io::ErrorKind::NotFound - { - warn!( - "failed pruning old memory extension resource {}: {err}", - resource.path.display() - ); + if let Err(err) = tokio::fs::remove_file(&resource_file_path).await + && err.kind() != std::io::ErrorKind::NotFound + { + warn!( + "failed pruning old memory extension resource {}: {err}", + resource_file_path.display() + ); + } } } } @@ -145,107 +97,5 @@ fn resource_timestamp(file_name: &str) -> Option> { } #[cfg(test)] -mod tests { - use super::*; - use pretty_assertions::assert_eq; - use tempfile::TempDir; - - #[tokio::test] - async fn finds_only_old_resources_from_extensions_with_instructions() { - let codex_home = TempDir::new().expect("create temp codex home"); - let memory_root = codex_home.path().join("memories"); - let extensions_root = memory_extensions_root(&memory_root); - let chronicle_resources = extensions_root.join("chronicle/resources"); - tokio::fs::create_dir_all(&chronicle_resources) - .await - .expect("create chronicle resources"); - tokio::fs::write( - extensions_root.join("chronicle/instructions.md"), - "instructions", - ) - .await - .expect("write chronicle instructions"); - - let now = DateTime::from_naive_utc_and_offset( - NaiveDateTime::parse_from_str("2026-04-14T12-00-00", FILENAME_TS_FORMAT) - .expect("parse now"), - Utc, - ); - let old_file = chronicle_resources.join("2026-04-06T11-59-59-abcd-10min-old.md"); - let exact_cutoff_file = - chronicle_resources.join("2026-04-07T12-00-00-abcd-10min-cutoff.md"); - let recent_file = chronicle_resources.join("2026-04-08T12-00-00-abcd-10min-recent.md"); - let invalid_file = chronicle_resources.join("not-a-timestamp.md"); - for file in [&old_file, &exact_cutoff_file, &recent_file, &invalid_file] { - tokio::fs::write(file, "resource") - .await - .expect("write chronicle resource"); - } - - let ignored_resources = extensions_root.join("ignored/resources"); - tokio::fs::create_dir_all(&ignored_resources) - .await - .expect("create ignored resources"); - let ignored_old_file = ignored_resources.join("2026-04-06T11-59-59-abcd-10min-old.md"); - tokio::fs::write(&ignored_old_file, "ignored") - .await - .expect("write ignored resource"); - - let pending = find_old_extension_resources_with_now(&memory_root, now).await; - - assert_eq!( - pending - .iter() - .map(|resource| resource.removed.clone()) - .collect::>(), - vec![ - RemovedExtensionResource { - extension: "chronicle".to_string(), - resource_path: "resources/2026-04-06T11-59-59-abcd-10min-old.md".to_string(), - }, - RemovedExtensionResource { - extension: "chronicle".to_string(), - resource_path: "resources/2026-04-07T12-00-00-abcd-10min-cutoff.md".to_string(), - }, - ] - ); - assert!( - tokio::fs::try_exists(&old_file) - .await - .expect("check old file before remove") - ); - assert!( - tokio::fs::try_exists(&exact_cutoff_file) - .await - .expect("check cutoff file before remove") - ); - - remove_extension_resources(&pending).await; - - assert!( - !tokio::fs::try_exists(&old_file) - .await - .expect("check old file") - ); - assert!( - !tokio::fs::try_exists(&exact_cutoff_file) - .await - .expect("check cutoff file") - ); - assert!( - tokio::fs::try_exists(&recent_file) - .await - .expect("check recent file") - ); - assert!( - tokio::fs::try_exists(&invalid_file) - .await - .expect("check invalid file") - ); - assert!( - tokio::fs::try_exists(&ignored_old_file) - .await - .expect("check ignored old file") - ); - } -} +#[path = "extensions_tests.rs"] +mod tests; diff --git a/codex-rs/core/src/memories/extensions_tests.rs b/codex-rs/core/src/memories/extensions_tests.rs new file mode 100644 index 0000000000..60cd187579 --- /dev/null +++ b/codex-rs/core/src/memories/extensions_tests.rs @@ -0,0 +1,81 @@ +use super::*; +use pretty_assertions::assert_eq; +use tempfile::TempDir; + +#[tokio::test] +async fn prunes_only_old_resources_from_extensions_with_instructions() { + let codex_home = TempDir::new().expect("create temp codex home"); + let memory_root = codex_home.path().join("memories"); + let extensions_root = memory_extensions_root(&memory_root); + let chronicle_resources = extensions_root.join("chronicle/resources"); + tokio::fs::create_dir_all(&chronicle_resources) + .await + .expect("create chronicle resources"); + tokio::fs::write( + extensions_root.join("chronicle/instructions.md"), + "instructions", + ) + .await + .expect("write chronicle instructions"); + + let now = DateTime::from_naive_utc_and_offset( + NaiveDateTime::parse_from_str("2026-04-14T12-00-00", FILENAME_TS_FORMAT) + .expect("parse now"), + Utc, + ); + let old_file = chronicle_resources.join("2026-04-06T11-59-59-abcd-10min-old.md"); + let exact_cutoff_file = chronicle_resources.join("2026-04-07T12-00-00-abcd-10min-cutoff.md"); + let recent_file = chronicle_resources.join("2026-04-08T12-00-00-abcd-10min-recent.md"); + let invalid_file = chronicle_resources.join("not-a-timestamp.md"); + for file in [&old_file, &exact_cutoff_file, &recent_file, &invalid_file] { + tokio::fs::write(file, "resource") + .await + .expect("write chronicle resource"); + } + + let ignored_resources = extensions_root.join("ignored/resources"); + tokio::fs::create_dir_all(&ignored_resources) + .await + .expect("create ignored resources"); + let ignored_old_file = ignored_resources.join("2026-04-06T11-59-59-abcd-10min-old.md"); + tokio::fs::write(&ignored_old_file, "ignored") + .await + .expect("write ignored resource"); + + prune_old_extension_resources_with_now(&memory_root, now).await; + + assert!( + !tokio::fs::try_exists(&old_file) + .await + .expect("check old file") + ); + assert!( + !tokio::fs::try_exists(&exact_cutoff_file) + .await + .expect("check cutoff file") + ); + assert!( + tokio::fs::try_exists(&recent_file) + .await + .expect("check recent file") + ); + assert!( + tokio::fs::try_exists(&invalid_file) + .await + .expect("check invalid file") + ); + assert!( + tokio::fs::try_exists(&ignored_old_file) + .await + .expect("check ignored file") + ); +} + +#[test] +fn parses_timestamp_prefix_from_resource_file_name() { + let parsed = resource_timestamp("2026-04-06T11-59-59-abcd-10min-old.md") + .expect("timestamp should parse"); + + assert_eq!(parsed.timestamp(), 1_775_476_799); + assert!(resource_timestamp("not-a-timestamp.md").is_none()); +} diff --git a/codex-rs/core/src/memories/mod.rs b/codex-rs/core/src/memories/mod.rs index d796063d2d..023ea9913a 100644 --- a/codex-rs/core/src/memories/mod.rs +++ b/codex-rs/core/src/memories/mod.rs @@ -6,6 +6,7 @@ pub(crate) mod citations; mod control; +mod extensions; mod phase1; mod phase2; pub(crate) mod prompts; @@ -14,6 +15,7 @@ mod storage; #[cfg(test)] mod tests; pub(crate) mod usage; +mod workspace; use codex_protocol::openai_models::ReasoningEffort; @@ -25,13 +27,11 @@ pub use control::clear_memory_roots_contents; pub(crate) use start::start_memories_startup_task; mod artifacts { - pub(super) const EXTENSIONS_SUBDIR: &str = "memories_extensions"; + pub(super) const EXTENSIONS_SUBDIR: &str = "extensions"; pub(super) const ROLLOUT_SUMMARIES_SUBDIR: &str = "rollout_summaries"; pub(super) const RAW_MEMORIES_FILENAME: &str = "raw_memories.md"; } -mod extensions; - /// Phase 1 (startup extraction). mod phase_one { /// Default model used for phase 1. @@ -111,7 +111,7 @@ fn rollout_summaries_dir(root: &Path) -> PathBuf { } fn memory_extensions_root(root: &Path) -> PathBuf { - root.with_file_name(artifacts::EXTENSIONS_SUBDIR) + root.join(artifacts::EXTENSIONS_SUBDIR) } fn raw_memories_file(root: &Path) -> PathBuf { diff --git a/codex-rs/core/src/memories/phase2.rs b/codex-rs/core/src/memories/phase2.rs index 248e61dbab..0b7ffd6130 100644 --- a/codex-rs/core/src/memories/phase2.rs +++ b/codex-rs/core/src/memories/phase2.rs @@ -1,16 +1,17 @@ use crate::agent::AgentStatus; use crate::agent::status::is_final as is_final_agent_status; use crate::config::Config; -use crate::memories::extensions::PendingExtensionResourceRemoval; -use crate::memories::extensions::find_old_extension_resources; -use crate::memories::extensions::remove_extension_resources; +use crate::memories::extensions::prune_old_extension_resources; use crate::memories::memory_root; use crate::memories::metrics; use crate::memories::phase_two; use crate::memories::prompts::build_consolidation_prompt; use crate::memories::storage::rebuild_raw_memories_file_from_memories; -use crate::memories::storage::rollout_summary_file_stem; use crate::memories::storage::sync_rollout_summaries_from_memories; +use crate::memories::workspace::memory_workspace_diff; +use crate::memories::workspace::prepare_memory_workspace; +use crate::memories::workspace::reset_memory_workspace_baseline; +use crate::memories::workspace::write_workspace_diff; use crate::session::emit_subagent_session_started; use crate::session::session::Session; use codex_config::Constrained; @@ -25,7 +26,7 @@ use codex_protocol::user_input::UserInput; use codex_state::Stage1Output; use codex_state::StateRuntime; use std::collections::HashMap; -use std::collections::HashSet; +use std::path::Path; use std::sync::Arc; use std::time::Duration; use tokio::sync::watch; @@ -59,7 +60,7 @@ pub(super) async fn run(session: &Arc, config: Arc) { let max_raw_memories = config.memories.max_raw_memories_for_consolidation; let max_unused_days = config.memories.max_unused_days; - // 1. Claim the job. + // 1. Claim the global Phase 2 lock before touching the memory workspace. let claim = match job::claim(session, db).await { Ok(claim) => claim, Err(e) => { @@ -72,71 +73,76 @@ pub(super) async fn run(session: &Arc, config: Arc) { } }; - // 2. Get the config for the agent - let Some(agent_config) = agent::get_config(config.clone()) else { + // 2. Ensure the memories root has a git baseline repository. + if let Err(err) = prepare_memory_workspace(&root).await { + tracing::error!("failed preparing memory workspace: {err}"); + job::failed(session, db, &claim, "failed_prepare_workspace").await; + return; + } + + // 3. Build the locked-down config used by the consolidation agent. + let Some(agent_config) = agent::get_config(config.as_ref()) else { // If we can't get the config, we can't consolidate. tracing::error!("failed to get agent config"); job::failed(session, db, &claim, "failed_sandbox_policy").await; return; }; - // 3. Query the memories - let selection = match db + // 4. Load current DB-backed Phase 2 inputs. + let raw_memories = match db .get_phase2_input_selection(max_raw_memories, max_unused_days) .await { - Ok(selection) => selection, + Ok(raw_memories) => raw_memories, Err(err) => { - tracing::error!("failed to list stage1 outputs from global: {}", err); + tracing::error!("failed to list stage1 outputs from global: {err}"); job::failed(session, db, &claim, "failed_load_stage1_outputs").await; return; } }; - let raw_memories = selection.selected.to_vec(); - let artifact_memories = artifact_memories_for_phase2(&selection); + let raw_memory_count = raw_memories.len(); let new_watermark = get_watermark(claim.watermark, &raw_memories); - // 4. Update the file system by syncing the raw memories with the one extracted from DB at - // step 3 - // [`rollout_summaries/`] - if let Err(err) = - sync_rollout_summaries_from_memories(&root, &artifact_memories, artifact_memories.len()) - .await - { - tracing::error!("failed syncing local memory artifacts for global consolidation: {err}"); - job::failed(session, db, &claim, "failed_sync_artifacts").await; + // 5. Sync the current inputs into the memory workspace. + if let Err(err) = sync_phase2_workspace_inputs(&root, &raw_memories).await { + tracing::error!("failed syncing phase2 workspace inputs: {err}"); + job::failed(session, db, &claim, "failed_sync_workspace_inputs").await; return; } - // [`raw_memories.md`] - if let Err(err) = - rebuild_raw_memories_file_from_memories(&root, &artifact_memories, artifact_memories.len()) - .await - { - tracing::error!("failed syncing local memory artifacts for global consolidation: {err}"); - job::failed(session, db, &claim, "failed_rebuild_raw_memories").await; - return; - } - let pending_extension_resource_removals = find_old_extension_resources(&root).await; - let removed_extension_resources = pending_extension_resource_removals - .iter() - .map(|resource| resource.removed.clone()) - .collect::>(); - if raw_memories.is_empty() && pending_extension_resource_removals.is_empty() { + + // 6. Use git to decide whether the synced workspace actually changed. + let workspace_diff = match memory_workspace_diff(&root).await { + Ok(diff) => diff, + Err(err) => { + tracing::error!("failed checking memory workspace changes: {err}"); + job::failed(session, db, &claim, "failed_workspace_status").await; + return; + } + }; + if !workspace_diff.has_changes() { + tracing::error!("Phase 2 no changes"); // We check only after sync of the file system. job::succeed( session, db, &claim, new_watermark, - &[], - "succeeded_no_input", + &raw_memories, + "succeeded_no_workspace_changes", ) .await; return; } - // 5. Spawn the agent - let prompt = agent::get_prompt(config, &selection, &removed_extension_resources); + // 7. Persist the diff for the consolidation agent to inspect. + if let Err(err) = write_workspace_diff(&root, &workspace_diff).await { + tracing::error!("failed writing memory workspace diff file: {err}"); + job::failed(session, db, &claim, "failed_workspace_diff_file").await; + return; + } + + // 8. Spawn the consolidation agent. + let prompt = agent::get_prompt(&root); let source = SessionSource::SubAgent(SubAgentSource::MemoryConsolidation); let agent_control = session.services.agent_control.detached_registry(); let thread_id = match agent_control @@ -172,39 +178,34 @@ pub(super) async fn run(session: &Arc, config: Arc) { warn!("failed to load memory consolidation thread config for analytics: {thread_id}"); } - // 6. Spawn the agent handler. + // 9. Hand off completion handling, heartbeats, and baseline reset. agent::handle( session, claim, new_watermark, raw_memories.clone(), - pending_extension_resource_removals, + root, thread_id, agent_control, phase_two_e2e_timer, ); - // 7. Metrics and logs. + // 10. Emit dispatch metrics. let counters = Counters { - input: raw_memories.len() as i64, + input: raw_memory_count as i64, }; emit_metrics(session, counters); } -fn artifact_memories_for_phase2( - selection: &codex_state::Phase2InputSelection, -) -> Vec { - let mut seen = HashSet::new(); - let mut memories = selection.selected.clone(); - for memory in &selection.selected { - seen.insert(rollout_summary_file_stem(memory)); - } - for memory in &selection.previous_selected { - if seen.insert(rollout_summary_file_stem(memory)) { - memories.push(memory.clone()); - } - } - memories +async fn sync_phase2_workspace_inputs( + root: &Path, + raw_memories: &[Stage1Output], +) -> std::io::Result<()> { + let raw_memory_count = raw_memories.len(); + sync_rollout_summaries_from_memories(root, raw_memories, raw_memory_count).await?; + rebuild_raw_memories_file_from_memories(root, raw_memories, raw_memory_count).await?; + prune_old_extension_resources(root).await; + Ok(()) } mod job { @@ -234,7 +235,9 @@ mod job { ); (ownership_token, input_watermark) } - codex_state::Phase2JobClaimOutcome::SkippedNotDirty => return Err("skipped_not_dirty"), + codex_state::Phase2JobClaimOutcome::SkippedRetryUnavailable => { + return Err("skipped_retry_unavailable"); + } codex_state::Phase2JobClaimOutcome::SkippedRunning => return Err("skipped_running"), }; @@ -293,9 +296,9 @@ mod job { mod agent { use super::*; - pub(super) fn get_config(config: Arc) -> Option { + pub(super) fn get_config(config: &Config) -> Option { let root = memory_root(&config.codex_home); - let mut agent_config = config.as_ref().clone(); + let mut agent_config = config.clone(); agent_config.cwd = root.clone(); // Consolidation threads must never feed back into phase-1 memory generation. @@ -342,13 +345,8 @@ mod agent { Some(agent_config) } - pub(super) fn get_prompt( - config: Arc, - selection: &codex_state::Phase2InputSelection, - removed_extension_resources: &[crate::memories::extensions::RemovedExtensionResource], - ) -> Vec { - let root = memory_root(&config.codex_home); - let prompt = build_consolidation_prompt(&root, selection, removed_extension_resources); + pub(super) fn get_prompt(root: &Path) -> Vec { + let prompt = build_consolidation_prompt(root); vec![UserInput::Text { text: prompt, text_elements: vec![], @@ -362,7 +360,7 @@ mod agent { claim: Claim, new_watermark: i64, selected_outputs: Vec, - pending_extension_resource_removals: Vec, + memory_root: codex_utils_absolute_path::AbsolutePathBuf, thread_id: ThreadId, agent_control: crate::agent::AgentControl, phase_two_e2e_timer: Option, @@ -386,20 +384,38 @@ mod agent { }; // Loop the agent until we have the final status. - let final_status = loop_agent( - db.clone(), - claim.token.clone(), - new_watermark, - thread_id, - rx, - ) - .await; + let final_status = loop_agent(db.clone(), claim.token.clone(), thread_id, rx).await; if matches!(final_status, AgentStatus::Completed(_)) { if let Some(token_usage) = agent_control.get_total_token_usage(thread_id).await { emit_token_usage_metrics(&session, &token_usage); } - if job::succeed( + // Do not reset the workspace baseline if we lost the lock. + let Ok(still_owns_lock) = db + .heartbeat_global_phase2_job(&claim.token, phase_two::JOB_LEASE_SECONDS) + .await + .inspect_err(|err| { + tracing::error!( + "failed confirming global memory consolidation ownership before resetting workspace baseline: {err}" + ); + }) + else { + job::failed(&session, &db, &claim, "failed_confirm_ownership").await; + return; + }; + if !still_owns_lock { + tracing::error!( + "lost global memory consolidation ownership before resetting workspace baseline" + ); + return; + } + + if let Err(err) = reset_memory_workspace_baseline(&memory_root).await { + tracing::error!("failed resetting memory workspace baseline: {err}"); + job::failed(&session, &db, &claim, "failed_workspace_commit").await; + return; + } + if !job::succeed( &session, &db, &claim, @@ -409,7 +425,9 @@ mod agent { ) .await { - remove_extension_resources(&pending_extension_resource_removals).await; + tracing::error!( + "failed marking global memory consolidation job succeeded after resetting workspace baseline" + ); } } else { job::failed(&session, &db, &claim, "failed_agent").await; @@ -433,7 +451,6 @@ mod agent { async fn loop_agent( db: Arc, token: String, - _new_watermark: i64, thread_id: ThreadId, mut rx: watch::Receiver, ) -> AgentStatus { @@ -491,7 +508,7 @@ pub(super) fn get_watermark( .map(|memory| memory.source_updated_at.timestamp()) .max() .unwrap_or(claimed_watermark) - .max(claimed_watermark) // todo double check the claimed here. + .max(claimed_watermark) } fn emit_metrics(session: &Arc, counters: Counters) { diff --git a/codex-rs/core/src/memories/prompts.rs b/codex-rs/core/src/memories/prompts.rs index 9425a53804..22e4008fa1 100644 --- a/codex-rs/core/src/memories/prompts.rs +++ b/codex-rs/core/src/memories/prompts.rs @@ -1,18 +1,12 @@ -use crate::memories::extensions::EXTENSION_RESOURCE_RETENTION_DAYS; -use crate::memories::extensions::RemovedExtensionResource; use crate::memories::memory_extensions_root; use crate::memories::memory_root; use crate::memories::phase_one; -use crate::memories::storage::rollout_summary_file_stem_from_parts; +use crate::memories::workspace::WORKSPACE_DIFF_FILENAME; use codex_protocol::openai_models::ModelInfo; -use codex_state::Phase2InputSelection; -use codex_state::Stage1Output; -use codex_state::Stage1OutputRef; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_output_truncation::TruncationPolicy; use codex_utils_output_truncation::truncate_text; use codex_utils_template::Template; -use std::fmt::Write as _; use std::path::Path; use std::sync::LazyLock; use tokio::fs; @@ -65,9 +59,9 @@ Memory extensions (under {{ memory_extensions_root }}/): source. If the user has any memory extensions, you MUST read the instructions for each extension to -determine how to use the memory source. If the Phase 2 diff lists removed memory extension -resources, use that extension-specific deletion diff to remove stale memories derived only from -those resources. If it has no extension folders, continue with the standard memory inputs only. +determine how to use the memory source. If the workspace diff shows deleted extension resource files, +remove stale memories derived only from those resources. If it has no extension folders, continue +with the standard memory inputs only. "#; const MEMORY_EXTENSIONS_PRIMARY_INPUTS: &str = r#" @@ -78,20 +72,17 @@ Under `{{ memory_extensions_root }}/`: - If extension folders exist, read each instructions.md first and follow it when interpreting that extension's memory source. -If the Phase 2 diff lists removed memory extension resources, use that extension-specific deletion -diff to remove stale memories derived only from those resources. +If the workspace diff shows deleted memory extension resources, use that extension-specific deletion +signal to remove stale memories derived only from those resources. "#; /// Builds the consolidation subagent prompt for a specific memory root. -pub(super) fn build_consolidation_prompt( - memory_root: &Path, - selection: &Phase2InputSelection, - removed_extension_resources: &[RemovedExtensionResource], -) -> String { +pub(super) fn build_consolidation_prompt(memory_root: &Path) -> String { let memory_extensions_root = memory_extensions_root(memory_root); let memory_extensions_exist = memory_extensions_root.is_dir(); let memory_root = memory_root.display().to_string(); let memory_extensions_root = memory_extensions_root.display().to_string(); + let phase2_workspace_diff_file = WORKSPACE_DIFF_FILENAME.to_string(); let memory_extensions_folder_structure = if memory_extensions_exist { render_memory_extensions_block( &MEMORY_EXTENSIONS_FOLDER_STRUCTURE_TEMPLATE, @@ -108,8 +99,6 @@ pub(super) fn build_consolidation_prompt( } else { String::new() }; - let phase2_input_selection = - render_phase2_input_selection(selection, removed_extension_resources); CONSOLIDATION_PROMPT_TEMPLATE .render([ ("memory_root", memory_root.as_str()), @@ -121,12 +110,15 @@ pub(super) fn build_consolidation_prompt( "memory_extensions_primary_inputs", memory_extensions_primary_inputs.as_str(), ), - ("phase2_input_selection", phase2_input_selection.as_str()), + ( + "phase2_workspace_diff_file", + phase2_workspace_diff_file.as_str(), + ), ]) .unwrap_or_else(|err| { warn!("failed to render memories consolidation prompt template: {err}"); format!( - "## Memory Phase 2 (Consolidation)\nConsolidate Codex memories in: {memory_root}\n\n{phase2_input_selection}" + "## Memory Phase 2 (Consolidation)\nConsolidate Codex memories in: {memory_root}\n\nRead {phase2_workspace_diff_file} first." ) }) } @@ -140,94 +132,6 @@ fn render_memory_extensions_block(template: &Template, memory_extensions_root: & }) } -fn render_phase2_input_selection( - selection: &Phase2InputSelection, - removed_extension_resources: &[RemovedExtensionResource], -) -> String { - let retained = selection.retained_thread_ids.len(); - let added = selection.selected.len().saturating_sub(retained); - let selected = if selection.selected.is_empty() { - "- none".to_string() - } else { - selection - .selected - .iter() - .map(|item| { - render_selected_input_line( - item, - selection.retained_thread_ids.contains(&item.thread_id), - ) - }) - .collect::>() - .join("\n") - }; - let removed = if selection.removed.is_empty() { - "- none".to_string() - } else { - selection - .removed - .iter() - .map(render_removed_input_line) - .collect::>() - .join("\n") - }; - - let mut rendered = format!( - "- selected inputs this run: {}\n- newly added since the last successful Phase 2 run: {added}\n- retained from the last successful Phase 2 run: {retained}\n- removed from the last successful Phase 2 run: {}\n\nCurrent selected Phase 1 inputs:\n{selected}\n\nRemoved from the last successful Phase 2 selection:\n{removed}\n", - selection.selected.len(), - selection.removed.len(), - ); - - if !removed_extension_resources.is_empty() { - rendered.push_str("\nMemory extension resources removed by retention pruning:\n"); - let _ = writeln!( - rendered, - "- retention window: {EXTENSION_RESOURCE_RETENTION_DAYS} days" - ); - let mut current_extension = ""; - for removed_resource in removed_extension_resources { - if removed_resource.extension != current_extension { - current_extension = &removed_resource.extension; - let _ = writeln!(rendered, "- extension: {current_extension}"); - } - let _ = writeln!(rendered, " - {}", removed_resource.resource_path); - } - } - - rendered -} - -fn render_selected_input_line(item: &Stage1Output, retained: bool) -> String { - let status = if retained { "retained" } else { "added" }; - let rollout_summary_file = format!( - "rollout_summaries/{}.md", - rollout_summary_file_stem_from_parts( - item.thread_id, - item.source_updated_at, - item.rollout_slug.as_deref(), - ) - ); - format!( - "- [{status}] thread_id={}, rollout_summary_file={rollout_summary_file}", - item.thread_id - ) -} - -fn render_removed_input_line(item: &Stage1OutputRef) -> String { - let rollout_summary_file = format!( - "rollout_summaries/{}.md", - rollout_summary_file_stem_from_parts( - item.thread_id, - item.source_updated_at, - item.rollout_slug.as_deref(), - ) - ); - format!( - "- thread_id={}, rollout_summary_file={rollout_summary_file}", - item.thread_id - ) -} - /// Builds the stage-1 user message containing rollout metadata and content. /// /// Large rollout payloads are truncated to 70% of the active model's effective diff --git a/codex-rs/core/src/memories/prompts_tests.rs b/codex-rs/core/src/memories/prompts_tests.rs index 937deac2c9..7c792e91cf 100644 --- a/codex-rs/core/src/memories/prompts_tests.rs +++ b/codex-rs/core/src/memories/prompts_tests.rs @@ -1,7 +1,5 @@ use super::*; -use crate::memories::extensions::RemovedExtensionResource; use codex_models_manager::model_info::model_info_from_slug; -use codex_state::Phase2InputSelection; use core_test_support::PathExt; use pretty_assertions::assert_eq; use tempfile::tempdir; @@ -58,33 +56,21 @@ fn build_stage_one_input_message_uses_default_limit_when_model_context_window_mi } #[test] -fn build_consolidation_prompt_includes_removed_extension_resources() { +fn build_consolidation_prompt_points_to_workspace_diff_and_extension_tree() { let temp = tempdir().unwrap(); let memory_root = temp.path().join("memories"); - std::fs::create_dir_all(temp.path().join("memories_extensions")).unwrap(); - let removed_extension_resources = vec![ - RemovedExtensionResource { - extension: "chronicle".to_string(), - resource_path: "resources/2026-04-06T11-59-59-abcd-10min-old.md".to_string(), - }, - RemovedExtensionResource { - extension: "chronicle".to_string(), - resource_path: "resources/2026-04-07T12-00-00-abcd-10min-cutoff.md".to_string(), - }, - ]; + let memory_extensions_root = memory_root.join("extensions"); + std::fs::create_dir_all(&memory_extensions_root).unwrap(); - let prompt = build_consolidation_prompt( - &memory_root, - &Phase2InputSelection::default(), - &removed_extension_resources, - ); + let prompt = build_consolidation_prompt(&memory_root); - assert!(prompt.contains("Memory extension resources removed by retention pruning:")); - assert!(prompt.contains("- retention window: 7 days")); - assert!(prompt.contains("- extension: chronicle")); - assert!(prompt.contains(" - resources/2026-04-06T11-59-59-abcd-10min-old.md")); - assert!(prompt.contains(" - resources/2026-04-07T12-00-00-abcd-10min-cutoff.md")); - assert!(prompt.contains("extension-specific deletion diff")); + assert!(prompt.contains("Memory workspace diff:")); + assert!(prompt.contains("phase2_workspace_diff.md")); + assert!(prompt.contains(&format!( + "Memory extensions (under {}/):", + memory_extensions_root.display() + ))); + assert!(prompt.contains("workspace diff shows deleted extension resource files")); } #[tokio::test] diff --git a/codex-rs/core/src/memories/storage.rs b/codex-rs/core/src/memories/storage.rs index 2455ae40df..e205ebe45c 100644 --- a/codex-rs/core/src/memories/storage.rs +++ b/codex-rs/core/src/memories/storage.rs @@ -38,24 +38,6 @@ pub(super) async fn sync_rollout_summaries_from_memories( write_rollout_summary_for_thread(root, memory).await?; } - if retained.is_empty() { - for file_name in ["MEMORY.md", "memory_summary.md"] { - let path = root.join(file_name); - if let Err(err) = tokio::fs::remove_file(path).await - && err.kind() != std::io::ErrorKind::NotFound - { - return Err(err); - } - } - - let skills_dir = root.join("skills"); - if let Err(err) = tokio::fs::remove_dir_all(skills_dir).await - && err.kind() != std::io::ErrorKind::NotFound - { - return Err(err); - } - } - Ok(()) } diff --git a/codex-rs/core/src/memories/tests.rs b/codex-rs/core/src/memories/tests.rs index 08ebcd802a..713f36b245 100644 --- a/codex-rs/core/src/memories/tests.rs +++ b/codex-rs/core/src/memories/tests.rs @@ -13,6 +13,7 @@ use codex_state::Stage1Output; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use serde_json::Value; +use std::path::Path; use std::path::PathBuf; use tempfile::tempdir; @@ -130,6 +131,56 @@ async fn clear_memory_root_contents_rejects_symlinked_root() { ); } +struct ConsolidatedOutputPaths { + memory_index: PathBuf, + memory_summary: PathBuf, + skill: PathBuf, +} + +async fn write_consolidated_outputs(root: &Path) -> ConsolidatedOutputPaths { + let paths = ConsolidatedOutputPaths { + memory_index: root.join("MEMORY.md"), + memory_summary: root.join("memory_summary.md"), + skill: root.join("skills/demo/SKILL.md"), + }; + + tokio::fs::write(&paths.memory_index, "consolidated memory index\n") + .await + .expect("write memory index"); + tokio::fs::write(&paths.memory_summary, "consolidated memory summary\n") + .await + .expect("write memory summary"); + tokio::fs::create_dir_all(paths.skill.parent().expect("skill parent")) + .await + .expect("create skill dir"); + tokio::fs::write(&paths.skill, "consolidated skill\n") + .await + .expect("write skill"); + + paths +} + +async fn assert_consolidated_outputs_exist(paths: &ConsolidatedOutputPaths, context: &str) { + assert!( + tokio::fs::try_exists(&paths.memory_index) + .await + .expect("check memory index existence"), + "{context} should leave MEMORY.md untouched" + ); + assert!( + tokio::fs::try_exists(&paths.memory_summary) + .await + .expect("check memory summary existence"), + "{context} should leave memory_summary.md untouched" + ); + assert!( + tokio::fs::try_exists(&paths.skill) + .await + .expect("check skill existence"), + "{context} should leave skills untouched" + ); +} + #[tokio::test] async fn sync_rollout_summaries_and_raw_memories_file_keeps_latest_memories_only() { let dir = tempdir().expect("tempdir"); @@ -236,6 +287,46 @@ async fn sync_rollout_summaries_and_raw_memories_file_keeps_latest_memories_only assert!(rollout_path_pos < file_pos); } +#[tokio::test] +async fn sync_empty_inputs_preserves_consolidated_outputs() { + let dir = tempdir().expect("tempdir"); + let root = dir.path().join("memory"); + ensure_layout(&root).await.expect("ensure layout"); + + let stale_rollout_summary_path = rollout_summaries_dir(&root).join("stale.md"); + tokio::fs::write(&stale_rollout_summary_path, "stale summary\n") + .await + .expect("write stale rollout summary"); + let outputs = write_consolidated_outputs(&root).await; + + sync_rollout_summaries_from_memories( + &root, + &[], + DEFAULT_MEMORIES_MAX_RAW_MEMORIES_FOR_CONSOLIDATION, + ) + .await + .expect("sync empty rollout summaries"); + rebuild_raw_memories_file_from_memories( + &root, + &[], + DEFAULT_MEMORIES_MAX_RAW_MEMORIES_FOR_CONSOLIDATION, + ) + .await + .expect("rebuild empty raw memories"); + + assert!( + !tokio::fs::try_exists(&stale_rollout_summary_path) + .await + .expect("check stale rollout summary existence"), + "empty sync should prune stale rollout summaries" + ); + let raw_memories = tokio::fs::read_to_string(raw_memories_file(&root)) + .await + .expect("read raw memories"); + assert_eq!(raw_memories, "# Raw Memories\n\nNo raw memories yet.\n"); + assert_consolidated_outputs_exist(&outputs, "empty sync").await; +} + #[tokio::test] async fn sync_rollout_summaries_uses_timestamp_hash_and_sanitized_slug_filename() { let dir = tempdir().expect("tempdir"); @@ -422,6 +513,9 @@ mod phase2 { use crate::memories::phase2; use crate::memories::raw_memories_file; use crate::memories::rollout_summaries_dir; + use crate::memories::storage::rebuild_raw_memories_file_from_memories; + use crate::memories::storage::sync_rollout_summaries_from_memories; + use crate::memories::workspace::prepare_memory_workspace; use crate::session::session::Session; use crate::session::tests::make_session_and_context; use chrono::Duration as ChronoDuration; @@ -520,7 +614,7 @@ mod phase2 { } } - async fn seed_stage1_output(&self, source_updated_at: i64) { + async fn seed_stage1_output(&self, source_updated_at: i64) -> ThreadId { let thread_id = ThreadId::new(); let mut metadata_builder = ThreadMetadataBuilder::new( thread_id, @@ -569,6 +663,7 @@ mod phase2 { .expect("mark stage-1 success"), "stage-1 success should enqueue global consolidation" ); + thread_id } async fn shutdown_threads(&self) { @@ -613,16 +708,85 @@ mod phase2 { } #[tokio::test] - async fn dispatch_skips_when_global_job_is_not_dirty() { + async fn dispatch_skips_when_memory_workspace_is_not_dirty() { let harness = DispatchHarness::new().await; + let root = memory_root(&harness.config.codex_home); + rebuild_raw_memories_file_from_memories( + &root, + &[], + /*max_raw_memories_for_consolidation*/ 0, + ) + .await + .expect("write empty raw memories baseline"); + let outputs = super::write_consolidated_outputs(&root).await; + prepare_memory_workspace(&root) + .await + .expect("commit empty memory workspace as baseline"); phase2::run(&harness.session, Arc::clone(&harness.config)).await; pretty_assertions::assert_eq!(harness.user_input_ops_count(), 0); + super::assert_consolidated_outputs_exist(&outputs, "clean no-input phase2").await; let thread_ids = harness.manager.list_thread_ids().await; pretty_assertions::assert_eq!(thread_ids.len(), 0); } + #[tokio::test] + async fn dispatch_uses_git_dirty_state_without_db_dirty_watermark() { + let harness = DispatchHarness::new().await; + let root = memory_root(&harness.config.codex_home); + rebuild_raw_memories_file_from_memories( + &root, + &[], + /*max_raw_memories_for_consolidation*/ 0, + ) + .await + .expect("write empty raw memories baseline"); + prepare_memory_workspace(&root) + .await + .expect("commit empty memory workspace as baseline"); + let extension_resource = root + .join("extensions") + .join("chronicle") + .join("resources") + .join("2026-04-22T12-00-00-abcd-10min-memory.md"); + tokio::fs::create_dir_all( + extension_resource + .parent() + .expect("extension resource parent"), + ) + .await + .expect("create extension resource dir"); + tokio::fs::write( + root.join("extensions/chronicle/instructions.md"), + "instructions\n", + ) + .await + .expect("write extension instructions"); + tokio::fs::write(&extension_resource, "extension memory\n") + .await + .expect("write extension resource"); + + phase2::run(&harness.session, Arc::clone(&harness.config)).await; + + pretty_assertions::assert_eq!(harness.user_input_ops_count(), 1); + let workspace_diff = tokio::fs::read_to_string(root.join("phase2_workspace_diff.md")) + .await + .expect("read workspace diff"); + assert!( + workspace_diff.contains("- A extensions/chronicle/instructions.md"), + "git-only extension instructions should dirty phase2: {workspace_diff}" + ); + assert!( + workspace_diff.contains("- A extensions/chronicle/resources/"), + "git-only extension resource should dirty phase2: {workspace_diff}" + ); + let thread_ids = harness.manager.list_thread_ids().await; + pretty_assertions::assert_eq!(thread_ids.len(), 1); + + harness.shutdown_threads().await; + } + #[tokio::test] async fn dispatch_skips_when_global_job_is_already_running() { let harness = DispatchHarness::new().await; @@ -696,7 +860,8 @@ mod phase2 { assert!( matches!( post_dispatch_claim, - Phase2JobClaimOutcome::SkippedRunning | Phase2JobClaimOutcome::SkippedNotDirty + Phase2JobClaimOutcome::SkippedRunning + | Phase2JobClaimOutcome::SkippedRetryUnavailable ), "stale-lock dispatch should either keep the reclaimed job running or finish it before re-claim" ); @@ -835,7 +1000,7 @@ mod phase2 { } #[tokio::test] - async fn dispatch_with_empty_stage1_outputs_rebuilds_local_artifacts() { + async fn dispatch_with_empty_stage1_outputs_spawns_for_workspace_changes() { let harness = DispatchHarness::new().await; let root = memory_root(&harness.config.codex_home); let summaries_dir = rollout_summaries_dir(&root); @@ -851,25 +1016,7 @@ mod phase2 { tokio::fs::write(&raw_memories_path, "stale raw memories\n") .await .expect("write stale raw memories"); - let memory_index_path = root.join("MEMORY.md"); - tokio::fs::write(&memory_index_path, "stale memory index\n") - .await - .expect("write stale memory index"); - let memory_summary_path = root.join("memory_summary.md"); - tokio::fs::write(&memory_summary_path, "stale memory summary\n") - .await - .expect("write stale memory summary"); - let stale_skill_file = root.join("skills/demo/SKILL.md"); - tokio::fs::create_dir_all( - stale_skill_file - .parent() - .expect("skills subdirectory parent should exist"), - ) - .await - .expect("create stale skills dir"); - tokio::fs::write(&stale_skill_file, "stale skill\n") - .await - .expect("write stale skill"); + let outputs = super::write_consolidated_outputs(&root).await; harness .state_db @@ -889,43 +1036,130 @@ mod phase2 { .await .expect("read rebuilt raw memories"); pretty_assertions::assert_eq!(raw_memories, "# Raw Memories\n\nNo raw memories yet.\n"); - assert!( - !tokio::fs::try_exists(&memory_index_path) - .await - .expect("check memory index existence"), - "empty consolidation should remove stale MEMORY.md" - ); - assert!( - !tokio::fs::try_exists(&memory_summary_path) - .await - .expect("check memory summary existence"), - "empty consolidation should remove stale memory_summary.md" - ); - assert!( - !tokio::fs::try_exists(&stale_skill_file) - .await - .expect("check stale skill existence"), - "empty consolidation should remove stale skills artifacts" - ); - assert!( - !tokio::fs::try_exists(root.join("skills")) - .await - .expect("check skills dir existence"), - "empty consolidation should remove stale skills directory" - ); + super::assert_consolidated_outputs_exist(&outputs, "empty consolidation").await; let next_claim = harness .state_db .try_claim_global_phase2_job(ThreadId::new(), /*lease_seconds*/ 3_600) .await - .expect("claim global job after empty consolidation success"); - pretty_assertions::assert_eq!(next_claim, Phase2JobClaimOutcome::SkippedNotDirty); - pretty_assertions::assert_eq!(harness.user_input_ops_count(), 0); + .expect("claim global job after empty consolidation dispatch"); + pretty_assertions::assert_eq!(next_claim, Phase2JobClaimOutcome::SkippedRunning); + pretty_assertions::assert_eq!(harness.user_input_ops_count(), 1); let thread_ids = harness.manager.list_thread_ids().await; - pretty_assertions::assert_eq!(thread_ids.len(), 0); + pretty_assertions::assert_eq!(thread_ids.len(), 1); harness.shutdown_threads().await; } + #[tokio::test] + async fn dispatch_with_empty_selected_inputs_preserves_consolidated_outputs() { + let harness = DispatchHarness::new().await; + let source_updated_at = Utc::now().timestamp(); + let thread_id = harness.seed_stage1_output(source_updated_at).await; + let root = memory_root(&harness.config.codex_home); + let selected = harness + .state_db + .get_phase2_input_selection(/*n*/ 1, /*max_unused_days*/ 30) + .await + .expect("load phase2 input selection"); + sync_rollout_summaries_from_memories(&root, &selected, selected.len()) + .await + .expect("sync selected rollout summaries"); + rebuild_raw_memories_file_from_memories(&root, &selected, selected.len()) + .await + .expect("sync selected raw memories"); + let outputs = super::write_consolidated_outputs(&root).await; + prepare_memory_workspace(&root) + .await + .expect("commit current memory workspace as baseline"); + + let claim = harness + .state_db + .try_claim_global_phase2_job(ThreadId::new(), /*lease_seconds*/ 3_600) + .await + .expect("claim global phase2 job"); + let Phase2JobClaimOutcome::Claimed { + ownership_token, .. + } = claim + else { + panic!("unexpected phase2 claim outcome: {claim:?}"); + }; + assert!( + harness + .state_db + .mark_global_phase2_job_succeeded(&ownership_token, source_updated_at, &selected) + .await + .expect("mark phase2 succeeded"), + "phase2 success should update selected baseline" + ); + assert!( + harness + .state_db + .mark_thread_memory_mode_polluted(thread_id) + .await + .expect("mark thread polluted"), + "polluted selected thread should enqueue phase2 forgetting" + ); + + phase2::run(&harness.session, Arc::clone(&harness.config)).await; + + pretty_assertions::assert_eq!(harness.user_input_ops_count(), 1); + super::assert_consolidated_outputs_exist(&outputs, "empty selected phase2").await; + let workspace_diff = tokio::fs::read_to_string(root.join("phase2_workspace_diff.md")) + .await + .expect("read workspace diff"); + assert!( + workspace_diff.contains("- D rollout_summaries/"), + "empty selected phase2 should surface deleted rollout summaries: {workspace_diff}" + ); + assert!( + !workspace_diff.contains("- D MEMORY.md"), + "empty selected phase2 should not delete MEMORY.md directly: {workspace_diff}" + ); + assert!( + !workspace_diff.contains("- D memory_summary.md"), + "empty selected phase2 should not delete memory_summary.md directly: {workspace_diff}" + ); + assert!( + !workspace_diff.contains("- D skills/demo/SKILL.md"), + "empty selected phase2 should not delete skills directly: {workspace_diff}" + ); + + harness.shutdown_threads().await; + } + + #[tokio::test] + async fn dispatch_with_clean_workspace_preserves_selected_phase2_baseline() { + let harness = DispatchHarness::new().await; + let thread_id = harness.seed_stage1_output(Utc::now().timestamp()).await; + let root = memory_root(&harness.config.codex_home); + let selected = harness + .state_db + .get_phase2_input_selection(/*n*/ 1, /*max_unused_days*/ 30) + .await + .expect("load phase2 input selection"); + + sync_rollout_summaries_from_memories(&root, &selected, selected.len()) + .await + .expect("sync selected rollout summaries"); + rebuild_raw_memories_file_from_memories(&root, &selected, selected.len()) + .await + .expect("sync selected raw memories"); + prepare_memory_workspace(&root) + .await + .expect("commit current memory workspace as baseline"); + + phase2::run(&harness.session, Arc::clone(&harness.config)).await; + + pretty_assertions::assert_eq!(harness.user_input_ops_count(), 0); + let selected = harness + .state_db + .get_phase2_input_selection(/*n*/ 1, /*max_unused_days*/ 30) + .await + .expect("load phase2 input selection after clean workspace success"); + pretty_assertions::assert_eq!(selected.len(), 1); + pretty_assertions::assert_eq!(selected[0].thread_id, thread_id); + } + #[tokio::test] async fn dispatch_marks_job_for_retry_when_sandbox_policy_cannot_be_overridden() { let harness = DispatchHarness::new().await; @@ -946,7 +1180,7 @@ mod phase2 { .try_claim_global_phase2_job(ThreadId::new(), /*lease_seconds*/ 3_600) .await .expect("claim global job after sandbox policy failure"); - pretty_assertions::assert_eq!(retry_claim, Phase2JobClaimOutcome::SkippedNotDirty); + pretty_assertions::assert_eq!(retry_claim, Phase2JobClaimOutcome::SkippedRetryUnavailable); pretty_assertions::assert_eq!(harness.user_input_ops_count(), 0); let thread_ids = harness.manager.list_thread_ids().await; pretty_assertions::assert_eq!(thread_ids.len(), 0); @@ -968,7 +1202,7 @@ mod phase2 { .try_claim_global_phase2_job(ThreadId::new(), /*lease_seconds*/ 3_600) .await .expect("claim global job after sync failure"); - pretty_assertions::assert_eq!(retry_claim, Phase2JobClaimOutcome::SkippedNotDirty); + pretty_assertions::assert_eq!(retry_claim, Phase2JobClaimOutcome::SkippedRetryUnavailable); pretty_assertions::assert_eq!(harness.user_input_ops_count(), 0); let thread_ids = harness.manager.list_thread_ids().await; pretty_assertions::assert_eq!(thread_ids.len(), 0); @@ -990,7 +1224,7 @@ mod phase2 { .try_claim_global_phase2_job(ThreadId::new(), /*lease_seconds*/ 3_600) .await .expect("claim global job after rebuild failure"); - pretty_assertions::assert_eq!(retry_claim, Phase2JobClaimOutcome::SkippedNotDirty); + pretty_assertions::assert_eq!(retry_claim, Phase2JobClaimOutcome::SkippedRetryUnavailable); pretty_assertions::assert_eq!(harness.user_input_ops_count(), 0); let thread_ids = harness.manager.list_thread_ids().await; pretty_assertions::assert_eq!(thread_ids.len(), 0); @@ -1067,14 +1301,14 @@ mod phase2 { let chronicle_resources = config .codex_home - .join("memories_extensions/chronicle/resources"); + .join("memories/extensions/chronicle/resources"); tokio::fs::create_dir_all(&chronicle_resources) .await .expect("create chronicle resources"); tokio::fs::write( config .codex_home - .join("memories_extensions/chronicle/instructions.md"), + .join("memories/extensions/chronicle/instructions.md"), "instructions", ) .await @@ -1095,14 +1329,22 @@ mod phase2 { .expect("claim global job after spawn failure"); pretty_assertions::assert_eq!( retry_claim, - Phase2JobClaimOutcome::SkippedNotDirty, + Phase2JobClaimOutcome::SkippedRetryUnavailable, "spawn failures should leave the job in retry backoff instead of running" ); assert!( - tokio::fs::try_exists(&old_file) + !tokio::fs::try_exists(&old_file) .await .expect("check old extension resource"), - "spawn failures should not prune extension resources before retry" + "old extension resources should still be pruned on failed phase2 attempts" + ); + let workspace_diff = + tokio::fs::read_to_string(config.codex_home.join("memories/phase2_workspace_diff.md")) + .await + .expect("read workspace diff"); + assert!( + workspace_diff.contains("- D extensions/chronicle/resources/"), + "spawn failures should keep a retryable workspace diff: {workspace_diff}" ); } } diff --git a/codex-rs/core/src/memories/workspace.rs b/codex-rs/core/src/memories/workspace.rs new file mode 100644 index 0000000000..205081aa41 --- /dev/null +++ b/codex-rs/core/src/memories/workspace.rs @@ -0,0 +1,124 @@ +use anyhow::Context; +use codex_git_utils::GitBaselineDiff; +use codex_git_utils::diff_since_latest_init; +use codex_git_utils::ensure_git_baseline_repository; +use codex_git_utils::reset_git_repository; +use std::path::Path; + +/// Generated diff file the Phase 2 consolidation agent reads before editing memories. +pub(super) const WORKSPACE_DIFF_FILENAME: &str = "phase2_workspace_diff.md"; + +const WORKSPACE_DIFF_MAX_BYTES: usize = 4 * 1024 * 1024; + +/// Prepares the memory directory for git-baseline diffing. +/// +/// This keeps an existing usable `.git/` baseline intact. It initializes a new git baseline when the +/// metadata is missing or unusable, and removes any stale generated `phase2_workspace_diff.md` file +/// so that the next diff does not include a previous prompt artifact. +pub(super) async fn prepare_memory_workspace(root: &Path) -> anyhow::Result<()> { + tokio::fs::create_dir_all(root) + .await + .with_context(|| format!("create memory workspace {}", root.display()))?; + remove_workspace_diff(root).await?; + ensure_git_baseline_repository(root).await?; + Ok(()) +} + +/// Returns the current workspace diff after removing any stale generated diff artifact. +/// +/// The removed file is only `phase2_workspace_diff.md`; memory artifacts and `.git/` metadata are +/// left intact. +pub(super) async fn memory_workspace_diff(root: &Path) -> anyhow::Result { + remove_workspace_diff(root).await?; + diff_since_latest_init(root).await +} + +/// Writes `phase2_workspace_diff.md` with a bounded git-style diff from the current baseline. +pub(super) async fn write_workspace_diff( + root: &Path, + diff: &GitBaselineDiff, +) -> anyhow::Result<()> { + let path = root.join(WORKSPACE_DIFF_FILENAME); + tokio::fs::write(&path, render_workspace_diff_file(diff)) + .await + .with_context(|| format!("write memory workspace diff file {}", path.display())) +} + +/// Marks the current memory root as the new baseline. +/// +/// The generated diff file is removed before resetting the baseline so deleted memory content is +/// not retained in the prompt artifact or in unreachable git objects. +pub(super) async fn reset_memory_workspace_baseline(root: &Path) -> anyhow::Result<()> { + remove_workspace_diff(root).await?; + reset_git_repository(root).await +} + +/// Removes the generated `phase2_workspace_diff.md` prompt artifact. +/// +/// This does not remove `.git/`, reset the baseline, or delete memory content. It is used before +/// diffing and before baseline reset so the generated diff file itself is not treated as memory +/// workspace input. +pub(super) async fn remove_workspace_diff(root: &Path) -> anyhow::Result<()> { + let path = root.join(WORKSPACE_DIFF_FILENAME); + match tokio::fs::remove_file(&path).await { + Ok(()) => Ok(()), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(err) => Err(err) + .with_context(|| format!("remove memory workspace diff file {}", path.display())), + } +} + +fn render_workspace_diff_file(diff: &GitBaselineDiff) -> String { + let mut rendered = String::from( + "# Memory Workspace Diff\n\n\ + Generated by Codex before Phase 2 memory consolidation. Read this file first and do not edit it.\n\n\ + ## Status\n", + ); + + if !diff.has_changes() { + rendered.push_str("- none\n"); + return rendered; + } + + for change in &diff.changes { + rendered.push_str(&format!("- {} {}\n", change.status.label(), change.path)); + } + rendered.push_str("\n## Diff\n\n```diff\n"); + append_bounded_diff(&mut rendered, &diff.unified_diff); + rendered.push_str("```\n"); + rendered +} + +fn append_bounded_diff(rendered: &mut String, diff: &str) { + if diff.len() <= WORKSPACE_DIFF_MAX_BYTES { + rendered.push_str(diff); + if !diff.ends_with('\n') { + rendered.push('\n'); + } + return; + } + + let boundary = previous_char_boundary(diff, WORKSPACE_DIFF_MAX_BYTES); + rendered.push_str(&diff[..boundary]); + if !rendered.ends_with('\n') { + rendered.push('\n'); + } + rendered.push_str(&format!( + "\n[workspace diff truncated at {WORKSPACE_DIFF_MAX_BYTES} bytes]\n" + )); +} + +fn previous_char_boundary(value: &str, max_bytes: usize) -> usize { + if max_bytes >= value.len() { + return value.len(); + } + let mut index = max_bytes; + while !value.is_char_boundary(index) { + index -= 1; + } + index +} + +#[cfg(test)] +#[path = "workspace_tests.rs"] +mod tests; diff --git a/codex-rs/core/src/memories/workspace_tests.rs b/codex-rs/core/src/memories/workspace_tests.rs new file mode 100644 index 0000000000..8fc4939d87 --- /dev/null +++ b/codex-rs/core/src/memories/workspace_tests.rs @@ -0,0 +1,78 @@ +use super::*; +use codex_git_utils::GitBaselineChange; +use codex_git_utils::GitBaselineChangeStatus; +use pretty_assertions::assert_eq; +use std::fs; +use tempfile::TempDir; + +#[test] +fn render_workspace_diff_file_bounds_large_diff() { + let diff = GitBaselineDiff { + changes: vec![GitBaselineChange { + status: GitBaselineChangeStatus::Modified, + path: "MEMORY.md".to_string(), + }], + unified_diff: "a".repeat(WORKSPACE_DIFF_MAX_BYTES + 128), + }; + + let rendered = render_workspace_diff_file(&diff); + + assert!(rendered.contains("- M MEMORY.md")); + assert!(rendered.contains("[workspace diff truncated at 4194304 bytes]")); + assert!(rendered.ends_with("```\n")); +} + +#[tokio::test] +async fn reset_memory_workspace_baseline_removes_generated_diff() { + let home = TempDir::new().expect("tempdir"); + let root = home.path().join("memories"); + prepare_memory_workspace(&root) + .await + .expect("prepare memory workspace"); + fs::write(root.join("MEMORY.md"), "memory").expect("write memory"); + write_workspace_diff( + &root, + &GitBaselineDiff { + changes: vec![GitBaselineChange { + status: GitBaselineChangeStatus::Added, + path: "MEMORY.md".to_string(), + }], + unified_diff: "+memory\n".to_string(), + }, + ) + .await + .expect("write workspace diff"); + + reset_memory_workspace_baseline(&root) + .await + .expect("reset baseline"); + + assert!(!root.join(WORKSPACE_DIFF_FILENAME).exists()); + let diff = memory_workspace_diff(&root) + .await + .expect("load workspace diff"); + assert_eq!(diff.changes, Vec::new()); +} + +#[tokio::test] +async fn prepare_memory_workspace_recovers_unusable_git_dir() { + let home = TempDir::new().expect("tempdir"); + let root = home.path().join("memories"); + fs::create_dir_all(root.join(".git")).expect("create unusable git dir"); + fs::write(root.join("MEMORY.md"), "memory").expect("write memory"); + + prepare_memory_workspace(&root) + .await + .expect("prepare memory workspace"); + + let diff = memory_workspace_diff(&root) + .await + .expect("load workspace diff"); + assert_eq!(diff.changes, Vec::new()); +} + +#[test] +fn previous_char_boundary_handles_multibyte_text() { + let text = "aé"; + assert_eq!(previous_char_boundary(text, /*max_bytes*/ 2), 1); +} diff --git a/codex-rs/core/templates/memories/consolidation.md b/codex-rs/core/templates/memories/consolidation.md index 15c718ec9d..8ce97a4322 100644 --- a/codex-rs/core/templates/memories/consolidation.md +++ b/codex-rs/core/templates/memories/consolidation.md @@ -143,29 +143,34 @@ Mode selection: - INCREMENTAL UPDATE: existing artifacts already exist and `raw_memories.md` mostly contains new additions. -Incremental thread diff snapshot (computed before the current artifact sync rewrites local files): +Memory workspace diff: -**Diff since last consolidation:** -{{ phase2_input_selection }} +The folder `{{ memory_root }}/` is a git repository managed by Codex. Read +`{{ phase2_workspace_diff_file }}` in this same folder first. It contains the git-style diff from +the previous successful Phase 2 baseline to the current worktree. It is generated by Codex for +this run and is not part of the committed memory artifacts. Incremental update and forgetting mechanism: -- Use the diff provided +- Use the git-style diff in `{{ phase2_workspace_diff_file }}` to identify relevant changed + sections and deleted inputs. +- Every changes in `{{ phase2_workspace_diff_file }}` are authoritative and must propagated and consolidated. If a + changes appears to be randomly placed in the files, it is probably a user change and you shouldn't just drop it. + Make sure to add it to the overall memories consolidation - Do not open raw sessions / original rollout transcripts. -- For each added thread id, search it in `raw_memories.md`, read that raw-memory section, and - read the corresponding `rollout_summaries/*.md` file only when needed for stronger evidence, - task placement, or conflict resolution. +- For added or modified `raw_memories.md` and `rollout_summaries/*.md` files, read the changed + raw-memory sections and the corresponding rollout summaries only when needed for stronger + evidence, task placement, or conflict resolution. - When scanning a raw-memory section, read the task-level `Preference signals:` subsections first, then the rest of the task blocks. -- For each removed thread id, search it in `MEMORY.md` and delete only the memory supported by - that thread. Use `thread_id=` in `### rollout_summary_files` when available; if not, - fall back to rollout summary filenames plus the corresponding `rollout_summaries/*.md` files. -- If a `MEMORY.md` block contains both removed and undeleted threads, do not delete the whole - block. Remove only the removed thread's references and thread-local guidance, preserve shared - or still-supported content, and split or rewrite the block only if needed to keep the undeleted - threads intact. +- For deleted `rollout_summaries/*.md` or `extensions/*/resources/*.md` files, search their + filenames, paths, and thread ids (when present) in `MEMORY.md`. Delete only memory supported + by deleted inputs. +- If a `MEMORY.md` block contains both deleted and still-present evidence, do not delete the whole + block. Remove only stale references and stale local guidance, preserve shared or still-supported + content, and split or rewrite the block only if needed. - After `MEMORY.md` cleanup is done, revisit `memory_summary.md` and remove or rewrite stale - summary/index content that was only supported by removed thread ids. + summary/index content that was only supported by deleted files. Outputs: Under `{{ memory_root }}/`: @@ -743,26 +748,28 @@ WORKFLOW 3. INCREMENTAL UPDATE behavior: - Read existing `MEMORY.md` and `memory_summary.md` first for continuity and to locate existing references that may need surgical cleanup. - - Use the injected thread-diff snapshot as the first routing pass: - - added thread ids = ingestion queue - - removed thread ids = forgetting / stale-cleanup queue + - Use the injected git-style workspace changes as the first routing pass: + - added/modified `raw_memories.md` and `rollout_summaries/*.md` = ingestion queue + - deleted `rollout_summaries/*.md` and `extensions/*/resources/*.md` = forgetting / + stale-cleanup queue - Build an index of rollout references already present in existing `MEMORY.md` before scanning raw memories so you can route net-new evidence into the right blocks. - Work in this order: - 1. For newly added thread ids, search them in `raw_memories.md`, read those sections, and - open the corresponding `rollout_summaries/*.md` files when necessary. + 1. For added or modified rollout inputs, search their paths/thread ids in `raw_memories.md`, + read those sections, and open the corresponding `rollout_summaries/*.md` files when + necessary. 2. Route the new signal into existing `MEMORY.md` blocks or create new ones when needed. - 3. For removed thread ids, search `MEMORY.md` and surgically delete or rewrite only the - unsupported thread-local memory. - 4. If a block mixes removed and undeleted threads, preserve the undeleted-thread content; - split or rewrite the block if that is the cleanest way to delete only the removed part. + 3. For deleted inputs, search `MEMORY.md` and surgically delete or rewrite only the + unsupported memory. + 4. If a block mixes deleted and still-present evidence, preserve the still-supported content; + split or rewrite the block if that is the cleanest way to delete only the stale part. 5. After `MEMORY.md` is correct, revisit `memory_summary.md` and remove or rewrite stale - summary/index content that no longer has undeleted support. + summary/index content that no longer has current support. - Integrate new signal into existing artifacts by: - - scanning the newly added raw-memory entries in recency order and identifying which existing blocks they should update + - scanning added or modified raw-memory entries in recency order and identifying which existing blocks they should update - updating existing knowledge with better/newer evidence - updating stale or contradicting guidance - - pruning or downgrading memory whose only provenance comes from removed thread ids + - pruning or downgrading memory whose only provenance comes from deleted inputs - expanding terse old blocks when new summaries/raw memories make the task family clearer - doing light clustering and merging if needed - refreshing `MEMORY.md` top-of-file ordering so recent high-utility task families stay easy to find @@ -774,8 +781,8 @@ WORKFLOW target, keep its wording, label, and relative order mostly stable. Rewrite/reorder/rename/ split/merge only when fixing a real problem (staleness, ambiguity, schema drift, wrong boundaries) or when meaningful new evidence materially improves retrieval clarity/searchability. - - Spend most of your deep-dive budget on newly added thread ids and on mixed blocks touched by - removed thread ids. Do not re-read unchanged older threads unless you need them for + - Spend most of your deep-dive budget on added/modified inputs and on mixed blocks touched by + deleted inputs. Do not re-read unchanged older threads unless you need them for conflict resolution, clustering, or provenance repair. 4. Evidence deep-dive rule (both modes): @@ -793,8 +800,7 @@ WORKFLOW evidence, procedural detail, validation signals, and user feedback before finalizing `MEMORY.md`. - When deleting stale memory from a mixed block, use the relevant rollout summaries to decide - which details are uniquely supported by removed threads versus still supported by undeleted - threads. + which details are uniquely supported by deleted inputs versus still-supported evidence. - Use `updated_at` and validation strength together to resolve stale/conflicting notes. - For user-profile or preference claims, recurrence matters: repeated evidence across rollouts should generally outrank a single polished but isolated summary. @@ -811,7 +817,7 @@ WORKFLOW - remove duplication in memory_summary, skills/, and MEMORY.md - remove stale or low-signal blocks that are less likely to be useful in the future - remove or rewrite blocks/task sections whose supporting rollout references point only to - removed thread ids or missing rollout summary files + deleted inputs or missing rollout summary files - run a global rollout-reference audit on final `MEMORY.md` and fix accidental duplicate entries / redundant repetition, while preserving intentional multi-task or multi-block reuse when it adds distinct task-local value diff --git a/codex-rs/core/tests/suite/memories.rs b/codex-rs/core/tests/suite/memories.rs index c327bc55df..a585ed24c5 100644 --- a/codex-rs/core/tests/suite/memories.rs +++ b/codex-rs/core/tests/suite/memories.rs @@ -2,6 +2,8 @@ use anyhow::Result; use chrono::Duration as ChronoDuration; use chrono::Utc; use codex_features::Feature; +use codex_git_utils::diff_since_latest_init; +use codex_git_utils::reset_git_repository; use codex_protocol::ThreadId; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; @@ -11,9 +13,7 @@ use core_test_support::responses::ResponsesRequest; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_response_created; -use core_test_support::responses::ev_web_search_call_done; use core_test_support::responses::mount_sse_once; -use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::sse; use core_test_support::responses::start_mock_server; use core_test_support::test_codex::TestCodex; @@ -27,13 +27,14 @@ use tokio::time::Duration; use tokio::time::Instant; #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn memories_startup_phase2_tracks_added_and_removed_inputs_across_runs() -> Result<()> { +async fn memories_startup_phase2_tracks_workspace_diff_across_runs() -> Result<()> { let server = start_mock_server().await; let home = Arc::new(TempDir::new()?); let db = init_state_db(&home).await?; + let memory_root = home.path().join("memories"); let now = Utc::now(); - let thread_a = seed_stage1_output( + let _thread_a = seed_stage1_output( db.as_ref(), home.path(), now - ChronoDuration::hours(2), @@ -43,53 +44,21 @@ async fn memories_startup_phase2_tracks_added_and_removed_inputs_across_runs() - ) .await?; - let first_phase2 = mount_sse_once( - &server, - sse(vec![ - ev_response_created("resp-phase2-1"), - ev_assistant_message("msg-phase2-1", "phase2 complete"), - ev_completed("resp-phase2-1"), - ]), + let rollout_summaries_root = memory_root.join("rollout_summaries"); + tokio::fs::create_dir_all(&rollout_summaries_root).await?; + tokio::fs::write( + memory_root.join("raw_memories.md"), + "# Raw Memories\n\nraw memory A\n", ) - .await; + .await?; + tokio::fs::write( + rollout_summaries_root.join("rollout-a.md"), + "git_branch: branch-rollout-a\n\nrollout summary A\n", + ) + .await?; + reset_git_repository(&memory_root).await?; - let first = build_test_codex(&server, home.clone()).await?; - let first_request = wait_for_single_request(&first_phase2).await; - let first_prompt = phase2_prompt_text(&first_request); - assert!( - first_prompt.contains("- selected inputs this run: 1"), - "expected selected count in first prompt: {first_prompt}" - ); - assert!( - first_prompt.contains("- newly added since the last successful Phase 2 run: 1"), - "expected added count in first prompt: {first_prompt}" - ); - assert!( - first_prompt.contains("- removed from the last successful Phase 2 run: 0"), - "expected removed count in first prompt: {first_prompt}" - ); - assert!( - first_prompt.contains(&format!("- [added] thread_id={thread_a},")), - "expected thread A to be marked added: {first_prompt}" - ); - assert!( - first_prompt.contains("Removed from the last successful Phase 2 selection:\n- none"), - "expected no removed items in first prompt: {first_prompt}" - ); - - wait_for_phase2_success(db.as_ref(), thread_a).await?; - let memory_root = home.path().join("memories"); - let raw_memories = tokio::fs::read_to_string(memory_root.join("raw_memories.md")).await?; - assert!(raw_memories.contains("raw memory A")); - assert!(!raw_memories.contains("raw memory B")); - let rollout_summaries = read_rollout_summary_bodies(&memory_root).await?; - assert_eq!(rollout_summaries.len(), 1); - assert!(rollout_summaries[0].contains("rollout summary A")); - assert!(rollout_summaries[0].contains("git_branch: branch-rollout-a")); - - shutdown_test_codex(&first).await?; - - let thread_b = seed_stage1_output( + let _thread_b = seed_stage1_output( db.as_ref(), home.path(), now - ChronoDuration::hours(1), @@ -99,46 +68,30 @@ async fn memories_startup_phase2_tracks_added_and_removed_inputs_across_runs() - ) .await?; - let second_phase2 = mount_sse_once( + let phase2 = mount_sse_once( &server, sse(vec![ - ev_response_created("resp-phase2-2"), - ev_assistant_message("msg-phase2-2", "phase2 complete"), - ev_completed("resp-phase2-2"), + ev_response_created("resp-phase2"), + ev_assistant_message("msg-phase2", "phase2 complete"), + ev_completed("resp-phase2"), ]), ) .await; - let second = build_test_codex(&server, home.clone()).await?; - let second_request = wait_for_single_request(&second_phase2).await; - let second_prompt = phase2_prompt_text(&second_request); + let codex = build_test_codex(&server, home.clone()).await?; + let request = wait_for_single_request(&phase2).await; + let prompt = phase2_prompt_text(&request); assert!( - second_prompt.contains("- selected inputs this run: 1"), - "expected selected count in second prompt: {second_prompt}" - ); - assert!( - second_prompt.contains("- newly added since the last successful Phase 2 run: 1"), - "expected added count in second prompt: {second_prompt}" - ); - assert!( - second_prompt.contains("- removed from the last successful Phase 2 run: 1"), - "expected removed count in second prompt: {second_prompt}" - ); - assert!( - second_prompt.contains(&format!("- [added] thread_id={thread_b},")), - "expected thread B to be marked added: {second_prompt}" - ); - assert!( - second_prompt.contains(&format!("- thread_id={thread_a},")), - "expected thread A to be marked removed: {second_prompt}" + prompt.contains("phase2_workspace_diff.md"), + "expected workspace diff file in prompt: {prompt}" ); - wait_for_phase2_success(db.as_ref(), thread_b).await?; + wait_for_phase2_workspace_reset(&memory_root).await?; let raw_memories = tokio::fs::read_to_string(memory_root.join("raw_memories.md")).await?; assert!(raw_memories.contains("raw memory B")); - assert!(raw_memories.contains("raw memory A")); + assert!(!raw_memories.contains("raw memory A")); let rollout_summaries = read_rollout_summary_bodies(&memory_root).await?; - assert_eq!(rollout_summaries.len(), 2); + assert_eq!(rollout_summaries.len(), 1); assert!( rollout_summaries .iter() @@ -152,20 +105,20 @@ async fn memories_startup_phase2_tracks_added_and_removed_inputs_across_runs() - assert!( rollout_summaries .iter() - .any(|summary| summary.contains("rollout summary A")) + .all(|summary| !summary.contains("rollout summary A")) ); - shutdown_test_codex(&second).await?; + shutdown_test_codex(&codex).await?; Ok(()) } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn memories_startup_phase2_prunes_old_extension_resources_and_reports_them() -> Result<()> { +async fn memories_startup_phase2_prunes_old_extension_resources() -> Result<()> { let server = start_mock_server().await; let home = Arc::new(TempDir::new()?); let db = init_state_db(&home).await?; let now = Utc::now(); - let thread_id = seed_stage1_output( + let _thread_id = seed_stage1_output( db.as_ref(), home.path(), now - ChronoDuration::hours(1), @@ -175,11 +128,11 @@ async fn memories_startup_phase2_prunes_old_extension_resources_and_reports_them ) .await?; - let chronicle_resources = home.path().join("memories_extensions/chronicle/resources"); + let chronicle_resources = home.path().join("memories/extensions/chronicle/resources"); tokio::fs::create_dir_all(&chronicle_resources).await?; tokio::fs::write( home.path() - .join("memories_extensions/chronicle/instructions.md"), + .join("memories/extensions/chronicle/instructions.md"), "instructions", ) .await?; @@ -210,23 +163,11 @@ async fn memories_startup_phase2_prunes_old_extension_resources_and_reports_them let prompt = phase2_prompt_text(&request); assert!( - prompt.contains("Memory extension resources removed by retention pruning:"), - "expected extension resource prune report in prompt: {prompt}" - ); - assert!( - prompt.contains("- retention window: 7 days"), - "expected retention window in prompt: {prompt}" - ); - assert!( - prompt.contains("- extension: chronicle"), - "expected extension name in prompt: {prompt}" - ); - assert!( - prompt.contains(&format!(" - resources/{old_file_name}")), - "expected old resource in prompt: {prompt}" + prompt.contains("phase2_workspace_diff.md"), + "expected workspace diff file in prompt: {prompt}" ); - wait_for_phase2_success(db.as_ref(), thread_id).await?; + wait_for_phase2_workspace_reset(&home.path().join("memories")).await?; wait_for_file_removed(&old_file).await?; assert!( !tokio::fs::try_exists(&old_file).await?, @@ -242,8 +183,8 @@ async fn memories_startup_phase2_prunes_old_extension_resources_and_reports_them } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn memories_startup_phase2_processes_old_extension_resources_without_stage1_input() --> Result<()> { +async fn memories_startup_phase2_prunes_old_extension_resources_without_stage1_input() -> Result<()> +{ let server = start_mock_server().await; let home = Arc::new(TempDir::new()?); let db = init_state_db(&home).await?; @@ -251,11 +192,11 @@ async fn memories_startup_phase2_processes_old_extension_resources_without_stage .await?; let now = Utc::now(); - let chronicle_resources = home.path().join("memories_extensions/chronicle/resources"); + let chronicle_resources = home.path().join("memories/extensions/chronicle/resources"); tokio::fs::create_dir_all(&chronicle_resources).await?; tokio::fs::write( home.path() - .join("memories_extensions/chronicle/instructions.md"), + .join("memories/extensions/chronicle/instructions.md"), "instructions", ) .await?; @@ -281,189 +222,16 @@ async fn memories_startup_phase2_processes_old_extension_resources_without_stage let prompt = phase2_prompt_text(&request); assert!( - prompt.contains("- selected inputs this run: 0"), - "expected no selected raw inputs in prompt: {prompt}" - ); - assert!( - prompt.contains("Memory extension resources removed by retention pruning:"), - "expected extension resource prune report in prompt: {prompt}" - ); - assert!( - prompt.contains(&format!(" - resources/{old_file_name}")), - "expected old resource in prompt: {prompt}" + prompt.contains("phase2_workspace_diff.md"), + "expected workspace diff file in prompt: {prompt}" ); wait_for_file_removed(&old_file).await?; + wait_for_phase2_workspace_reset(&home.path().join("memories")).await?; shutdown_test_codex(&codex).await?; Ok(()) } -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn web_search_pollution_moves_selected_thread_into_removed_phase2_inputs() -> Result<()> { - let server = start_mock_server().await; - let home = Arc::new(TempDir::new()?); - let db = init_state_db(&home).await?; - - let mut initial_builder = test_codex().with_home(home.clone()).with_config(|config| { - config - .features - .enable(Feature::Sqlite) - .expect("test config should allow feature update"); - config - .features - .enable(Feature::MemoryTool) - .expect("test config should allow feature update"); - config.memories.max_raw_memories_for_consolidation = 1; - config.memories.disable_on_external_context = true; - }); - let initial = initial_builder.build(&server).await?; - mount_sse_once( - &server, - sse(vec![ - ev_response_created("resp-initial-1"), - ev_assistant_message("msg-initial-1", "initial turn complete"), - ev_completed("resp-initial-1"), - ]), - ) - .await; - initial.submit_turn("hello before memories").await?; - let rollout_path = initial - .session_configured - .rollout_path - .clone() - .expect("rollout path"); - let thread_id = initial.session_configured.session_id; - let updated_at = { - let deadline = Instant::now() + Duration::from_secs(10); - loop { - if let Some(metadata) = db.get_thread(thread_id).await? { - break metadata.updated_at; - } - assert!( - Instant::now() < deadline, - "timed out waiting for thread metadata for {thread_id}" - ); - tokio::time::sleep(Duration::from_millis(50)).await; - } - }; - - seed_stage1_output_for_existing_thread( - db.as_ref(), - thread_id, - updated_at.timestamp(), - "raw memory seeded for web search pollution", - "rollout summary seeded for web search pollution", - Some("pollution-rollout"), - ) - .await?; - - shutdown_test_codex(&initial).await?; - - let responses = mount_sse_sequence( - &server, - vec![ - sse(vec![ - ev_response_created("resp-phase2-1"), - ev_assistant_message("msg-phase2-1", "phase2 complete"), - ev_completed("resp-phase2-1"), - ]), - sse(vec![ - ev_response_created("resp-web-1"), - ev_web_search_call_done("ws-1", "completed", "weather seattle"), - ev_completed("resp-web-1"), - ]), - ], - ) - .await; - - let mut resumed_builder = test_codex().with_home(home.clone()).with_config(|config| { - config - .features - .enable(Feature::Sqlite) - .expect("test config should allow feature update"); - config - .features - .enable(Feature::MemoryTool) - .expect("test config should allow feature update"); - config.memories.max_raw_memories_for_consolidation = 1; - config.memories.disable_on_external_context = true; - }); - let resumed = resumed_builder - .resume(&server, home.clone(), rollout_path.clone()) - .await?; - - let first_phase2_request = wait_for_request(&responses, /*expected_count*/ 1) - .await - .remove(0); - let first_phase2_prompt = phase2_prompt_text(&first_phase2_request); - assert!( - first_phase2_prompt.contains("- selected inputs this run: 1"), - "expected seeded thread to be selected before pollution: {first_phase2_prompt}" - ); - assert!( - first_phase2_prompt.contains("- newly added since the last successful Phase 2 run: 1"), - "expected seeded thread to be added before pollution: {first_phase2_prompt}" - ); - assert!( - first_phase2_prompt.contains(&format!("- [added] thread_id={thread_id},")), - "expected selected thread in first phase2 prompt: {first_phase2_prompt}" - ); - - wait_for_phase2_success(db.as_ref(), thread_id).await?; - - resumed - .submit_turn("search the web for weather seattle") - .await?; - assert_eq!( - { - let deadline = Instant::now() + Duration::from_secs(10); - loop { - let memory_mode = db.get_thread_memory_mode(thread_id).await?; - if memory_mode.as_deref() == Some("polluted") { - break memory_mode; - } - assert!( - Instant::now() < deadline, - "timed out waiting for polluted memory mode for {thread_id}" - ); - tokio::time::sleep(Duration::from_millis(50)).await; - } - } - .as_deref(), - Some("polluted") - ); - - let selection = { - let deadline = Instant::now() + Duration::from_secs(10); - loop { - let selection = db - .get_phase2_input_selection(/*n*/ 1, /*max_unused_days*/ 30) - .await?; - if selection.selected.is_empty() - && selection.retained_thread_ids.is_empty() - && selection.removed.len() == 1 - && selection.removed[0].thread_id == thread_id - { - break selection; - } - assert!( - Instant::now() < deadline, - "timed out waiting for polluted thread to move into removed phase2 inputs: \ - {selection:?}" - ); - tokio::time::sleep(Duration::from_millis(50)).await; - } - }; - assert_eq!(responses.requests().len(), 2); - assert!(selection.selected.is_empty()); - assert_eq!(selection.retained_thread_ids, Vec::::new()); - assert_eq!(selection.removed.len(), 1); - assert_eq!(selection.removed[0].thread_id, thread_id); - - shutdown_test_codex(&resumed).await?; - Ok(()) -} - async fn build_test_codex(server: &wiremock::MockServer, home: Arc) -> Result { #[allow(clippy::expect_used)] let mut builder = test_codex().with_home(home).with_config(|config| { @@ -560,30 +328,22 @@ fn phase2_prompt_text(request: &ResponsesRequest) -> String { request .message_input_texts("user") .into_iter() - .find(|text| text.contains("Current selected Phase 1 inputs:")) + .find(|text| text.contains("Memory workspace diff:")) .expect("phase2 prompt text") } -async fn wait_for_phase2_success( - db: &codex_state::StateRuntime, - expected_thread_id: ThreadId, -) -> Result<()> { +async fn wait_for_phase2_workspace_reset(memory_root: &Path) -> Result<()> { + wait_for_file_removed(&memory_root.join("phase2_workspace_diff.md")).await?; let deadline = Instant::now() + Duration::from_secs(10); loop { - let selection = db - .get_phase2_input_selection(/*n*/ 1, /*max_unused_days*/ 30) - .await?; - if selection.selected.len() == 1 - && selection.selected[0].thread_id == expected_thread_id - && selection.retained_thread_ids == vec![expected_thread_id] - && selection.removed.is_empty() + if let Ok(diff) = diff_since_latest_init(memory_root).await + && !diff.has_changes() { return Ok(()); } - assert!( Instant::now() < deadline, - "timed out waiting for phase2 success for {expected_thread_id}" + "timed out waiting for clean memory workspace baseline" ); tokio::time::sleep(Duration::from_millis(50)).await; } diff --git a/codex-rs/git-utils/README.md b/codex-rs/git-utils/README.md index 1fd1051e3b..30a209e3bd 100644 --- a/codex-rs/git-utils/README.md +++ b/codex-rs/git-utils/README.md @@ -3,9 +3,11 @@ Helpers for interacting with git, including patch application and worktree snapshot utilities. The crate also exposes a lightweight baseline API for internal directories that use git only as a resettable diff mechanism: -`reset_git_repository` replaces `root/.git` with a fresh one-commit baseline, -and `diff_since_latest_init` returns structured file changes plus a unified -diff from that baseline to the current directory contents. +`ensure_git_baseline_repository` preserves a usable `root/.git` baseline or +creates one when it is missing or unusable, `reset_git_repository` replaces +`root/.git` with a fresh one-commit baseline, and `diff_since_latest_init` +returns structured file changes plus a unified diff from that baseline to the +current directory contents. ```rust,no_run use std::path::Path; diff --git a/codex-rs/git-utils/src/baseline.rs b/codex-rs/git-utils/src/baseline.rs index 5239598a2a..c63b894049 100644 --- a/codex-rs/git-utils/src/baseline.rs +++ b/codex-rs/git-utils/src/baseline.rs @@ -12,6 +12,8 @@ use std::path::Path; use std::path::PathBuf; use tokio::task; +use crate::operations::run_git_for_status; + const BASELINE_COMMIT_MESSAGE: &str = "Initialize Codex git baseline\n\nCo-authored-by: Codex "; @@ -65,18 +67,40 @@ struct GitBaselineFileEntry { /// This is intentionally destructive for `root/.git`. It is meant for internal directories where /// git is used only as a baseline/diff implementation detail, not for user repositories. pub async fn reset_git_repository(root: &Path) -> anyhow::Result<()> { + let root = root.to_path_buf(); + task::spawn_blocking(move || reset_git_repository_sync(&root)).await? +} + +/// Ensures `root` has a usable git baseline repository. +/// +/// Existing usable `.git/` metadata is preserved. Missing or unusable metadata is replaced with a +/// fresh one-commit baseline. +pub async fn ensure_git_baseline_repository(root: &Path) -> anyhow::Result<()> { let root = root.to_path_buf(); task::spawn_blocking(move || { fs::create_dir_all(&root) .with_context(|| format!("create git baseline root {}", root.display()))?; - remove_git_metadata(&root)?; - let repo = gix::init(&root).with_context(|| format!("init git repo {}", root.display()))?; - commit_current_tree(&repo, BASELINE_COMMIT_MESSAGE)?; - anyhow::Ok(()) + if root.join(".git").is_dir() + && let Ok(repo) = gix::open(&root) + && head_file_entries(&repo).is_ok() + { + return Ok(()); + } + reset_git_repository_sync(&root) }) .await? } +fn reset_git_repository_sync(root: &Path) -> anyhow::Result<()> { + fs::create_dir_all(root) + .with_context(|| format!("create git baseline root {}", root.display()))?; + remove_git_metadata(root)?; + let repo = gix::init(root).with_context(|| format!("init git repo {}", root.display()))?; + commit_current_tree(&repo, BASELINE_COMMIT_MESSAGE)?; + write_index_from_head(root)?; + Ok(()) +} + /// Returns the diff between the latest baseline reset and the current directory contents. pub async fn diff_since_latest_init(root: &Path) -> anyhow::Result { let root = root.to_path_buf(); @@ -130,6 +154,11 @@ fn commit_current_tree(repo: &gix::Repository, message: &str) -> anyhow::Result< Ok(()) } +fn write_index_from_head(root: &Path) -> anyhow::Result<()> { + run_git_for_status(root, ["read-tree", "--reset", "HEAD"], /*env*/ None) + .context("write git baseline index from HEAD") +} + fn codex_signature() -> gix::actor::Signature { gix::actor::Signature { name: "Codex".into(), @@ -501,8 +530,24 @@ mod tests { use super::*; use pretty_assertions::assert_eq; use std::fs; + use std::process::Command; use tempfile::TempDir; + fn git_stdout(root: &Path, args: &[&str]) -> String { + let output = Command::new("git") + .current_dir(root) + .args(args) + .output() + .expect("run git command"); + assert!( + output.status.success(), + "git command failed: {args:?}\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8_lossy(&output.stdout).to_string() + } + #[tokio::test] async fn reset_creates_fresh_baseline() { let home = TempDir::new().expect("tempdir"); @@ -513,9 +558,30 @@ mod tests { reset_git_repository(&root).await.expect("reset repo"); assert!(root.join(".git").is_dir()); + assert!(root.join(".git/index").is_file()); let diff = diff_since_latest_init(&root).await.expect("diff"); assert!(!diff.has_changes()); assert_eq!(diff.unified_diff, ""); + assert_eq!(git_stdout(&root, &["status", "--porcelain"]), ""); + assert_eq!(git_stdout(&root, &["ls-files"]), "MEMORY.md\n"); + } + + #[tokio::test] + async fn ensure_recovers_from_unborn_repository() { + let home = TempDir::new().expect("tempdir"); + let root = home.path().join("repo"); + fs::create_dir_all(&root).expect("create root"); + fs::write(root.join("MEMORY.md"), "memory").expect("write memory"); + gix::init(&root).expect("init git repo without baseline commit"); + + ensure_git_baseline_repository(&root) + .await + .expect("ensure repo"); + + let diff = diff_since_latest_init(&root).await.expect("diff"); + assert!(!diff.has_changes()); + assert_eq!(git_stdout(&root, &["status", "--porcelain"]), ""); + assert_eq!(git_stdout(&root, &["ls-files"]), "MEMORY.md\n"); } #[tokio::test] diff --git a/codex-rs/git-utils/src/lib.rs b/codex-rs/git-utils/src/lib.rs index 5973b9cc41..ea7685b675 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -17,6 +17,7 @@ pub use baseline::GitBaselineChange; pub use baseline::GitBaselineChangeStatus; pub use baseline::GitBaselineDiff; pub use baseline::diff_since_latest_init; +pub use baseline::ensure_git_baseline_repository; pub use baseline::reset_git_repository; pub use branch::merge_base_with_head; pub use codex_protocol::models::GhostCommit; diff --git a/codex-rs/state/src/lib.rs b/codex-rs/state/src/lib.rs index c3dacae715..005cfa4958 100644 --- a/codex-rs/state/src/lib.rs +++ b/codex-rs/state/src/lib.rs @@ -14,7 +14,6 @@ mod runtime; pub use model::LogEntry; pub use model::LogQuery; pub use model::LogRow; -pub use model::Phase2InputSelection; pub use model::Phase2JobClaimOutcome; /// Preferred entrypoint: owns configuration and metrics. pub use runtime::StateRuntime; @@ -42,7 +41,6 @@ pub use model::SortKey; pub use model::Stage1JobClaim; pub use model::Stage1JobClaimOutcome; pub use model::Stage1Output; -pub use model::Stage1OutputRef; pub use model::Stage1StartupClaimParams; pub use model::ThreadGoal; pub use model::ThreadGoalStatus; diff --git a/codex-rs/state/src/model/memories.rs b/codex-rs/state/src/model/memories.rs index 0e663bf904..006b51a0db 100644 --- a/codex-rs/state/src/model/memories.rs +++ b/codex-rs/state/src/model/memories.rs @@ -22,21 +22,6 @@ pub struct Stage1Output { pub generated_at: DateTime, } -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct Stage1OutputRef { - pub thread_id: ThreadId, - pub source_updated_at: DateTime, - pub rollout_slug: Option, -} - -#[derive(Debug, Clone, PartialEq, Eq, Default)] -pub struct Phase2InputSelection { - pub selected: Vec, - pub previous_selected: Vec, - pub retained_thread_ids: Vec, - pub removed: Vec, -} - #[derive(Debug)] pub(crate) struct Stage1OutputRow { thread_id: String, @@ -89,18 +74,6 @@ fn epoch_seconds_to_datetime(secs: i64) -> Result> { .ok_or_else(|| anyhow::anyhow!("invalid unix timestamp: {secs}")) } -pub(crate) fn stage1_output_ref_from_parts( - thread_id: String, - source_updated_at: i64, - rollout_slug: Option, -) -> Result { - Ok(Stage1OutputRef { - thread_id: ThreadId::try_from(thread_id)?, - source_updated_at: epoch_seconds_to_datetime(source_updated_at)?, - rollout_slug, - }) -} - /// Result of trying to claim a stage-1 memory extraction job. #[derive(Debug, Clone, PartialEq, Eq)] pub enum Stage1JobClaimOutcome { @@ -136,14 +109,14 @@ pub struct Stage1StartupClaimParams<'a> { /// Result of trying to claim a phase-2 consolidation job. #[derive(Debug, Clone, PartialEq, Eq)] pub enum Phase2JobClaimOutcome { - /// The caller owns the global lock and should spawn consolidation. + /// The caller owns the global lock and may inspect the memory workspace. Claimed { ownership_token: String, /// Snapshot of `input_watermark` at claim time. input_watermark: i64, }, - /// The global job is not pending consolidation (or is already up to date). - SkippedNotDirty, + /// The global job is in retry backoff or has exhausted its retry budget. + SkippedRetryUnavailable, /// Another worker currently owns a fresh global consolidation lease. SkippedRunning, } diff --git a/codex-rs/state/src/model/mod.rs b/codex-rs/state/src/model/mod.rs index 213ae81fea..a431bc64c0 100644 --- a/codex-rs/state/src/model/mod.rs +++ b/codex-rs/state/src/model/mod.rs @@ -19,12 +19,10 @@ pub use graph::DirectionalThreadSpawnEdgeStatus; pub use log::LogEntry; pub use log::LogQuery; pub use log::LogRow; -pub use memories::Phase2InputSelection; pub use memories::Phase2JobClaimOutcome; pub use memories::Stage1JobClaim; pub use memories::Stage1JobClaimOutcome; pub use memories::Stage1Output; -pub use memories::Stage1OutputRef; pub use memories::Stage1StartupClaimParams; pub use thread_goal::ThreadGoal; pub use thread_goal::ThreadGoalStatus; @@ -40,7 +38,6 @@ pub use thread_metadata::ThreadsPage; pub(crate) use agent_job::AgentJobItemRow; pub(crate) use agent_job::AgentJobRow; pub(crate) use memories::Stage1OutputRow; -pub(crate) use memories::stage1_output_ref_from_parts; pub(crate) use thread_goal::ThreadGoalRow; pub(crate) use thread_metadata::ThreadRow; pub(crate) use thread_metadata::anchor_from_item; diff --git a/codex-rs/state/src/runtime/memories.rs b/codex-rs/state/src/runtime/memories.rs index b9cf432e43..1e051720d5 100644 --- a/codex-rs/state/src/runtime/memories.rs +++ b/codex-rs/state/src/runtime/memories.rs @@ -3,7 +3,6 @@ use super::threads::push_thread_filters; use super::threads::push_thread_order_and_limit; use super::*; use crate::SortDirection; -use crate::model::Phase2InputSelection; use crate::model::Phase2JobClaimOutcome; use crate::model::Stage1JobClaim; use crate::model::Stage1JobClaimOutcome; @@ -11,12 +10,10 @@ use crate::model::Stage1Output; use crate::model::Stage1OutputRow; use crate::model::Stage1StartupClaimParams; use crate::model::ThreadRow; -use crate::model::stage1_output_ref_from_parts; use chrono::Duration; use sqlx::Executor; use sqlx::QueryBuilder; use sqlx::Sqlite; -use std::collections::HashSet; use uuid::Uuid; const JOB_KIND_MEMORY_STAGE1: &str = "memory_stage1"; @@ -331,8 +328,7 @@ WHERE thread_id IN ( Ok(rows_affected as usize) } - /// Returns the current phase-2 input set along with its diff against the - /// last successful phase-2 selection. + /// Returns the current phase-2 input set. /// /// Query behavior: /// - current selection keeps only non-empty stage-1 outputs whose @@ -342,22 +338,17 @@ WHERE thread_id IN ( /// - eligible rows are ordered by `usage_count DESC`, /// `COALESCE(last_usage, source_updated_at) DESC`, `source_updated_at DESC`, /// `thread_id DESC` - /// - previously selected rows are identified by `selected_for_phase2 = 1` - /// - `previous_selected` contains the current persisted rows that belonged - /// to the last successful phase-2 baseline, even if those threads are no - /// longer memory-eligible - /// - `retained_thread_ids` records which current rows still match the exact - /// snapshot selected in the last successful phase-2 run - /// - removed rows are previously selected rows that are still present in - /// `stage1_outputs` but are no longer in the current selection, including - /// threads that are no longer memory-eligible + /// + /// The returned rows are the complete Phase 2 filesystem input. Phase 2 + /// syncs these rows directly; deletions are represented by the workspace + /// diff against the previous successful memory baseline. pub async fn get_phase2_input_selection( &self, n: usize, max_unused_days: i64, - ) -> anyhow::Result { + ) -> anyhow::Result> { if n == 0 { - return Ok(Phase2InputSelection::default()); + return Ok(Vec::new()); } let cutoff = (Utc::now() - Duration::days(max_unused_days.max(0))).timestamp(); @@ -372,9 +363,7 @@ SELECT so.rollout_slug, so.generated_at, COALESCE(t.cwd, '') AS cwd, - t.git_branch AS git_branch, - so.selected_for_phase2, - so.selected_for_phase2_source_updated_at + t.git_branch AS git_branch FROM stage1_outputs AS so LEFT JOIN threads AS t ON t.id = so.thread_id @@ -398,70 +387,14 @@ LIMIT ? .fetch_all(self.pool.as_ref()) .await?; - let mut current_thread_ids = HashSet::with_capacity(current_rows.len()); let mut selected = Vec::with_capacity(current_rows.len()); - let mut retained_thread_ids = Vec::new(); for row in current_rows { - let thread_id = row.try_get::("thread_id")?; - current_thread_ids.insert(thread_id.clone()); - let source_updated_at = row.try_get::("source_updated_at")?; - if row.try_get::("selected_for_phase2")? != 0 - && row.try_get::, _>("selected_for_phase2_source_updated_at")? - == Some(source_updated_at) - { - retained_thread_ids.push(ThreadId::try_from(thread_id.clone())?); - } selected.push(Stage1Output::try_from(Stage1OutputRow::try_from_row( &row, )?)?); } - let previous_rows = sqlx::query( - r#" -SELECT - so.thread_id, - COALESCE(t.rollout_path, '') AS rollout_path, - so.source_updated_at, - so.raw_memory, - so.rollout_summary, - so.rollout_slug, - so.generated_at, - COALESCE(t.cwd, '') AS cwd, - t.git_branch AS git_branch -FROM stage1_outputs AS so -LEFT JOIN threads AS t - ON t.id = so.thread_id -WHERE so.selected_for_phase2 = 1 -ORDER BY so.source_updated_at DESC, so.thread_id DESC - "#, - ) - .fetch_all(self.pool.as_ref()) - .await?; - - let previous_selected = previous_rows - .iter() - .map(Stage1OutputRow::try_from_row) - .map(|row| row.and_then(Stage1Output::try_from)) - .collect::, _>>()?; - let mut removed = Vec::new(); - for row in previous_rows { - let thread_id = row.try_get::("thread_id")?; - if current_thread_ids.contains(thread_id.as_str()) { - continue; - } - removed.push(stage1_output_ref_from_parts( - thread_id, - row.try_get("source_updated_at")?, - row.try_get("rollout_slug")?, - )?); - } - - Ok(Phase2InputSelection { - selected, - previous_selected, - retained_thread_ids, - removed, - }) + Ok(selected) } /// Marks a thread as polluted and enqueues phase-2 forgetting when the @@ -909,19 +842,22 @@ WHERE kind = ? AND job_key = ? /// Enqueues or advances the global phase-2 consolidation job watermark. /// /// The underlying upsert keeps the job `running` when already running, resets - /// `pending/error` jobs to `pending`, and advances `input_watermark` so each - /// enqueue marks new consolidation work even when `source_updated_at` is - /// older than prior maxima. + /// `pending/error` jobs to `pending`, and advances `input_watermark` as + /// bookkeeping even when `source_updated_at` is older than prior maxima. + /// Phase 2 does not use this watermark as a dirty check; git workspace diffing + /// decides whether consolidation work exists after the lock is claimed. pub async fn enqueue_global_consolidation(&self, input_watermark: i64) -> anyhow::Result<()> { enqueue_global_consolidation_with_executor(self.pool.as_ref(), input_watermark).await } - /// Attempts to claim the global phase-2 consolidation job. + /// Attempts to claim the global phase-2 consolidation lock. /// /// Claim semantics: /// - reads the singleton global job row (`kind='memory_consolidate_global'`) - /// - returns `SkippedNotDirty` when `input_watermark <= last_success_watermark` - /// - returns `SkippedNotDirty` when retries are exhausted or retry backoff is active + /// - creates and claims the singleton row when it does not exist yet + /// - does not use DB watermarks to decide whether Phase 2 has work; git workspace + /// dirtiness is the source of truth after the caller materializes inputs + /// - returns `SkippedRetryUnavailable` when retries are exhausted or retry backoff is active /// - returns `SkippedRunning` when an active running lease exists /// - otherwise updates the row to `running`, sets ownership + lease, and /// returns `Claimed` @@ -939,7 +875,7 @@ WHERE kind = ? AND job_key = ? let existing_job = sqlx::query( r#" -SELECT status, lease_until, retry_at, retry_remaining, input_watermark, last_success_watermark +SELECT status, lease_until, retry_at, retry_remaining, input_watermark FROM jobs WHERE kind = ? AND job_key = ? "#, @@ -950,18 +886,49 @@ WHERE kind = ? AND job_key = ? .await?; let Some(existing_job) = existing_job else { + let rows_affected = sqlx::query( + r#" +INSERT INTO jobs ( + kind, + job_key, + status, + worker_id, + ownership_token, + started_at, + finished_at, + lease_until, + retry_at, + retry_remaining, + last_error, + input_watermark, + last_success_watermark +) VALUES (?, ?, 'running', ?, ?, ?, NULL, ?, NULL, ?, NULL, 0, 0) + "#, + ) + .bind(JOB_KIND_MEMORY_CONSOLIDATE_GLOBAL) + .bind(MEMORY_CONSOLIDATION_JOB_KEY) + .bind(worker_id.as_str()) + .bind(ownership_token.as_str()) + .bind(now) + .bind(lease_until) + .bind(DEFAULT_RETRY_REMAINING) + .execute(&mut *tx) + .await? + .rows_affected(); + tx.commit().await?; - return Ok(Phase2JobClaimOutcome::SkippedNotDirty); + return if rows_affected == 0 { + Ok(Phase2JobClaimOutcome::SkippedRunning) + } else { + Ok(Phase2JobClaimOutcome::Claimed { + ownership_token, + input_watermark: 0, + }) + }; }; let input_watermark: Option = existing_job.try_get("input_watermark")?; let input_watermark_value = input_watermark.unwrap_or(0); - let last_success_watermark: Option = existing_job.try_get("last_success_watermark")?; - if input_watermark_value <= last_success_watermark.unwrap_or(0) { - tx.commit().await?; - return Ok(Phase2JobClaimOutcome::SkippedNotDirty); - } - let status: String = existing_job.try_get("status")?; let existing_lease_until: Option = existing_job.try_get("lease_until")?; let retry_at: Option = existing_job.try_get("retry_at")?; @@ -969,11 +936,11 @@ WHERE kind = ? AND job_key = ? if retry_remaining <= 0 { tx.commit().await?; - return Ok(Phase2JobClaimOutcome::SkippedNotDirty); + return Ok(Phase2JobClaimOutcome::SkippedRetryUnavailable); } if retry_at.is_some_and(|retry_at| retry_at > now) { tx.commit().await?; - return Ok(Phase2JobClaimOutcome::SkippedNotDirty); + return Ok(Phase2JobClaimOutcome::SkippedRetryUnavailable); } if status == "running" && existing_lease_until.is_some_and(|lease_until| lease_until > now) { @@ -994,7 +961,6 @@ SET retry_at = NULL, last_error = NULL WHERE kind = ? AND job_key = ? - AND input_watermark > COALESCE(last_success_watermark, 0) AND (status != 'running' OR lease_until IS NULL OR lease_until <= ?) AND (retry_at IS NULL OR retry_at <= ?) AND retry_remaining > 0 @@ -1063,8 +1029,7 @@ WHERE kind = ? AND job_key = ? /// `max(existing_last_success_watermark, completed_watermark)` /// - rewrites `selected_for_phase2` so only the exact selected stage-1 /// snapshots remain marked as part of the latest successful phase-2 - /// selection, and persists each selected snapshot's - /// `source_updated_at` for future retained-vs-added diffing + /// selection, and persists each selected snapshot's `source_updated_at` pub async fn mark_global_phase2_job_succeeded( &self, ownership_token: &str, @@ -2271,16 +2236,6 @@ WHERE kind = 'memory_stage1' "no-output without an existing stage1 output should not enqueue phase2" ); - let claim_phase2 = runtime - .try_claim_global_phase2_job(owner, /*lease_seconds*/ 3600) - .await - .expect("claim phase2"); - assert_eq!( - claim_phase2, - Phase2JobClaimOutcome::SkippedNotDirty, - "phase2 should remain clean when no-output deleted nothing" - ); - let _ = tokio::fs::remove_dir_all(codex_home).await; } @@ -2350,7 +2305,7 @@ WHERE kind = 'memory_stage1' ) .await .expect("mark initial phase2 succeeded"), - "initial phase2 success should clear global dirty state" + "initial phase2 success should finalize the global job" ); let no_output_claim = runtime @@ -2505,7 +2460,7 @@ WHERE kind = 'memory_stage1' } #[tokio::test] - async fn phase2_global_consolidation_reruns_when_watermark_advances() { + async fn phase2_global_lock_can_be_reclaimed_after_success_without_new_watermark() { let codex_home = unique_temp_dir(); let runtime = StateRuntime::init(codex_home.clone(), "test-provider".to_string()) .await @@ -2537,24 +2492,13 @@ WHERE kind = 'memory_stage1' "phase2 success should finalize for current token" ); - let claim_up_to_date = runtime + let claim_after_success = runtime .try_claim_global_phase2_job(owner, /*lease_seconds*/ 3600) .await - .expect("claim phase2 up-to-date"); - assert_eq!(claim_up_to_date, Phase2JobClaimOutcome::SkippedNotDirty); - - runtime - .enqueue_global_consolidation(/*input_watermark*/ 101) - .await - .expect("enqueue global consolidation again"); - - let claim_rerun = runtime - .try_claim_global_phase2_job(owner, /*lease_seconds*/ 3600) - .await - .expect("claim phase2 rerun"); + .expect("claim phase2 after success"); assert!( - matches!(claim_rerun, Phase2JobClaimOutcome::Claimed { .. }), - "advanced watermark should be claimable" + matches!(claim_after_success, Phase2JobClaimOutcome::Claimed { .. }), + "the DB claim is only a lock; git workspace diff decides whether there is work" ); let _ = tokio::fs::remove_dir_all(codex_home).await; @@ -2801,7 +2745,7 @@ VALUES (?, ?, ?, ?, ?) } #[tokio::test] - async fn get_phase2_input_selection_reports_added_retained_and_removed_rows() { + async fn get_phase2_input_selection_returns_current_selected_rows() { let codex_home = unique_temp_dir(); let runtime = StateRuntime::init(codex_home.clone(), "test-provider".to_string()) .await @@ -2895,28 +2839,19 @@ VALUES (?, ?, ?, ?, ?) .await .expect("load phase2 input selection"); - assert_eq!(selection.selected.len(), 2); - assert_eq!(selection.previous_selected.len(), 2); - assert_eq!(selection.selected[0].thread_id, thread_id_c); + assert_eq!(selection.len(), 2); + assert_eq!(selection[0].thread_id, thread_id_c); assert_eq!( - selection.selected[0].rollout_path, + selection[0].rollout_path, codex_home.join(format!("rollout-{thread_id_c}.jsonl")) ); - assert_eq!(selection.selected[1].thread_id, thread_id_b); - assert_eq!(selection.retained_thread_ids, vec![thread_id_c]); - - assert_eq!(selection.removed.len(), 1); - assert_eq!(selection.removed[0].thread_id, thread_id_a); - assert_eq!( - selection.removed[0].rollout_slug.as_deref(), - Some("rollout-a") - ); + assert_eq!(selection[1].thread_id, thread_id_b); let _ = tokio::fs::remove_dir_all(codex_home).await; } #[tokio::test] - async fn get_phase2_input_selection_marks_polluted_previous_selection_as_removed() { + async fn get_phase2_input_selection_excludes_polluted_previous_selection() { let codex_home = unique_temp_dir(); let runtime = StateRuntime::init(codex_home.clone(), "test-provider".to_string()) .await @@ -3002,24 +2937,8 @@ VALUES (?, ?, ?, ?, ?) .await .expect("load phase2 input selection"); - assert_eq!(selection.selected.len(), 1); - assert_eq!(selection.selected[0].thread_id, thread_id_enabled); - assert_eq!(selection.previous_selected.len(), 2); - assert!( - selection - .previous_selected - .iter() - .any(|item| item.thread_id == thread_id_enabled) - ); - assert!( - selection - .previous_selected - .iter() - .any(|item| item.thread_id == thread_id_polluted) - ); - assert_eq!(selection.retained_thread_ids, vec![thread_id_enabled]); - assert_eq!(selection.removed.len(), 1); - assert_eq!(selection.removed[0].thread_id, thread_id_polluted); + assert_eq!(selection.len(), 1); + assert_eq!(selection[0].thread_id, thread_id_enabled); let _ = tokio::fs::remove_dir_all(codex_home).await; } @@ -3113,7 +3032,7 @@ VALUES (?, ?, ?, ?, ?) } #[tokio::test] - async fn get_phase2_input_selection_treats_regenerated_selected_rows_as_added() { + async fn get_phase2_input_selection_returns_regenerated_selected_rows() { let codex_home = unique_temp_dir(); let runtime = StateRuntime::init(codex_home.clone(), "test-provider".to_string()) .await @@ -3213,12 +3132,9 @@ VALUES (?, ?, ?, ?, ?) .get_phase2_input_selection(/*n*/ 1, /*max_unused_days*/ 36_500) .await .expect("load phase2 input selection"); - assert_eq!(selection.selected.len(), 1); - assert_eq!(selection.previous_selected.len(), 1); - assert_eq!(selection.selected[0].thread_id, thread_id); - assert_eq!(selection.selected[0].source_updated_at.timestamp(), 101); - assert!(selection.retained_thread_ids.is_empty()); - assert!(selection.removed.is_empty()); + assert_eq!(selection.len(), 1); + assert_eq!(selection[0].thread_id, thread_id); + assert_eq!(selection[0].source_updated_at.timestamp(), 101); let (selected_for_phase2, selected_for_phase2_source_updated_at) = sqlx::query_as::<_, (i64, Option)>( @@ -3235,7 +3151,7 @@ VALUES (?, ?, ?, ?, ?) } #[tokio::test] - async fn get_phase2_input_selection_reports_regenerated_previous_selection_as_removed() { + async fn get_phase2_input_selection_uses_current_ranking_after_refreshes() { let codex_home = unique_temp_dir(); let runtime = StateRuntime::init(codex_home.clone(), "test-provider".to_string()) .await @@ -3368,29 +3284,11 @@ VALUES (?, ?, ?, ?, ?) .expect("load phase2 input selection"); assert_eq!( selection - .selected .iter() .map(|output| output.thread_id) .collect::>(), vec![thread_id_d, thread_id_c] ); - assert_eq!( - selection - .previous_selected - .iter() - .map(|output| output.thread_id) - .collect::>(), - vec![thread_id_a, thread_id_b] - ); - assert!(selection.retained_thread_ids.is_empty()); - assert_eq!( - selection - .removed - .iter() - .map(|output| (output.thread_id, output.source_updated_at.timestamp())) - .collect::>(), - vec![(thread_id_a, 102), (thread_id_b, 101)] - ); let _ = tokio::fs::remove_dir_all(codex_home).await; } @@ -3527,7 +3425,8 @@ VALUES (?, ?, ?, ?, ?) .get_phase2_input_selection(/*n*/ 1, /*max_unused_days*/ 36_500) .await .expect("load phase2 input selection after refresh"); - assert_eq!(selection.retained_thread_ids, vec![thread_id]); + assert_eq!(selection.len(), 1); + assert_eq!(selection[0].thread_id, thread_id); let (selected_for_phase2, selected_for_phase2_source_updated_at) = sqlx::query_as::<_, (i64, Option)>( @@ -3657,9 +3556,8 @@ VALUES (?, ?, ?, ?, ?) .get_phase2_input_selection(/*n*/ 1, /*max_unused_days*/ 36_500) .await .expect("load phase2 input selection"); - assert_eq!(selection.selected.len(), 1); - assert_eq!(selection.selected[0].source_updated_at.timestamp(), 101); - assert!(selection.retained_thread_ids.is_empty()); + assert_eq!(selection.len(), 1); + assert_eq!(selection[0].source_updated_at.timestamp(), 101); let _ = tokio::fs::remove_dir_all(codex_home).await; } @@ -3870,7 +3768,6 @@ VALUES (?, ?, ?, ?, ?) assert_eq!( selection - .selected .iter() .map(|output| output.thread_id) .collect::>(), @@ -3967,7 +3864,6 @@ VALUES (?, ?, ?, ?, ?) assert_eq!( selection - .selected .iter() .map(|output| output.thread_id) .collect::>(), @@ -4056,9 +3952,9 @@ VALUES (?, ?, ?, ?, ?) .await .expect("load phase2 input selection"); - assert_eq!(selection.selected.len(), 1); - assert_eq!(selection.selected[0].thread_id, newer_thread); - assert_eq!(selection.selected[0].source_updated_at.timestamp(), 200); + assert_eq!(selection.len(), 1); + assert_eq!(selection[0].thread_id, newer_thread); + assert_eq!(selection[0].source_updated_at.timestamp(), 200); let _ = tokio::fs::remove_dir_all(codex_home).await; } @@ -4411,6 +4307,59 @@ VALUES (?, ?, ?, ?, ?) let _ = tokio::fs::remove_dir_all(codex_home).await; } + #[tokio::test] + async fn phase2_global_lock_creates_missing_job_row() { + let codex_home = unique_temp_dir(); + let runtime = StateRuntime::init(codex_home.clone(), "test-provider".to_string()) + .await + .expect("initialize runtime"); + + let owner_a = ThreadId::from_string(&Uuid::new_v4().to_string()).expect("owner a"); + let owner_b = ThreadId::from_string(&Uuid::new_v4().to_string()).expect("owner b"); + + let claim = runtime + .try_claim_global_phase2_job(owner_a, /*lease_seconds*/ 3_600) + .await + .expect("claim global phase2 lock"); + let ownership_token = match claim { + Phase2JobClaimOutcome::Claimed { + ownership_token, + input_watermark, + } => { + assert_eq!(input_watermark, 0); + ownership_token + } + other => panic!("unexpected phase2 lock claim outcome: {other:?}"), + }; + + let second_claim = runtime + .try_claim_global_phase2_job(owner_b, /*lease_seconds*/ 3_600) + .await + .expect("claim global phase2 lock from second owner"); + assert_eq!(second_claim, Phase2JobClaimOutcome::SkippedRunning); + + assert!( + runtime + .mark_global_phase2_job_succeeded( + ownership_token.as_str(), + /*completed_watermark*/ 0, + &[] + ) + .await + .expect("mark phase2 lock success") + ); + let claim_after_success = runtime + .try_claim_global_phase2_job(owner_b, /*lease_seconds*/ 3_600) + .await + .expect("claim global phase2 lock after success"); + assert!( + matches!(claim_after_success, Phase2JobClaimOutcome::Claimed { .. }), + "git workspace diff, not the DB watermark, decides whether the claimed lock has work" + ); + + let _ = tokio::fs::remove_dir_all(codex_home).await; + } + #[tokio::test] async fn phase2_global_lock_stale_lease_allows_takeover() { let codex_home = unique_temp_dir(); @@ -4487,7 +4436,7 @@ VALUES (?, ?, ?, ?, ?) } #[tokio::test] - async fn phase2_backfilled_inputs_below_last_success_still_become_dirty() { + async fn enqueue_global_consolidation_keeps_phase2_input_watermark_monotonic() { let codex_home = unique_temp_dir(); let runtime = StateRuntime::init(codex_home.clone(), "test-provider".to_string()) .await @@ -4527,23 +4476,23 @@ VALUES (?, ?, ?, ?, ?) runtime .enqueue_global_consolidation(/*input_watermark*/ 400) .await - .expect("enqueue backfilled consolidation"); + .expect("enqueue lower-watermark consolidation"); let owner_b = ThreadId::from_string(&Uuid::new_v4().to_string()).expect("owner b"); let claim_b = runtime .try_claim_global_phase2_job(owner_b, /*lease_seconds*/ 3_600) .await - .expect("claim backfilled consolidation"); + .expect("claim lower-watermark consolidation"); match claim_b { Phase2JobClaimOutcome::Claimed { input_watermark, .. } => { assert!( input_watermark > 500, - "backfilled enqueue should advance dirty watermark beyond last success" + "lower-watermark enqueue should still advance the bookkeeping watermark" ); } - other => panic!("unexpected backfilled phase2 claim outcome: {other:?}"), + other => panic!("unexpected lower-watermark phase2 claim outcome: {other:?}"), } let _ = tokio::fs::remove_dir_all(codex_home).await; @@ -4608,7 +4557,7 @@ VALUES (?, ?, ?, ?, ?) .try_claim_global_phase2_job(ThreadId::new(), /*lease_seconds*/ 3_600) .await .expect("claim after fallback failure"); - assert_eq!(claim, Phase2JobClaimOutcome::SkippedNotDirty); + assert_eq!(claim, Phase2JobClaimOutcome::SkippedRetryUnavailable); let _ = tokio::fs::remove_dir_all(codex_home).await; } diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index e81f4476f4..f7d24e1e0e 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -1579,7 +1579,7 @@ async fn reset_memories_clears_local_memory_directories() -> Result<()> { app.config.sqlite_home = codex_home.path().to_path_buf(); let memory_root = codex_home.path().join("memories"); - let extensions_root = codex_home.path().join("memories_extensions"); + let extensions_root = memory_root.join("extensions"); std::fs::create_dir_all(memory_root.join("rollout_summaries"))?; std::fs::create_dir_all(&extensions_root)?; std::fs::write(memory_root.join("MEMORY.md"), "stale memory\n")?; @@ -1594,7 +1594,6 @@ async fn reset_memories_clears_local_memory_directories() -> Result<()> { app.reset_memories_with_app_server(&mut app_server).await; assert_eq!(std::fs::read_dir(&memory_root)?.count(), 0); - assert_eq!(std::fs::read_dir(&extensions_root)?.count(), 0); app_server.shutdown().await?; Ok(()) From 5d314f324c7ffc54ac24ecf464c45f9c4bcfa861 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 27 Apr 2026 14:58:11 +0200 Subject: [PATCH 04/13] Allow Phase 2 memory claims after retry exhaustion (#19809) ## Why The Phase 2 memories job row is only the global lock for the git-backed memory workspace. Manual memory edits do not enqueue new Stage 1 work, so a Phase 2 row with `retry_remaining = 0` could be skipped before the worker ever claimed the lock and generated `phase2_workspace_diff.md`. That left workspace-only changes unconsolidated after repeated failures, even when retry backoff had elapsed and the filesystem had real diffable work. ## What Changed - Allow `try_claim_global_phase2_job` to claim the Phase 2 lock after the retry budget is exhausted, while still respecting active `retry_at` backoff and fresh running leases. - Treat `SkippedRetryUnavailable` for Phase 2 as backoff-only, and update the outcome docs to match. - Clamp Phase 2 retry bookkeeping at zero when failed attempts are recorded. ## Verification - Added `phase2_global_lock_can_be_claimed_after_retry_budget_is_exhausted` to cover the exhausted-budget lock claim path. - Ran `cargo test -p codex-state`. --- codex-rs/state/src/model/memories.rs | 2 +- codex-rs/state/src/runtime/memories.rs | 87 ++++++++++++++++++++++---- 2 files changed, 76 insertions(+), 13 deletions(-) diff --git a/codex-rs/state/src/model/memories.rs b/codex-rs/state/src/model/memories.rs index 006b51a0db..ada9d4e1e4 100644 --- a/codex-rs/state/src/model/memories.rs +++ b/codex-rs/state/src/model/memories.rs @@ -115,7 +115,7 @@ pub enum Phase2JobClaimOutcome { /// Snapshot of `input_watermark` at claim time. input_watermark: i64, }, - /// The global job is in retry backoff or has exhausted its retry budget. + /// The global job is in retry backoff. SkippedRetryUnavailable, /// Another worker currently owns a fresh global consolidation lease. SkippedRunning, diff --git a/codex-rs/state/src/runtime/memories.rs b/codex-rs/state/src/runtime/memories.rs index 1e051720d5..62175d15ae 100644 --- a/codex-rs/state/src/runtime/memories.rs +++ b/codex-rs/state/src/runtime/memories.rs @@ -857,7 +857,7 @@ WHERE kind = ? AND job_key = ? /// - creates and claims the singleton row when it does not exist yet /// - does not use DB watermarks to decide whether Phase 2 has work; git workspace /// dirtiness is the source of truth after the caller materializes inputs - /// - returns `SkippedRetryUnavailable` when retries are exhausted or retry backoff is active + /// - returns `SkippedRetryUnavailable` when retry backoff is active /// - returns `SkippedRunning` when an active running lease exists /// - otherwise updates the row to `running`, sets ownership + lease, and /// returns `Claimed` @@ -875,7 +875,7 @@ WHERE kind = ? AND job_key = ? let existing_job = sqlx::query( r#" -SELECT status, lease_until, retry_at, retry_remaining, input_watermark +SELECT status, lease_until, retry_at, input_watermark FROM jobs WHERE kind = ? AND job_key = ? "#, @@ -932,12 +932,6 @@ INSERT INTO jobs ( let status: String = existing_job.try_get("status")?; let existing_lease_until: Option = existing_job.try_get("lease_until")?; let retry_at: Option = existing_job.try_get("retry_at")?; - let retry_remaining: i64 = existing_job.try_get("retry_remaining")?; - - if retry_remaining <= 0 { - tx.commit().await?; - return Ok(Phase2JobClaimOutcome::SkippedRetryUnavailable); - } if retry_at.is_some_and(|retry_at| retry_at > now) { tx.commit().await?; return Ok(Phase2JobClaimOutcome::SkippedRetryUnavailable); @@ -963,7 +957,6 @@ SET WHERE kind = ? AND job_key = ? AND (status != 'running' OR lease_until IS NULL OR lease_until <= ?) AND (retry_at IS NULL OR retry_at <= ?) - AND retry_remaining > 0 "#, ) .bind(worker_id.as_str()) @@ -1104,7 +1097,7 @@ WHERE thread_id = ? AND source_updated_at = ? /// - updates only the owned running singleton global row /// - sets `status='error'`, clears lease /// - writes failure reason and retry time - /// - decrements `retry_remaining` + /// - decrements `retry_remaining` without going below zero pub async fn mark_global_phase2_job_failed( &self, ownership_token: &str, @@ -1121,7 +1114,7 @@ SET finished_at = ?, lease_until = NULL, retry_at = ?, - retry_remaining = retry_remaining - 1, + retry_remaining = max(retry_remaining - 1, 0), last_error = ? WHERE kind = ? AND job_key = ? AND status = 'running' AND ownership_token = ? @@ -1162,7 +1155,7 @@ SET finished_at = ?, lease_until = NULL, retry_at = ?, - retry_remaining = retry_remaining - 1, + retry_remaining = max(retry_remaining - 1, 0), last_error = ? WHERE kind = ? AND job_key = ? AND status = 'running' @@ -2504,6 +2497,76 @@ WHERE kind = 'memory_stage1' let _ = tokio::fs::remove_dir_all(codex_home).await; } + #[tokio::test] + async fn phase2_global_lock_can_be_claimed_after_retry_budget_is_exhausted() { + let codex_home = unique_temp_dir(); + let runtime = StateRuntime::init(codex_home.clone(), "test-provider".to_string()) + .await + .expect("initialize runtime"); + + runtime + .enqueue_global_consolidation(/*input_watermark*/ 100) + .await + .expect("enqueue global consolidation"); + + let owner = ThreadId::from_string(&Uuid::new_v4().to_string()).expect("owner id"); + for attempt in 0..3 { + let claim = runtime + .try_claim_global_phase2_job(owner, /*lease_seconds*/ 3_600) + .await + .expect("claim phase2 before retry exhaustion"); + let ownership_token = match claim { + Phase2JobClaimOutcome::Claimed { + ownership_token, .. + } => ownership_token, + other => panic!( + "attempt {} should claim phase2 before retries are exhausted: {other:?}", + attempt + 1 + ), + }; + assert!( + runtime + .mark_global_phase2_job_failed( + ownership_token.as_str(), + "boom", + /*retry_delay_seconds*/ 0, + ) + .await + .expect("mark phase2 failed"), + "attempt {} should decrement retry budget", + attempt + 1 + ); + } + + let job_row = + sqlx::query("SELECT retry_remaining FROM jobs WHERE kind = ? AND job_key = ?") + .bind("memory_consolidate_global") + .bind("global") + .fetch_one(runtime.pool.as_ref()) + .await + .expect("load phase2 job row after retry exhaustion"); + assert_eq!( + job_row + .try_get::("retry_remaining") + .expect("retry_remaining"), + 0 + ); + + let claim_after_exhaustion = runtime + .try_claim_global_phase2_job(owner, /*lease_seconds*/ 3_600) + .await + .expect("claim phase2 after retry exhaustion"); + assert!( + matches!( + claim_after_exhaustion, + Phase2JobClaimOutcome::Claimed { .. } + ), + "phase2 claim should only lock; workspace diffing decides whether there is work" + ); + + let _ = tokio::fs::remove_dir_all(codex_home).await; + } + #[tokio::test] async fn list_stage1_outputs_for_global_returns_latest_outputs() { let codex_home = unique_temp_dir(); From 79b4f691a673eda0906f70debe0a0bd42d25096d Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 27 Apr 2026 15:14:16 +0200 Subject: [PATCH 05/13] Avoid rewriting Phase 2 selection on clean workspace (#19812) ## Why Phase 2 can now claim the global consolidation lock on startup even when the git-backed memory workspace is already clean. The clean-workspace path still finalized through the normal Phase 2 success path, which clears and re-marks `selected_for_phase2` rows. That made no-op startups perform avoidable writes to `stage1_outputs`, creating unnecessary DB I/O and contention when no memory files changed. ## What Changed - Added a preserving-selection Phase 2 finalizer in `codex-state` that only marks the global job row as succeeded. - Kept the existing `mark_global_phase2_job_succeeded` behavior for real consolidation runs, where the selected Phase 2 snapshot must be rewritten. - Switched the `succeeded_no_workspace_changes` branch in `core/src/memories/phase2.rs` to use the preserving-selection finalizer. - Added a regression test that installs a SQLite trigger on `stage1_outputs` and verifies the clean finalizer performs zero updates there. ## Testing - `cargo test -p codex-state` - `cargo test -p codex-core memories::tests::phase2` --- codex-rs/core/src/memories/phase2.rs | 20 ++- codex-rs/state/src/runtime/memories.rs | 190 ++++++++++++++++++++++--- 2 files changed, 186 insertions(+), 24 deletions(-) diff --git a/codex-rs/core/src/memories/phase2.rs b/codex-rs/core/src/memories/phase2.rs index 0b7ffd6130..09e259a728 100644 --- a/codex-rs/core/src/memories/phase2.rs +++ b/codex-rs/core/src/memories/phase2.rs @@ -122,12 +122,11 @@ pub(super) async fn run(session: &Arc, config: Arc) { if !workspace_diff.has_changes() { tracing::error!("Phase 2 no changes"); // We check only after sync of the file system. - job::succeed( + job::succeed_preserving_selection( session, db, &claim, new_watermark, - &raw_memories, "succeeded_no_workspace_changes", ) .await; @@ -291,6 +290,23 @@ mod job { .await .unwrap_or(false) } + + pub(super) async fn succeed_preserving_selection( + session: &Arc, + db: &StateRuntime, + claim: &Claim, + completion_watermark: i64, + reason: &'static str, + ) -> bool { + session.services.session_telemetry.counter( + metrics::MEMORY_PHASE_TWO_JOBS, + /*inc*/ 1, + &[("status", reason)], + ); + db.mark_global_phase2_job_succeeded_preserving_selection(&claim.token, completion_watermark) + .await + .unwrap_or(false) + } } mod agent { diff --git a/codex-rs/state/src/runtime/memories.rs b/codex-rs/state/src/runtime/memories.rs index 62175d15ae..1c07842bfa 100644 --- a/codex-rs/state/src/runtime/memories.rs +++ b/codex-rs/state/src/runtime/memories.rs @@ -1029,29 +1029,10 @@ WHERE kind = ? AND job_key = ? completed_watermark: i64, selected_outputs: &[Stage1Output], ) -> anyhow::Result { - let now = Utc::now().timestamp(); let mut tx = self.pool.begin().await?; - let rows_affected = sqlx::query( - r#" -UPDATE jobs -SET - status = 'done', - finished_at = ?, - lease_until = NULL, - last_error = NULL, - last_success_watermark = max(COALESCE(last_success_watermark, 0), ?) -WHERE kind = ? AND job_key = ? - AND status = 'running' AND ownership_token = ? - "#, - ) - .bind(now) - .bind(completed_watermark) - .bind(JOB_KIND_MEMORY_CONSOLIDATE_GLOBAL) - .bind(MEMORY_CONSOLIDATION_JOB_KEY) - .bind(ownership_token) - .execute(&mut *tx) - .await? - .rows_affected(); + let rows_affected = + mark_global_phase2_job_succeeded_row(&mut *tx, ownership_token, completed_watermark) + .await?; if rows_affected == 0 { tx.commit().await?; @@ -1091,6 +1072,27 @@ WHERE thread_id = ? AND source_updated_at = ? Ok(true) } + /// Marks the owned running global phase-2 job as succeeded without + /// rewriting the selected stage-1 snapshot. + /// + /// This is used when the materialized memory workspace is already clean: + /// the previous successful phase-2 selection is still authoritative, so + /// only the singleton job row needs to be finalized. + pub async fn mark_global_phase2_job_succeeded_preserving_selection( + &self, + ownership_token: &str, + completed_watermark: i64, + ) -> anyhow::Result { + let rows_affected = mark_global_phase2_job_succeeded_row( + self.pool.as_ref(), + ownership_token, + completed_watermark, + ) + .await?; + + Ok(rows_affected > 0) + } + /// Marks the owned running global phase-2 job as failed and schedules retry. /// /// Query behavior: @@ -1176,6 +1178,40 @@ WHERE kind = ? AND job_key = ? } } +async fn mark_global_phase2_job_succeeded_row<'e, E>( + executor: E, + ownership_token: &str, + completed_watermark: i64, +) -> anyhow::Result +where + E: Executor<'e, Database = Sqlite>, +{ + let now = Utc::now().timestamp(); + let rows_affected = sqlx::query( + r#" +UPDATE jobs +SET + status = 'done', + finished_at = ?, + lease_until = NULL, + last_error = NULL, + last_success_watermark = max(COALESCE(last_success_watermark, 0), ?) +WHERE kind = ? AND job_key = ? + AND status = 'running' AND ownership_token = ? + "#, + ) + .bind(now) + .bind(completed_watermark) + .bind(JOB_KIND_MEMORY_CONSOLIDATE_GLOBAL) + .bind(MEMORY_CONSOLIDATION_JOB_KEY) + .bind(ownership_token) + .execute(executor) + .await? + .rows_affected(); + + Ok(rows_affected) +} + async fn enqueue_global_consolidation_with_executor<'e, E>( executor: E, input_watermark: i64, @@ -2497,6 +2533,116 @@ WHERE kind = 'memory_stage1' let _ = tokio::fs::remove_dir_all(codex_home).await; } + #[tokio::test] + async fn phase2_success_preserving_selection_does_not_rewrite_stage1_outputs() { + let codex_home = unique_temp_dir(); + let runtime = StateRuntime::init(codex_home.clone(), "test-provider".to_string()) + .await + .expect("initialize runtime"); + + let thread_id = ThreadId::from_string(&Uuid::new_v4().to_string()).expect("thread id"); + runtime + .upsert_thread(&test_thread_metadata( + &codex_home, + thread_id, + codex_home.join("workspace"), + )) + .await + .expect("upsert thread"); + + let source_updated_at = Utc::now().timestamp(); + sqlx::query( + r#" +INSERT INTO stage1_outputs ( + thread_id, + source_updated_at, + raw_memory, + rollout_summary, + generated_at, + selected_for_phase2, + selected_for_phase2_source_updated_at +) VALUES (?, ?, 'raw', 'summary', ?, 1, ?) + "#, + ) + .bind(thread_id.to_string()) + .bind(source_updated_at) + .bind(source_updated_at) + .bind(source_updated_at) + .execute(runtime.pool.as_ref()) + .await + .expect("insert selected stage1 output"); + + sqlx::query("CREATE TABLE stage1_update_counter (updates INTEGER NOT NULL)") + .execute(runtime.pool.as_ref()) + .await + .expect("create update counter"); + sqlx::query("INSERT INTO stage1_update_counter (updates) VALUES (0)") + .execute(runtime.pool.as_ref()) + .await + .expect("initialize update counter"); + sqlx::query( + r#" +CREATE TRIGGER count_stage1_updates +AFTER UPDATE ON stage1_outputs +BEGIN + UPDATE stage1_update_counter SET updates = updates + 1; +END + "#, + ) + .execute(runtime.pool.as_ref()) + .await + .expect("create update trigger"); + + runtime + .enqueue_global_consolidation(source_updated_at) + .await + .expect("enqueue phase2"); + let phase2_claim = runtime + .try_claim_global_phase2_job(thread_id, /*lease_seconds*/ 3_600) + .await + .expect("claim phase2"); + let (ownership_token, input_watermark) = match phase2_claim { + Phase2JobClaimOutcome::Claimed { + ownership_token, + input_watermark, + } => (ownership_token, input_watermark), + other => panic!("unexpected phase2 claim outcome: {other:?}"), + }; + + assert!( + runtime + .mark_global_phase2_job_succeeded_preserving_selection( + ownership_token.as_str(), + input_watermark, + ) + .await + .expect("mark clean phase2 succeeded"), + "clean phase2 success should finalize the job" + ); + + let updates = sqlx::query_scalar::<_, i64>("SELECT updates FROM stage1_update_counter") + .fetch_one(runtime.pool.as_ref()) + .await + .expect("load stage1 update count"); + assert_eq!(updates, 0); + + let (selected_for_phase2, selected_for_phase2_source_updated_at) = + sqlx::query_as::<_, (i64, Option)>( + "SELECT selected_for_phase2, selected_for_phase2_source_updated_at FROM stage1_outputs WHERE thread_id = ?", + ) + .bind(thread_id.to_string()) + .fetch_one(runtime.pool.as_ref()) + .await + .expect("load selected snapshot"); + assert_eq!(selected_for_phase2, 1); + assert_eq!( + selected_for_phase2_source_updated_at, + Some(source_updated_at) + ); + + let _ = tokio::fs::remove_dir_all(codex_home).await; + } + #[tokio::test] async fn phase2_global_lock_can_be_claimed_after_retry_budget_is_exhausted() { let codex_home = unique_temp_dir(); From f431ec12c9f9e2671c1258fe2d259daf0ba25c95 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 27 Apr 2026 15:32:31 +0200 Subject: [PATCH 06/13] nit: one more fix (#19813) Fix this: https://github.com/openai/codex/pull/19812#discussion_r3147529230 --- codex-rs/core/src/memories/phase2.rs | 20 +--- codex-rs/core/src/memories/tests.rs | 12 ++- codex-rs/state/src/runtime/memories.rs | 131 ------------------------- 3 files changed, 12 insertions(+), 151 deletions(-) diff --git a/codex-rs/core/src/memories/phase2.rs b/codex-rs/core/src/memories/phase2.rs index 09e259a728..0b7ffd6130 100644 --- a/codex-rs/core/src/memories/phase2.rs +++ b/codex-rs/core/src/memories/phase2.rs @@ -122,11 +122,12 @@ pub(super) async fn run(session: &Arc, config: Arc) { if !workspace_diff.has_changes() { tracing::error!("Phase 2 no changes"); // We check only after sync of the file system. - job::succeed_preserving_selection( + job::succeed( session, db, &claim, new_watermark, + &raw_memories, "succeeded_no_workspace_changes", ) .await; @@ -290,23 +291,6 @@ mod job { .await .unwrap_or(false) } - - pub(super) async fn succeed_preserving_selection( - session: &Arc, - db: &StateRuntime, - claim: &Claim, - completion_watermark: i64, - reason: &'static str, - ) -> bool { - session.services.session_telemetry.counter( - metrics::MEMORY_PHASE_TWO_JOBS, - /*inc*/ 1, - &[("status", reason)], - ); - db.mark_global_phase2_job_succeeded_preserving_selection(&claim.token, completion_watermark) - .await - .unwrap_or(false) - } } mod agent { diff --git a/codex-rs/core/src/memories/tests.rs b/codex-rs/core/src/memories/tests.rs index 713f36b245..7c75a64484 100644 --- a/codex-rs/core/src/memories/tests.rs +++ b/codex-rs/core/src/memories/tests.rs @@ -1128,9 +1128,10 @@ mod phase2 { } #[tokio::test] - async fn dispatch_with_clean_workspace_preserves_selected_phase2_baseline() { + async fn dispatch_with_clean_workspace_rebuilds_selected_phase2_baseline() { let harness = DispatchHarness::new().await; - let thread_id = harness.seed_stage1_output(Utc::now().timestamp()).await; + let source_updated_at = (Utc::now() - ChronoDuration::days(1)).timestamp(); + let thread_id = harness.seed_stage1_output(source_updated_at).await; let root = memory_root(&harness.config.codex_home); let selected = harness .state_db @@ -1151,6 +1152,13 @@ mod phase2 { phase2::run(&harness.session, Arc::clone(&harness.config)).await; pretty_assertions::assert_eq!(harness.user_input_ops_count(), 0); + let pruned = harness + .state_db + .prune_stage1_outputs_for_retention(/*max_unused_days*/ 0, /*limit*/ 10) + .await + .expect("prune stage1 outputs after clean phase2"); + pretty_assertions::assert_eq!(pruned, 0); + let selected = harness .state_db .get_phase2_input_selection(/*n*/ 1, /*max_unused_days*/ 30) diff --git a/codex-rs/state/src/runtime/memories.rs b/codex-rs/state/src/runtime/memories.rs index 1c07842bfa..ccabca2253 100644 --- a/codex-rs/state/src/runtime/memories.rs +++ b/codex-rs/state/src/runtime/memories.rs @@ -1072,27 +1072,6 @@ WHERE thread_id = ? AND source_updated_at = ? Ok(true) } - /// Marks the owned running global phase-2 job as succeeded without - /// rewriting the selected stage-1 snapshot. - /// - /// This is used when the materialized memory workspace is already clean: - /// the previous successful phase-2 selection is still authoritative, so - /// only the singleton job row needs to be finalized. - pub async fn mark_global_phase2_job_succeeded_preserving_selection( - &self, - ownership_token: &str, - completed_watermark: i64, - ) -> anyhow::Result { - let rows_affected = mark_global_phase2_job_succeeded_row( - self.pool.as_ref(), - ownership_token, - completed_watermark, - ) - .await?; - - Ok(rows_affected > 0) - } - /// Marks the owned running global phase-2 job as failed and schedules retry. /// /// Query behavior: @@ -2533,116 +2512,6 @@ WHERE kind = 'memory_stage1' let _ = tokio::fs::remove_dir_all(codex_home).await; } - #[tokio::test] - async fn phase2_success_preserving_selection_does_not_rewrite_stage1_outputs() { - let codex_home = unique_temp_dir(); - let runtime = StateRuntime::init(codex_home.clone(), "test-provider".to_string()) - .await - .expect("initialize runtime"); - - let thread_id = ThreadId::from_string(&Uuid::new_v4().to_string()).expect("thread id"); - runtime - .upsert_thread(&test_thread_metadata( - &codex_home, - thread_id, - codex_home.join("workspace"), - )) - .await - .expect("upsert thread"); - - let source_updated_at = Utc::now().timestamp(); - sqlx::query( - r#" -INSERT INTO stage1_outputs ( - thread_id, - source_updated_at, - raw_memory, - rollout_summary, - generated_at, - selected_for_phase2, - selected_for_phase2_source_updated_at -) VALUES (?, ?, 'raw', 'summary', ?, 1, ?) - "#, - ) - .bind(thread_id.to_string()) - .bind(source_updated_at) - .bind(source_updated_at) - .bind(source_updated_at) - .execute(runtime.pool.as_ref()) - .await - .expect("insert selected stage1 output"); - - sqlx::query("CREATE TABLE stage1_update_counter (updates INTEGER NOT NULL)") - .execute(runtime.pool.as_ref()) - .await - .expect("create update counter"); - sqlx::query("INSERT INTO stage1_update_counter (updates) VALUES (0)") - .execute(runtime.pool.as_ref()) - .await - .expect("initialize update counter"); - sqlx::query( - r#" -CREATE TRIGGER count_stage1_updates -AFTER UPDATE ON stage1_outputs -BEGIN - UPDATE stage1_update_counter SET updates = updates + 1; -END - "#, - ) - .execute(runtime.pool.as_ref()) - .await - .expect("create update trigger"); - - runtime - .enqueue_global_consolidation(source_updated_at) - .await - .expect("enqueue phase2"); - let phase2_claim = runtime - .try_claim_global_phase2_job(thread_id, /*lease_seconds*/ 3_600) - .await - .expect("claim phase2"); - let (ownership_token, input_watermark) = match phase2_claim { - Phase2JobClaimOutcome::Claimed { - ownership_token, - input_watermark, - } => (ownership_token, input_watermark), - other => panic!("unexpected phase2 claim outcome: {other:?}"), - }; - - assert!( - runtime - .mark_global_phase2_job_succeeded_preserving_selection( - ownership_token.as_str(), - input_watermark, - ) - .await - .expect("mark clean phase2 succeeded"), - "clean phase2 success should finalize the job" - ); - - let updates = sqlx::query_scalar::<_, i64>("SELECT updates FROM stage1_update_counter") - .fetch_one(runtime.pool.as_ref()) - .await - .expect("load stage1 update count"); - assert_eq!(updates, 0); - - let (selected_for_phase2, selected_for_phase2_source_updated_at) = - sqlx::query_as::<_, (i64, Option)>( - "SELECT selected_for_phase2, selected_for_phase2_source_updated_at FROM stage1_outputs WHERE thread_id = ?", - ) - .bind(thread_id.to_string()) - .fetch_one(runtime.pool.as_ref()) - .await - .expect("load selected snapshot"); - assert_eq!(selected_for_phase2, 1); - assert_eq!( - selected_for_phase2_source_updated_at, - Some(source_updated_at) - ); - - let _ = tokio::fs::remove_dir_all(codex_home).await; - } - #[tokio::test] async fn phase2_global_lock_can_be_claimed_after_retry_budget_is_exhausted() { let codex_home = unique_temp_dir(); From bb83eec825b74aaf06f74650d2c004b0629dd19a Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 27 Apr 2026 16:01:05 +0200 Subject: [PATCH 07/13] chore: split memories part 1 (#19818) Extract memories into 2 different crates --- codex-rs/Cargo.lock | 36 ++++++++++ codex-rs/Cargo.toml | 4 ++ codex-rs/core/Cargo.toml | 2 + codex-rs/core/src/config/mod.rs | 2 +- codex-rs/core/src/lib.rs | 3 +- codex-rs/core/src/memories/mod.rs | 54 +------------- codex-rs/core/src/memories/phase1.rs | 2 +- codex-rs/core/src/memories/phase2.rs | 18 ++--- codex-rs/core/src/memories/tests.rs | 34 ++++----- .../{memories/usage.rs => memory_usage.rs} | 70 ++----------------- codex-rs/core/src/session/handlers.rs | 4 +- codex-rs/core/src/session/mod.rs | 2 +- codex-rs/core/src/stream_events_utils.rs | 4 +- codex-rs/core/src/tools/registry.rs | 2 +- codex-rs/{core/src => }/memories/README.md | 26 +++++-- codex-rs/memories/read/BUILD.bazel | 9 +++ codex-rs/memories/read/Cargo.toml | 25 +++++++ .../read/src}/citations.rs | 0 .../read/src}/citations_tests.rs | 0 codex-rs/memories/read/src/lib.rs | 19 +++++ codex-rs/memories/read/src/prompts.rs | 56 +++++++++++++++ codex-rs/memories/read/src/prompts_tests.rs | 35 ++++++++++ codex-rs/memories/read/src/usage.rs | 57 +++++++++++++++ .../read}/templates/memories/read_path.md | 0 codex-rs/memories/write/BUILD.bazel | 9 +++ codex-rs/memories/write/Cargo.toml | 31 ++++++++ .../write/src}/control.rs | 0 .../write/src}/extensions.rs | 6 +- .../write/src}/extensions_tests.rs | 0 codex-rs/memories/write/src/lib.rs | 63 +++++++++++++++++ .../write/src}/prompts.rs | 57 +++------------ .../write/src}/prompts_tests.rs | 37 +--------- .../write/src}/storage.rs | 14 ++-- .../write/src}/storage_tests.rs | 9 --- .../write/src}/workspace.rs | 13 ++-- .../write/src}/workspace_tests.rs | 0 .../templates/memories/consolidation.md | 0 .../templates/memories/stage_one_input.md | 0 .../templates/memories/stage_one_system.md | 0 39 files changed, 436 insertions(+), 267 deletions(-) rename codex-rs/core/src/{memories/usage.rs => memory_usage.rs} (60%) rename codex-rs/{core/src => }/memories/README.md (88%) create mode 100644 codex-rs/memories/read/BUILD.bazel create mode 100644 codex-rs/memories/read/Cargo.toml rename codex-rs/{core/src/memories => memories/read/src}/citations.rs (100%) rename codex-rs/{core/src/memories => memories/read/src}/citations_tests.rs (100%) create mode 100644 codex-rs/memories/read/src/lib.rs create mode 100644 codex-rs/memories/read/src/prompts.rs create mode 100644 codex-rs/memories/read/src/prompts_tests.rs create mode 100644 codex-rs/memories/read/src/usage.rs rename codex-rs/{core => memories/read}/templates/memories/read_path.md (100%) create mode 100644 codex-rs/memories/write/BUILD.bazel create mode 100644 codex-rs/memories/write/Cargo.toml rename codex-rs/{core/src/memories => memories/write/src}/control.rs (100%) rename codex-rs/{core/src/memories => memories/write/src}/extensions.rs (94%) rename codex-rs/{core/src/memories => memories/write/src}/extensions_tests.rs (100%) create mode 100644 codex-rs/memories/write/src/lib.rs rename codex-rs/{core/src/memories => memories/write/src}/prompts.rs (74%) rename codex-rs/{core/src/memories => memories/write/src}/prompts_tests.rs (67%) rename codex-rs/{core/src/memories => memories/write/src}/storage.rs (95%) rename codex-rs/{core/src/memories => memories/write/src}/storage_tests.rs (88%) rename codex-rs/{core/src/memories => memories/write/src}/workspace.rs (90%) rename codex-rs/{core/src/memories => memories/write/src}/workspace_tests.rs (100%) rename codex-rs/{core => memories/write}/templates/memories/consolidation.md (100%) rename codex-rs/{core => memories/write}/templates/memories/stage_one_input.md (100%) rename codex-rs/{core => memories/write}/templates/memories/stage_one_system.md (100%) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index fbab962cbc..464b7d72a2 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2370,6 +2370,8 @@ dependencies = [ "codex-hooks", "codex-login", "codex-mcp", + "codex-memories-read", + "codex-memories-write", "codex-model-provider", "codex-model-provider-info", "codex-models-manager", @@ -2921,6 +2923,40 @@ dependencies = [ "wiremock", ] +[[package]] +name = "codex-memories-read" +version = "0.0.0" +dependencies = [ + "codex-protocol", + "codex-shell-command", + "codex-utils-absolute-path", + "codex-utils-output-truncation", + "codex-utils-template", + "pretty_assertions", + "tempfile", + "tokio", +] + +[[package]] +name = "codex-memories-write" +version = "0.0.0" +dependencies = [ + "anyhow", + "chrono", + "codex-git-utils", + "codex-models-manager", + "codex-protocol", + "codex-state", + "codex-utils-absolute-path", + "codex-utils-output-truncation", + "codex-utils-template", + "pretty_assertions", + "tempfile", + "tokio", + "tracing", + "uuid", +] + [[package]] name = "codex-model-provider" version = "0.0.0" diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 648c184ec8..85c947aea2 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -46,6 +46,8 @@ members = [ "login", "codex-mcp", "mcp-server", + "memories/read", + "memories/write", "model-provider-info", "models-manager", "network-proxy", @@ -153,6 +155,8 @@ codex-keyring-store = { path = "keyring-store" } codex-linux-sandbox = { path = "linux-sandbox" } codex-lmstudio = { path = "lmstudio" } codex-login = { path = "login" } +codex-memories-read = { path = "memories/read" } +codex-memories-write = { path = "memories/write" } codex-mcp = { path = "codex-mcp" } codex-mcp-server = { path = "mcp-server" } codex-model-provider-info = { path = "model-provider-info" } diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index b8d3b146f6..c95b57b718 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -39,6 +39,8 @@ codex-exec-server = { workspace = true } codex-features = { workspace = true } codex-feedback = { workspace = true } codex-login = { workspace = true } +codex-memories-read = { workspace = true } +codex-memories-write = { workspace = true } codex-mcp = { workspace = true } codex-model-provider-info = { workspace = true } codex-models-manager = { workspace = true } diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index ba21b2ea10..47f2e4dd9f 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1,7 +1,6 @@ use crate::agents_md::AgentsMdManager; use crate::config::edit::ConfigEdit; use crate::config::edit::ConfigEditsBuilder; -use crate::memories::memory_root; use crate::path_utils::normalize_for_native_workdir; use crate::unified_exec::DEFAULT_MAX_BACKGROUND_TERMINAL_TIMEOUT_MS; use crate::unified_exec::MIN_EMPTY_YIELD_TIME_MS; @@ -63,6 +62,7 @@ use codex_features::MultiAgentV2ConfigToml; use codex_git_utils::resolve_root_git_project_for_trust; use codex_login::AuthManagerConfig; use codex_mcp::McpConfig; +use codex_memories_write::memory_root; use codex_model_provider_info::LEGACY_OLLAMA_CHAT_PROVIDER_ID; use codex_model_provider_info::ModelProviderInfo; use codex_model_provider_info::OLLAMA_CHAT_PROVIDER_REMOVED_ERROR; diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index c6f879209d..c5e0da9b8f 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -57,7 +57,7 @@ pub use codex_mcp::SandboxState; mod mcp_openai_file; mod mcp_tool_call; mod memories; -pub use memories::clear_memory_roots_contents; +pub use codex_memories_write::clear_memory_roots_contents; pub(crate) mod mention_syntax; pub(crate) mod message_history; pub(crate) mod utils; @@ -200,4 +200,5 @@ pub mod compact; pub(crate) mod memory_trace; pub use memory_trace::BuiltMemory; pub use memory_trace::build_memories_from_trace_files; +mod memory_usage; pub mod otel_init; diff --git a/codex-rs/core/src/memories/mod.rs b/codex-rs/core/src/memories/mod.rs index 023ea9913a..efa70f4bbb 100644 --- a/codex-rs/core/src/memories/mod.rs +++ b/codex-rs/core/src/memories/mod.rs @@ -1,37 +1,23 @@ -//! Memory subsystem for startup extraction and consolidation. +//! Memory startup extraction and consolidation orchestration. //! //! The startup memory pipeline is split into two phases: //! - Phase 1: select rollouts, extract stage-1 raw memories, persist stage-1 outputs, and enqueue consolidation. //! - Phase 2: claim a global consolidation lock, materialize consolidation inputs, and dispatch one consolidation agent. -pub(crate) mod citations; -mod control; -mod extensions; mod phase1; mod phase2; -pub(crate) mod prompts; mod start; -mod storage; #[cfg(test)] mod tests; -pub(crate) mod usage; -mod workspace; use codex_protocol::openai_models::ReasoningEffort; -pub use control::clear_memory_roots_contents; /// Starts the memory startup pipeline for eligible root sessions. /// This is the single entrypoint that `codex` uses to trigger memory startup. /// /// This is the entry point to read and understand this module. pub(crate) use start::start_memories_startup_task; -mod artifacts { - pub(super) const EXTENSIONS_SUBDIR: &str = "extensions"; - pub(super) const ROLLOUT_SUMMARIES_SUBDIR: &str = "rollout_summaries"; - pub(super) const RAW_MEMORIES_FILENAME: &str = "raw_memories.md"; -} - /// Phase 1 (startup extraction). mod phase_one { /// Default model used for phase 1. @@ -39,21 +25,9 @@ mod phase_one { /// Default reasoning effort used for phase 1. pub(super) const REASONING_EFFORT: super::ReasoningEffort = super::ReasoningEffort::Low; /// Prompt used for phase 1. - pub(super) const PROMPT: &str = include_str!("../../templates/memories/stage_one_system.md"); + pub(super) const PROMPT: &str = codex_memories_write::STAGE_ONE_PROMPT; /// Concurrency cap for startup memory extraction and consolidation scheduling. pub(super) const CONCURRENCY_LIMIT: usize = 8; - /// Fallback stage-1 rollout truncation limit (tokens) when model metadata - /// does not include a valid context window. - pub(super) const DEFAULT_STAGE_ONE_ROLLOUT_TOKEN_LIMIT: usize = 150_000; - /// Maximum number of tokens from `memory_summary.md` injected into memory - /// tool developer instructions. - pub(super) const MEMORY_TOOL_DEVELOPER_INSTRUCTIONS_SUMMARY_TOKEN_LIMIT: usize = 5_000; - /// Portion of the model effective input window reserved for the stage-1 - /// rollout input. - /// - /// Keeping this below 100% leaves room for system instructions, prompt - /// framing, and model output. - pub(super) const CONTEXT_WINDOW_PERCENT: i64 = 70; /// Lease duration (seconds) for phase-1 job ownership. pub(super) const JOB_LEASE_SECONDS: i64 = 3_600; /// Backoff delay (seconds) before retrying a failed stage-1 extraction job. @@ -97,27 +71,3 @@ mod metrics { /// Histogram for aggregate token usage across one phase-2 consolidation run. pub(super) const MEMORY_PHASE_TWO_TOKEN_USAGE: &str = "codex.memory.phase2.token_usage"; } - -use codex_utils_absolute_path::AbsolutePathBuf; -use std::path::Path; -use std::path::PathBuf; - -pub fn memory_root(codex_home: &AbsolutePathBuf) -> AbsolutePathBuf { - codex_home.join("memories") -} - -fn rollout_summaries_dir(root: &Path) -> PathBuf { - root.join(artifacts::ROLLOUT_SUMMARIES_SUBDIR) -} - -fn memory_extensions_root(root: &Path) -> PathBuf { - root.join(artifacts::EXTENSIONS_SUBDIR) -} - -fn raw_memories_file(root: &Path) -> PathBuf { - root.join(artifacts::RAW_MEMORIES_FILENAME) -} - -async fn ensure_layout(root: &Path) -> std::io::Result<()> { - tokio::fs::create_dir_all(rollout_summaries_dir(root)).await -} diff --git a/codex-rs/core/src/memories/phase1.rs b/codex-rs/core/src/memories/phase1.rs index 8fed735c49..40f65c6db0 100644 --- a/codex-rs/core/src/memories/phase1.rs +++ b/codex-rs/core/src/memories/phase1.rs @@ -5,13 +5,13 @@ use crate::context::is_memory_excluded_contextual_user_fragment; use crate::memories::metrics; use crate::memories::phase_one; use crate::memories::phase_one::PRUNE_BATCH_SIZE; -use crate::memories::prompts::build_stage_one_input_message; use crate::rollout::INTERACTIVE_SESSION_SOURCES; use crate::rollout::policy::should_persist_response_item_for_memories; use crate::session::session::Session; use crate::session::turn_context::TurnContext; use codex_api::ResponseEvent; use codex_config::types::MemoriesConfig; +use codex_memories_write::build_stage_one_input_message; use codex_otel::SessionTelemetry; use codex_protocol::config_types::ReasoningSummary as ReasoningSummaryConfig; use codex_protocol::config_types::ServiceTier; diff --git a/codex-rs/core/src/memories/phase2.rs b/codex-rs/core/src/memories/phase2.rs index 0b7ffd6130..d156d1dee5 100644 --- a/codex-rs/core/src/memories/phase2.rs +++ b/codex-rs/core/src/memories/phase2.rs @@ -1,21 +1,21 @@ use crate::agent::AgentStatus; use crate::agent::status::is_final as is_final_agent_status; use crate::config::Config; -use crate::memories::extensions::prune_old_extension_resources; -use crate::memories::memory_root; use crate::memories::metrics; use crate::memories::phase_two; -use crate::memories::prompts::build_consolidation_prompt; -use crate::memories::storage::rebuild_raw_memories_file_from_memories; -use crate::memories::storage::sync_rollout_summaries_from_memories; -use crate::memories::workspace::memory_workspace_diff; -use crate::memories::workspace::prepare_memory_workspace; -use crate::memories::workspace::reset_memory_workspace_baseline; -use crate::memories::workspace::write_workspace_diff; use crate::session::emit_subagent_session_started; use crate::session::session::Session; use codex_config::Constrained; use codex_features::Feature; +use codex_memories_write::build_consolidation_prompt; +use codex_memories_write::memory_root; +use codex_memories_write::prune_old_extension_resources; +use codex_memories_write::rebuild_raw_memories_file_from_memories; +use codex_memories_write::sync_rollout_summaries_from_memories; +use codex_memories_write::workspace::memory_workspace_diff; +use codex_memories_write::workspace::prepare_memory_workspace; +use codex_memories_write::workspace::reset_memory_workspace_baseline; +use codex_memories_write::workspace::write_workspace_diff; use codex_protocol::ThreadId; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::SandboxPolicy; diff --git a/codex-rs/core/src/memories/tests.rs b/codex-rs/core/src/memories/tests.rs index 7c75a64484..c00c77b8e4 100644 --- a/codex-rs/core/src/memories/tests.rs +++ b/codex-rs/core/src/memories/tests.rs @@ -1,13 +1,13 @@ -use super::control::clear_memory_root_contents; -use super::storage::rebuild_raw_memories_file_from_memories; -use super::storage::sync_rollout_summaries_from_memories; -use crate::memories::ensure_layout; -use crate::memories::memory_root; -use crate::memories::raw_memories_file; -use crate::memories::rollout_summaries_dir; use chrono::TimeZone; use chrono::Utc; use codex_config::types::DEFAULT_MEMORIES_MAX_RAW_MEMORIES_FOR_CONSOLIDATION; +use codex_memories_write::clear_memory_roots_contents; +use codex_memories_write::ensure_layout; +use codex_memories_write::memory_root; +use codex_memories_write::raw_memories_file; +use codex_memories_write::rebuild_raw_memories_file_from_memories; +use codex_memories_write::rollout_summaries_dir; +use codex_memories_write::sync_rollout_summaries_from_memories; use codex_protocol::ThreadId; use codex_state::Stage1Output; use codex_utils_absolute_path::AbsolutePathBuf; @@ -68,7 +68,7 @@ fn stage_one_output_schema_requires_rollout_slug_and_keeps_it_nullable() { #[tokio::test] async fn clear_memory_root_contents_preserves_root_directory() { let dir = tempdir().expect("tempdir"); - let root = dir.path().join("memory"); + let root = dir.path().join("memories"); let nested_dir = root.join("rollout_summaries"); tokio::fs::create_dir_all(&nested_dir) .await @@ -80,7 +80,7 @@ async fn clear_memory_root_contents_preserves_root_directory() { .await .expect("write rollout summary"); - clear_memory_root_contents(&root) + clear_memory_roots_contents(dir.path()) .await .expect("clear memory root contents"); @@ -116,10 +116,10 @@ async fn clear_memory_root_contents_rejects_symlinked_root() { .await .expect("write target file"); - let root = dir.path().join("memory"); + let root = dir.path().join("memories"); std::os::unix::fs::symlink(&target, &root).expect("create memory root symlink"); - let err = clear_memory_root_contents(&root) + let err = clear_memory_roots_contents(dir.path()) .await .expect_err("symlinked memory root should be rejected"); assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput); @@ -509,13 +509,7 @@ mod phase2 { use crate::agent::AgentControl; use crate::config::Config; use crate::config::test_config; - use crate::memories::memory_root; use crate::memories::phase2; - use crate::memories::raw_memories_file; - use crate::memories::rollout_summaries_dir; - use crate::memories::storage::rebuild_raw_memories_file_from_memories; - use crate::memories::storage::sync_rollout_summaries_from_memories; - use crate::memories::workspace::prepare_memory_workspace; use crate::session::session::Session; use crate::session::tests::make_session_and_context; use chrono::Duration as ChronoDuration; @@ -524,6 +518,12 @@ mod phase2 { use codex_config::types::McpServerConfig; use codex_features::Feature; use codex_login::CodexAuth; + use codex_memories_write::memory_root; + use codex_memories_write::raw_memories_file; + use codex_memories_write::rebuild_raw_memories_file_from_memories; + use codex_memories_write::rollout_summaries_dir; + use codex_memories_write::sync_rollout_summaries_from_memories; + use codex_memories_write::workspace::prepare_memory_workspace; use codex_protocol::AgentPath; use codex_protocol::ThreadId; use codex_protocol::models::PermissionProfile; diff --git a/codex-rs/core/src/memories/usage.rs b/codex-rs/core/src/memory_usage.rs similarity index 60% rename from codex-rs/core/src/memories/usage.rs rename to codex-rs/core/src/memory_usage.rs index 480ef5dd9b..02f74ea593 100644 --- a/codex-rs/core/src/memories/usage.rs +++ b/codex-rs/core/src/memory_usage.rs @@ -1,38 +1,17 @@ use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::handlers::unified_exec::ExecCommandArgs; +use codex_memories_read::usage::MEMORIES_USAGE_METRIC; +use codex_memories_read::usage::memories_usage_kinds_from_command; use codex_protocol::models::ShellCommandToolCallParams; use codex_protocol::models::ShellToolCallParams; -use codex_protocol::parse_command::ParsedCommand; -use codex_shell_command::is_safe_command::is_known_safe_command; -use codex_shell_command::parse_command::parse_command; use std::path::PathBuf; -const MEMORIES_USAGE_METRIC: &str = "codex.memories.usage"; - -#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)] -enum MemoriesUsageKind { - MemoryMd, - MemorySummary, - RawMemories, - RolloutSummaries, - Skills, -} - -impl MemoriesUsageKind { - fn as_tag(self) -> &'static str { - match self { - Self::MemoryMd => "memory_md", - Self::MemorySummary => "memory_summary", - Self::RawMemories => "raw_memories", - Self::RolloutSummaries => "rollout_summaries", - Self::Skills => "skills", - } - } -} - pub(crate) async fn emit_metric_for_tool_read(invocation: &ToolInvocation, success: bool) { - let kinds = memories_usage_kinds_from_invocation(invocation).await; + let Some((command, _)) = shell_command_for_invocation(invocation) else { + return; + }; + let kinds = memories_usage_kinds_from_command(&command); if kinds.is_empty() { return; } @@ -52,27 +31,6 @@ pub(crate) async fn emit_metric_for_tool_read(invocation: &ToolInvocation, succe } } -async fn memories_usage_kinds_from_invocation( - invocation: &ToolInvocation, -) -> Vec { - let Some((command, _)) = shell_command_for_invocation(invocation) else { - return Vec::new(); - }; - if !is_known_safe_command(&command) { - return Vec::new(); - } - - let parsed_commands = parse_command(&command); - parsed_commands - .into_iter() - .filter_map(|command| match command { - ParsedCommand::Read { path, .. } => get_memory_kind(path.display().to_string()), - ParsedCommand::Search { path, .. } => path.and_then(get_memory_kind), - ParsedCommand::ListFiles { .. } | ParsedCommand::Unknown { .. } => None, - }) - .collect() -} - fn shell_command_for_invocation(invocation: &ToolInvocation) -> Option<(Vec, PathBuf)> { let ToolPayload::Function { arguments } = &invocation.payload else { return None; @@ -129,19 +87,3 @@ fn shell_command_for_invocation(invocation: &ToolInvocation) -> Option<(Vec None, } } - -fn get_memory_kind(path: String) -> Option { - if path.contains("memories/MEMORY.md") { - Some(MemoriesUsageKind::MemoryMd) - } else if path.contains("memories/memory_summary.md") { - Some(MemoriesUsageKind::MemorySummary) - } else if path.contains("memories/raw_memories.md") { - Some(MemoriesUsageKind::RawMemories) - } else if path.contains("memories/rollout_summaries/") { - Some(MemoriesUsageKind::RolloutSummaries) - } else if path.contains("memories/skills/") { - Some(MemoriesUsageKind::Skills) - } else { - None - } -} diff --git a/codex-rs/core/src/session/handlers.rs b/codex-rs/core/src/session/handlers.rs index 86ce79c90f..5c11b2d9da 100644 --- a/codex-rs/core/src/session/handlers.rs +++ b/codex-rs/core/src/session/handlers.rs @@ -681,7 +681,7 @@ pub async fn drop_memories(sess: &Arc, config: &Arc, sub_id: St errors.push("state db unavailable; memory rows were not cleared".to_string()); } - if let Err(err) = crate::memories::clear_memory_roots_contents(&config.codex_home).await { + if let Err(err) = codex_memories_write::clear_memory_roots_contents(&config.codex_home).await { errors.push(format!( "failed clearing memory directories under {}: {err}", config.codex_home.display() @@ -689,7 +689,7 @@ pub async fn drop_memories(sess: &Arc, config: &Arc, sub_id: St } if errors.is_empty() { - let memory_root = crate::memories::memory_root(&config.codex_home); + let memory_root = codex_memories_write::memory_root(&config.codex_home); sess.send_event_raw(Event { id: sub_id, msg: EventMsg::Warning(WarningEvent { diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 598d9d7dc4..1ebe2b4fc7 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -3323,7 +3323,7 @@ fn errors_to_info(errors: &[SkillError]) -> Vec { .collect() } -use crate::memories::prompts::build_memory_tool_developer_instructions; +use codex_memories_read::build_memory_tool_developer_instructions; #[cfg(test)] pub(crate) mod tests; diff --git a/codex-rs/core/src/stream_events_utils.rs b/codex-rs/core/src/stream_events_utils.rs index 55c85747e5..5a31d18020 100644 --- a/codex-rs/core/src/stream_events_utils.rs +++ b/codex-rs/core/src/stream_events_utils.rs @@ -11,13 +11,13 @@ use tokio_util::sync::CancellationToken; use crate::context::ContextualUserFragment; use crate::context::ImageGenerationInstructions; use crate::function_tool::FunctionCallError; -use crate::memories::citations::parse_memory_citation; -use crate::memories::citations::thread_ids_from_memory_citation; use crate::parse_turn_item; use crate::session::session::Session; use crate::session::turn_context::TurnContext; use crate::tools::parallel::ToolCallRuntime; use crate::tools::router::ToolRouter; +use codex_memories_read::citations::parse_memory_citation; +use codex_memories_read::citations::thread_ids_from_memory_citation; use codex_protocol::error::CodexErr; use codex_protocol::error::Result; use codex_protocol::models::FunctionCallOutputBody; diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index acc1eacbf3..29abab69f3 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -8,7 +8,7 @@ use crate::goals::GoalRuntimeEvent; use crate::hook_runtime::record_additional_contexts; use crate::hook_runtime::run_post_tool_use_hooks; use crate::hook_runtime::run_pre_tool_use_hooks; -use crate::memories::usage::emit_metric_for_tool_read; +use crate::memory_usage::emit_metric_for_tool_read; use crate::sandbox_tags::permission_profile_policy_tag; use crate::sandbox_tags::permission_profile_sandbox_tag; use crate::session::turn_context::TurnContext; diff --git a/codex-rs/core/src/memories/README.md b/codex-rs/memories/README.md similarity index 88% rename from codex-rs/core/src/memories/README.md rename to codex-rs/memories/README.md index 8a885fd864..dfea3969f7 100644 --- a/codex-rs/core/src/memories/README.md +++ b/codex-rs/memories/README.md @@ -1,16 +1,28 @@ -# Memories Pipeline (Core) +# Memories -This module runs a startup memory pipeline for eligible sessions. +This directory owns reusable memory crates and the memory pipeline documentation. + +Runtime orchestration for Phase 1 and Phase 2 still lives in `codex-core` under +`codex-rs/core/src/memories/`. + +## Crates + +- `codex-rs/memories/read` (`codex-memories-read`) owns the read path: + memory developer-instruction injection, memory citation parsing, and + read-usage telemetry classification. +- `codex-rs/memories/write` (`codex-memories-write`) owns the write path: + Phase 1 and Phase 2 prompt rendering, filesystem artifact helpers, + workspace diff helpers, and extension resource pruning. ## Prompt Templates -Memory prompt templates live under `codex-rs/core/templates/memories/`. +Memory prompt templates live with the crate that uses them: - The undated template files are the canonical latest versions used at runtime: - - `stage_one_system.md` - - `stage_one_input.md` - - `consolidation.md` - - `read_path.md` + - `read/templates/memories/read_path.md` + - `write/templates/memories/stage_one_system.md` + - `write/templates/memories/stage_one_input.md` + - `write/templates/memories/consolidation.md` - In `codex`, edit those undated template files in place. - The dated snapshot-copy workflow is used in the separate `openai/project/agent_memory/write` harness repo, not here. diff --git a/codex-rs/memories/read/BUILD.bazel b/codex-rs/memories/read/BUILD.bazel new file mode 100644 index 0000000000..54cf2e3b00 --- /dev/null +++ b/codex-rs/memories/read/BUILD.bazel @@ -0,0 +1,9 @@ +load("//:defs.bzl", "codex_rust_crate") + +codex_rust_crate( + name = "read", + crate_name = "codex_memories_read", + compile_data = glob([ + "templates/**", + ]), +) diff --git a/codex-rs/memories/read/Cargo.toml b/codex-rs/memories/read/Cargo.toml new file mode 100644 index 0000000000..57aff37d6d --- /dev/null +++ b/codex-rs/memories/read/Cargo.toml @@ -0,0 +1,25 @@ +[package] +edition.workspace = true +license.workspace = true +name = "codex-memories-read" +version.workspace = true + +[lib] +name = "codex_memories_read" +path = "src/lib.rs" + +[lints] +workspace = true + +[dependencies] +codex-protocol = { workspace = true } +codex-shell-command = { workspace = true } +codex-utils-absolute-path = { workspace = true } +codex-utils-output-truncation = { workspace = true } +codex-utils-template = { workspace = true } +tokio = { workspace = true, features = ["fs"] } + +[dev-dependencies] +pretty_assertions = { workspace = true } +tempfile = { workspace = true } +tokio = { workspace = true, features = ["fs", "macros"] } diff --git a/codex-rs/core/src/memories/citations.rs b/codex-rs/memories/read/src/citations.rs similarity index 100% rename from codex-rs/core/src/memories/citations.rs rename to codex-rs/memories/read/src/citations.rs diff --git a/codex-rs/core/src/memories/citations_tests.rs b/codex-rs/memories/read/src/citations_tests.rs similarity index 100% rename from codex-rs/core/src/memories/citations_tests.rs rename to codex-rs/memories/read/src/citations_tests.rs diff --git a/codex-rs/memories/read/src/lib.rs b/codex-rs/memories/read/src/lib.rs new file mode 100644 index 0000000000..e085684081 --- /dev/null +++ b/codex-rs/memories/read/src/lib.rs @@ -0,0 +1,19 @@ +//! Read-path helpers for Codex memories. +//! +//! This crate owns memory injection, memory citation parsing, and telemetry +//! classification for read access to the memory folder. It intentionally does +//! not depend on the memory write pipeline. + +pub mod citations; +mod prompts; +pub mod usage; + +use codex_utils_absolute_path::AbsolutePathBuf; + +pub use prompts::build_memory_tool_developer_instructions; + +const MEMORY_TOOL_DEVELOPER_INSTRUCTIONS_SUMMARY_TOKEN_LIMIT: usize = 5_000; + +pub fn memory_root(codex_home: &AbsolutePathBuf) -> AbsolutePathBuf { + codex_home.join("memories") +} diff --git a/codex-rs/memories/read/src/prompts.rs b/codex-rs/memories/read/src/prompts.rs new file mode 100644 index 0000000000..5bba68aa69 --- /dev/null +++ b/codex-rs/memories/read/src/prompts.rs @@ -0,0 +1,56 @@ +use crate::MEMORY_TOOL_DEVELOPER_INSTRUCTIONS_SUMMARY_TOKEN_LIMIT; +use crate::memory_root; +use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_output_truncation::TruncationPolicy; +use codex_utils_output_truncation::truncate_text; +use codex_utils_template::Template; +use std::sync::LazyLock; +use tokio::fs; + +static MEMORY_TOOL_DEVELOPER_INSTRUCTIONS_TEMPLATE: LazyLock