Skip to content

feat(evolve-lite): unify sharing into scoped repos list (#217)#218

Open
illeatmyhat wants to merge 7 commits intomainfrom
feature/issue-217-memory-sharing-repo
Open

feat(evolve-lite): unify sharing into scoped repos list (#217)#218
illeatmyhat wants to merge 7 commits intomainfrom
feature/issue-217-memory-sharing-repo

Conversation

@illeatmyhat
Copy link
Copy Markdown
Collaborator

@illeatmyhat illeatmyhat commented Apr 23, 2026

Summary

  • Collapse public_repo + subscriptions into a single repos: list where each entry has scope: read (subscribe only) or scope: write (publish target, also synced) — closes Memory Sharing Repo #217.
  • Update publish / subscribe / sync / unsubscribe / recall skills for both Claude and Codex plugins to operate on the unified model; write-scope repos double as publish targets and sync sources so co-writers see each other's publishes.
  • Add helpers in lib/config.py (normalize_repos, read_repos, write_repos, set_repos, get_repo, is_valid_repo_name) and broaden tests across tests/platform_integrations/.

Test plan

  • pytest tests/platform_integrations/ passes locally
  • /evolve-lite:subscribe with scope=read clones into .evolve/entities/subscribed/<name>/ and recall picks it up
  • /evolve-lite:subscribe with scope=write then /evolve-lite:publish moves guideline into the clone, commits, and pushes
  • /evolve-lite:sync pulls read-scope repos (fetch + reset --hard) and rebases write-scope repos without losing unpushed publishes
  • /evolve-lite:unsubscribe warns before removing a write-scope repo

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Unified repos: configure multiple repos with explicit scope (read/write).
    • Publish now targets a selected write-scope repo, stamps metadata, and reports the repo.
    • Subscribe/clone honors scope (read = shallow, write = full).
  • Bug Fixes / Behavior Changes

    • Sync is scope-aware: read = fetch+hard-reset, write = fetch+rebase to preserve local publishes.
    • Recall no longer loads from a separate public directory.
    • Remove warns for write-scope deletions and deletes only local clone.
  • Documentation

    • Storage layout and workflows updated to reflect the repos model (public dir removed).
  • Tests

    • Tests updated to the repos schema and new publish/sync/recall behaviors.

Replace separate public_repo + subscriptions config with a single
`repos:` list where each entry has `scope: read` or `scope: write`.
Write-scope repos are publish targets *and* synced, so co-writers see
each other's publishes. Updates publish/subscribe/sync/unsubscribe/recall
skills (claude + codex) and adds helpers in lib/config.py
(normalize_repos, read_repos, write_repos, set_repos, is_valid_repo_name).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 23, 2026

📝 Walkthrough

Walkthrough

Refactors sharing from a single public-repo + subscriptions model to a unified repos: config with explicit scope: read|write, updates config helpers and entity discovery (only .evolve/entities/), and adjusts publish/subscribe/sync/unsubscribe flows, storage layout, and tests accordingly.

Changes

Cohort / File(s) Summary
Core Config & Entity I/O
platform-integrations/claude/plugins/evolve-lite/lib/config.py, platform-integrations/claude/plugins/evolve-lite/lib/entity_io.py
Adds unified repos: model with VALID_SCOPES, safe-name regex, normalization/coercion helpers (normalize_repos, _coerce_repo, get_repo, read_repos, write_repos, set_repos, is_valid_repo_name), and limits recall discovery to .evolve/entities/ only.
Publish Skill (Claude & Codex)
platform-integrations/claude/.../skills/publish/SKILL.md, platform-integrations/claude/.../publish/scripts/publish.py, platform-integrations/codex/.../skills/publish/SKILL.md, platform-integrations/codex/.../publish/scripts/publish.py
Moves publish destination from .evolve/public/ to .evolve/entities/subscribed/{repo}/guideline/; adds --repo, enforces scope: write, stamps published_at/source from repo remote, updates push/rebase retry logic and audit logging.
Subscribe Skill (Claude & Codex)
platform-integrations/claude/.../skills/subscribe/SKILL.md, platform-integrations/claude/.../subscribe/scripts/subscribe.py, platform-integrations/codex/.../skills/subscribe/SKILL.md, platform-integrations/codex/.../subscribe/scripts/subscribe.py
Replaces subscriptions with repos: entries; adds --scope/--notes; validates names via is_valid_repo_name; clones to .evolve/entities/subscribed/{name} (shallow for read, full for write) and persists via set_repos().
Sync Skill (Claude & Codex)
platform-integrations/claude/.../skills/sync/SKILL.md, platform-integrations/claude/.../sync/scripts/sync.py, platform-integrations/codex/.../skills/sync/SKILL.md, platform-integrations/codex/.../sync/scripts/sync.py
Sync iterates normalized repos: and applies scope-specific git strategies: readgit fetch + git reset --hard origin/{branch}; writegit fetch + git rebase origin/{branch} (with abort on failure). Adds centralized _git() with timeout, shallow clone for read, per-repo [scope] labels, and session-start gating.
Recall / Retrieve
platform-integrations/claude/.../skills/recall/SKILL.md, platform-integrations/claude/.../recall/scripts/retrieve_entities.py, platform-integrations/codex/.../skills/recall/SKILL.md, platform-integrations/codex/.../recall/scripts/retrieve_entities.py
Drops .evolve/public/ as a discovery source; recall loads only from .evolve/entities/ (including subscribed clones), and simplify imports to use find_entities_dir().
Unsubscribe / Remove Repo
platform-integrations/claude/.../skills/unsubscribe/SKILL.md, platform-integrations/claude/.../unsubscribe/scripts/unsubscribe.py, platform-integrations/codex/.../skills/unsubscribe/SKILL.md, platform-integrations/codex/.../unsubscribe/scripts/unsubscribe.py
Renames action to remove a repo from repos:; validates names, deletes only .evolve/entities/subscribed/{name}, warns for scope: write (loss of unpushed publishes), and persists via set_repos().
Docs / READMEs
platform-integrations/claude/plugins/evolve-lite/README.md, platform-integrations/codex/plugins/evolve-lite/README.md
Rewrites docs to document repos: schema, updated .evolve/entities/subscribed/ layout, scope semantics, and revised skill behaviors.
Tests
tests/platform_integrations/test_config.py, tests/platform_integrations/test_entity_io.py, tests/platform_integrations/test_codex_sharing.py, tests/platform_integrations/test_publish.py, tests/platform_integrations/test_subscribe.py, tests/platform_integrations/test_sync.py, tests/platform_integrations/test_retrieve.py
Migrates tests from subscriptions/public_repo to repos: with scope; adds unit tests for config helpers (normalize_repos, set_repos, read_repos, write_repos, get_repo, is_valid_repo_name); updates publish/recall assertions to use subscribed clone paths and repo-derived source; adds publish failure/validation cases.

Sequence Diagram(s)

sequenceDiagram
    participant User as CLI/User
    participant FS as Local FS (.evolve/entities/subscribed/{repo})
    participant Git as Remote Git
    User->>FS: invoke publish --repo <name>
    FS->>FS: validate repo exists & .git present
    FS->>FS: stamp frontmatter (published_at, source from remote)
    FS->>FS: write guideline to subscribed/{repo}/guideline/
    FS->>Git: git add/commit && git push origin/{branch}
    alt push rejected (non-FF)
        Git-->>FS: reject
        FS->>Git: git fetch && git rebase origin/{branch}
        alt rebase success
            FS->>Git: git push
        else rebase conflict
            FS->>User: prompt to abort/resolve (interactive)
        end
    end
    FS->>User: success (repo recorded in audit)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • visahak
  • vinodmut

Poem

🐇 Hopping through cloned dirs, names tidy and bright,

Read and write now share the same cozy site,
No lone public folder—just subscribed trees,
Rebase for the writers, fetch-reset for the reads,
A rabbit’s small cheer for repos done right.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.36% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: unifying the previous public_repo + subscriptions model into a single scoped repos list.
Linked Issues check ✅ Passed The PR fully implements the requirements from issue #217: unifies repos into a single scoped list (name, scope, notes), supports multi-reader/multi-writer semantics, exposes UI for selecting target namespaces for publishing, and includes config helpers for unified repos management.
Out of Scope Changes check ✅ Passed All changes are directly aligned with issue #217 objectives: refactoring config models (public_repo + subscriptions → repos), updating skill workflows, adding config helpers, and extending test coverage. No unrelated changes detected.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/issue-217-memory-sharing-repo

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
tests/platform_integrations/test_sync.py (1)

128-132: ⚠️ Potential issue | 🟡 Minor

Doc/script message inconsistency.

This test correctly matches the script output "No subscriptions configured...", but the updated sync/SKILL.md now tells the assistant to display "All repos are up to date." for the no-repos-configured case. Either update sync.py to emit the new phrasing (and then update this assertion) or revise the SKILL doc to reflect the actual script output. Otherwise users will see a message that doesn't match what the skill doc advertises.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/platform_integrations/test_sync.py` around lines 128 - 132, The test
and SKILL doc disagree on the no-subscriptions message: update the script logic
in sync.py (where it prints the no-subscriptions message used by
test_no_subscriptions_exits_cleanly) to emit the SKILL.md phrasing "All repos
are up to date." (or alternatively change SKILL.md to match the current
message); if you choose to change the code, locate the print/log statement in
sync.py that outputs "No subscriptions..." and replace the text with "All repos
are up to date.", then update the test assertion in
test_no_subscriptions_exits_cleanly to expect the new string.
platform-integrations/claude/plugins/evolve-lite/skills/subscribe/scripts/subscribe.py (1)

83-109: ⚠️ Potential issue | 🟡 Minor

Clean up the clone if config persistence fails.

git clone runs at line 97 before set_repos + save_config at lines 108‑109. If save_config raises (disk full, permission error, YAML dump bug), the on-disk clone at dest is orphaned — no config entry references it, and a retry of subscribe --name <same> will fail the "directory already exists" check at line 83 and force the user to manually rm -rf it.

Wrap the post-clone steps in a try/except that removes dest on failure, e.g.:

🛠 Proposed cleanup-on-failure
     subprocess.run(clone_cmd, check=True)
 
-    repos.append(
-        {
-            "name": args.name,
-            "scope": args.scope,
-            "remote": args.remote,
-            "branch": args.branch,
-            "notes": args.notes,
-        }
-    )
-    set_repos(cfg, repos)
-    save_config(cfg, project_root)
+    try:
+        repos.append(
+            {
+                "name": args.name,
+                "scope": args.scope,
+                "remote": args.remote,
+                "branch": args.branch,
+                "notes": args.notes,
+            }
+        )
+        set_repos(cfg, repos)
+        save_config(cfg, project_root)
+    except Exception:
+        import shutil
+        shutil.rmtree(dest, ignore_errors=True)
+        raise
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@platform-integrations/claude/plugins/evolve-lite/skills/subscribe/scripts/subscribe.py`
around lines 83 - 109, After running the git clone (clone_cmd / subprocess.run)
the code calls set_repos(cfg, repos) and save_config(cfg, project_root) but does
not clean up the newly created dest if persistence fails; wrap the post-clone
steps (appending to repos, set_repos and save_config) in a try/except/finally so
that on any exception you remove the on-disk clone at dest (e.g., rmtree dest)
and re-raise the error, ensuring dest is only left behind when
set_repos/save_config succeed.
platform-integrations/claude/plugins/evolve-lite/README.md (1)

244-266: ⚠️ Potential issue | 🟡 Minor

Plugin Structure tree is stale — missing the sharing skills this PR documents.

The "Plugin Structure" diagram still lists only learn/, recall/, save/, and save-trajectory/, but the Sharing section above now references subscribe, publish, sync, and unsubscribe extensively. Readers scanning the tree will be misled about what's shipped.

📚 Proposed tree update
 ├── skills/
 │   ├── learn/
 │   │   ├── SKILL.md
 │   │   └── scripts/
 │   │       └── save_entities.py
 │   ├── recall/
 │   │   ├── SKILL.md
 │   │   └── scripts/
 │   │       └── retrieve_entities.py
+│   ├── subscribe/
+│   │   ├── SKILL.md
+│   │   └── scripts/
+│   │       └── subscribe.py
+│   ├── publish/
+│   │   ├── SKILL.md
+│   │   └── scripts/
+│   │       └── publish.py
+│   ├── sync/
+│   │   ├── SKILL.md
+│   │   └── scripts/
+│   │       └── sync.py
+│   ├── unsubscribe/
+│   │   ├── SKILL.md
+│   │   └── scripts/
+│   │       └── unsubscribe.py
 │   ├── save/
 │   │   └── SKILL.md
 │   └── save-trajectory/

Adjust to match actual filenames if they differ.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@platform-integrations/claude/plugins/evolve-lite/README.md` around lines 244
- 266, The plugin README's "Plugin Structure" tree is out of date and omits the
new sharing skills; update the tree under the evolve/ example (in README.md) to
include the new skills directories referenced elsewhere—add entries for
skills/subscribe/, skills/publish/, skills/sync/, and skills/unsubscribe/ (each
with SKILL.md and any scripts/ subfolders as appropriate) so the diagram matches
the documented Sharing section and actual filenames like subscribe, publish,
sync, unsubscribe.
🧹 Nitpick comments (10)
tests/platform_integrations/test_sync.py (1)

38-58: Assertion only meaningfully covers the third parametrization.

not (evolve_dir / "entities" / "subscribed" / "alice").exists() is trivially true for the name: 123 and name: " " cases since those names would never map to alice/. Consider parametrizing the expected-missing path alongside the config so each case's "nothing was written" invariant is actually checked (e.g., for name: 123, assert no 123/ directory exists).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/platform_integrations/test_sync.py` around lines 38 - 58, The test
test_sync_handles_malformed_repo_entries only checks that "alice" wasn't
written, which doesn't validate the other parametrized configs; update the
parametrization to include an expected-missing repo name/path alongside each
config_text (e.g., add a tuple like (config_text, expected_missing_dir_name) for
the three cases) and change the final assertion to check that (evolve_dir /
"entities" / "subscribed" / expected_missing_dir_name).exists() is false; modify
uses of SYNC_SCRIPT_VARIANTS and the test signature to accept the extra
parameter and update the assertion so each case verifies the correct directory
was not created.
platform-integrations/claude/plugins/evolve-lite/lib/entity_io.py (1)

78-87: Consider removing find_recall_entity_dirs() as it appears to be dead code.

With the removal of the public/ candidate, this function now returns only what find_entities_dir() already provides. The codebase shows no non-test callers—the definition exists but is not invoked anywhere. Since both recall scripts were updated to call find_entities_dir() directly, this function can be safely removed along with its tests to keep the API surface tight.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@platform-integrations/claude/plugins/evolve-lite/lib/entity_io.py` around
lines 78 - 87, The function find_recall_entity_dirs is dead code now because it
only returns the same path as find_entities_dir and has no non-test callers;
remove the function definition for find_recall_entity_dirs from entity_io.py and
delete any associated tests that target it, and update any references (none
expected) to use find_entities_dir instead; ensure imports and module exports
are adjusted so removing find_recall_entity_dirs does not leave unused imports
or break test discovery.
tests/platform_integrations/test_subscribe.py (1)

87-110: Consider parametrizing write-scope coverage across both platforms.

test_subscribe_rejects_invalid_name_characters runs against both Claude and Codex via SUBSCRIBE_SCRIPT_VARIANTS, but the new test_write_scope_recorded_in_repos_list (and the rest of TestSubscribe) only exercises SUBSCRIBE_SCRIPT (Claude). Since the Codex subscribe script was also updated for --scope/--notes, adding at least the write-scope happy path to the parametrized set would catch any Codex-side regression.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/platform_integrations/test_subscribe.py` around lines 87 - 110, The new
test test_write_scope_recorded_in_repos_list only exercises SUBSCRIBE_SCRIPT
(Claude) but should run against both implementations; change the test to be
parametrized over SUBSCRIBE_SCRIPT_VARIANTS (or otherwise iterate the variants)
so the same happy-path assertions (using run_script, checking
cfg_module.write_repos) execute for each variant; update references in the test
to accept a parameter like subscribe_script (or loop over
SUBSCRIBE_SCRIPT_VARIANTS) and call run_script(subscribe_script, ...) so
Codex-side regressions for --scope/--notes are caught as well.
platform-integrations/codex/plugins/evolve-lite/skills/subscribe/scripts/subscribe.py (1)

63-85: Consider rolling back the clone if config write fails.

If git clone succeeds but a subsequent failure (e.g., save_config) occurs, the directory .evolve/entities/subscribed/{name}/ is left on disk without a matching repos: entry, and a re-run will hit the "destination already exists" guard. Optional: wrap the config write in a try/except and shutil.rmtree(dest) on failure. Not a blocker — the existing dup/exists guards keep behavior safe, just slightly annoying to recover from.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@platform-integrations/codex/plugins/evolve-lite/skills/subscribe/scripts/subscribe.py`
around lines 63 - 85, The clone step can leave dest (the path
`.evolve/entities/subscribed/{name}/`) on disk if subsequent config writes
(set_repos/save_config) fail; wrap the subprocess.run(clone_cmd, check=True) +
repos append + set_repos(cfg, repos) + save_config(cfg, project_root) sequence
in a try/except (or try/finally) that on exception removes the created
destination directory (use shutil.rmtree(dest)) and then re-raises the error so
callers see the failure; reference the existing symbols clone_cmd,
subprocess.run, dest, repos, set_repos, and save_config to locate where to add
the rollback.
platform-integrations/claude/plugins/evolve-lite/lib/config.py (1)

336-349: Optional: dedupe set_repos against normalize_repos.

set_repos re-implements the same coerce+dedupe loop as normalize_repos. You can express one in terms of the other to keep the sanitization logic single-sourced:

♻️ Proposed simplification
 def set_repos(cfg, repos):
     """Replace the ``repos`` list in-place with sanitized entries."""
     if not isinstance(cfg, dict):
         return cfg
-    sanitized = []
-    seen = set()
-    for entry in repos or []:
-        repo = _coerce_repo(entry)
-        if repo is None or repo["name"] in seen:
-            continue
-        seen.add(repo["name"])
-        sanitized.append(repo)
-    cfg["repos"] = sanitized
+    cfg["repos"] = normalize_repos({"repos": list(repos or [])})
     return cfg
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@platform-integrations/claude/plugins/evolve-lite/lib/config.py` around lines
336 - 349, The set_repos function duplicates the coerce+dedupe logic already
implemented in normalize_repos; refactor set_repos to reuse normalize_repos so
the sanitization is single-sourced: keep the initial cfg type check, call
normalize_repos(repos) (which internally uses _coerce_repo and dedupes) and
assign its result to cfg["repos"], then return cfg — this removes the manual
loop and keeps behavior consistent with normalize_repos.
platform-integrations/claude/plugins/evolve-lite/skills/subscribe/scripts/subscribe.py (1)

59-70: Error wording is stale — says "subscription name" instead of "repo name".

The two error messages still read invalid subscription name: ..., but under the unified repos: model a subscribe with --scope write is a publish target rather than a "subscription". Minor nit — align with the new vocabulary used elsewhere in the PR (README, SKILL.md):

✏️ Wording tweak
-            f"Error: invalid subscription name: {args.name!r} (only A-Z, a-z, 0-9, '.', '_', '-' allowed)",
+            f"Error: invalid repo name: {args.name!r} (only A-Z, a-z, 0-9, '.', '_', '-' allowed)",
...
-        print(f"Error: invalid subscription name: {args.name!r}", file=sys.stderr)
+        print(f"Error: invalid repo name: {args.name!r}", file=sys.stderr)

The same wording appears in both Claude and Codex unsubscribe.py; consider sweeping those too.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@platform-integrations/claude/plugins/evolve-lite/skills/subscribe/scripts/subscribe.py`
around lines 59 - 70, The error messages in subscribe.py use outdated wording
"subscription name" — update both prints that reference args.name to say "repo
name" (or consistent repo vocabulary used elsewhere) so the check in
is_valid_repo_name(args.name) and the path-safety check comparing dest against
subscribed_base produce messages like "Error: invalid repo name: {args.name!r}".
Change both occurrences (the validation block using is_valid_repo_name and the
path check that compares dest.is_relative_to(subscribed_base) or dest ==
subscribed_base) and make the same wording change in the corresponding
unsubscribe.py to keep vocabulary consistent across the repo.
platform-integrations/codex/plugins/evolve-lite/README.md (1)

12-14: Minor: Feature bullets still reference "your public repo".

Line 12 says publish private guidelines to your public repo, but under the unified repos: model publishes go to a selected write-scope repo (which need not be a single "public" one). Not a blocker and not part of the modified lines, but consider aligning this bullet with the rest of the README for consistency, e.g.:

✏️ Wording tweak
-- Manual `evolve-lite:publish` skill to publish private guidelines to your public repo
+- Manual `evolve-lite:publish` skill to publish private guidelines to a configured write-scope repo
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@platform-integrations/codex/plugins/evolve-lite/README.md` around lines 12 -
14, Update the README bullet that says "publish private guidelines to your
public repo" to reflect the unified repos model: change wording to indicate
publishes target the configured write-scope repo(s) rather than a single public
repo (reference the `evolve-lite:publish` skill and the `repos:` model), e.g.
"publish private guidelines to the configured write-scope repo(s)". Ensure the
other bullets remain consistent with this terminology.
platform-integrations/codex/plugins/evolve-lite/skills/unsubscribe/scripts/unsubscribe.py (1)

54-60: Nit: validate name before constructing dest.

Purely stylistic — .resolve() is safe for arbitrary strings, and the combined guard on line 58 catches any bad name. But computing dest from an unvalidated name and then validating in one expression is harder to read than the Claude sibling's flow (validate → then build paths). Consider mirroring the Claude layout for consistency:

✏️ Ordering tweak
     name = args.name
+    if not is_valid_repo_name(name):
+        print(f"Error: invalid subscription name: {name!r}", file=sys.stderr)
+        sys.exit(1)
+
     subscribed_base = (evolve_dir / "entities" / "subscribed").resolve()
     dest = (evolve_dir / "entities" / "subscribed" / name).resolve()
-
-    if not is_valid_repo_name(name) or dest == subscribed_base or not dest.is_relative_to(subscribed_base):
+    if dest == subscribed_base or not dest.is_relative_to(subscribed_base):
         print(f"Error: invalid subscription name: {name!r}", file=sys.stderr)
         sys.exit(1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@platform-integrations/codex/plugins/evolve-lite/skills/unsubscribe/scripts/unsubscribe.py`
around lines 54 - 60, Move the validation of the subscription name to occur
before building the destination path: first check name with
is_valid_repo_name(name) and the special-case comparison against
subscribed_base, then only construct dest = (evolve_dir / "entities" /
"subscribed" / name).resolve() afterwards; update the conditional that currently
uses dest to instead use the pre-checks so the sequence reads validate name →
build dest → ensure dest.is_relative_to(subscribed_base).
platform-integrations/claude/plugins/evolve-lite/skills/publish/scripts/publish.py (1)

26-59: Duplicated helpers with the Codex publish script.

_resolve_source and _select_target_repo are byte-identical to the Codex version. Given the Codex plugin copies this lib/ at install time, a shared lib/publish_target.py (or additional helpers in lib/config.py) would let both scripts import them and keep behavior in lockstep — currently any bugfix/feature has to be applied twice, as the utcnow inconsistency above already shows.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@platform-integrations/claude/plugins/evolve-lite/skills/publish/scripts/publish.py`
around lines 26 - 59, The two helper functions _resolve_source and
_select_target_repo are duplicated from the Codex publish script; extract them
into a shared module (e.g., lib/publish_target.py or add to lib/config.py) and
update this file to import them instead of defining them locally, ensuring the
functions keep the same names and signatures so callers (e.g., publish.py)
continue to work; update the Codex plugin to import the same shared module,
remove the duplicated definitions here, and run/adjust any tests or imports that
reference _resolve_source or _select_target_repo to confirm behavior stays
identical (and fix any utcnow/other inconsistencies centrally).
platform-integrations/codex/plugins/evolve-lite/skills/publish/scripts/publish.py (1)

33-59: Consider lifting _select_target_repo/_resolve_source into the shared lib/.

These two helpers are duplicated byte-for-byte between Claude publish.py and this Codex publish.py. Since the installer copies Claude's lib/ into the Codex plugin at install time (per the Codex README), you can move both into e.g. lib/publish_target.py (or add them to lib/config.py) and drop the duplication — keeping the two call sites at a thin wrapper.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@platform-integrations/codex/plugins/evolve-lite/skills/publish/scripts/publish.py`
around lines 33 - 59, The two helpers _select_target_repo and _resolve_source
are duplicated; move their implementations into a shared lib module (e.g.,
create lib/publish_target.py or add to lib/config.py) and export them there,
then replace the duplicated bodies in both Claude's and Codex's publish.py with
thin wrappers that import and call lib._select_target_repo and
lib._resolve_source (or re-exported names) — update import paths accordingly and
ensure any references to write_repos, get_repo, normalize_repos remain
resolvable from the new module or passed in as parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@platform-integrations/claude/plugins/evolve-lite/lib/config.py`:
- Around line 278-280: _fix_ the silent coercion of unknown repo scopes in
_coerce_repo by first .strip()ing the incoming scope string, then checking
membership against VALID_SCOPES; if the stripped scope is not in VALID_SCOPES,
write a one-line warning to stderr (including the offending scope and the repo
identifier) and do NOT silently set scope = "read" — instead let normalize_repos
drop/skip that entry (aligning behavior with its docstring) or otherwise
propagate the invalid entry so normalize_repos can skip it; update
normalize_repos to skip entries with invalid scopes rather than relying on
_coerce_repo to coerce them.

In
`@platform-integrations/claude/plugins/evolve-lite/skills/publish/scripts/publish.py`:
- Line 104: Replace the naive call to datetime.datetime.utcnow() when setting
entity["published_at"] with a timezone-aware timestamp: call
datetime.datetime.now(datetime.timezone.utc).strftime(...) instead and ensure
datetime.timezone is available (i.e., keep using the existing datetime import or
add the timezone reference if needed) so the stored ISO string is UTC-aware and
matches the sibling Codex implementation.

In
`@platform-integrations/claude/plugins/evolve-lite/skills/sync/scripts/sync.py`:
- Around line 133-137: The current gate uses session_gated = args.session_start
or args.quiet which incorrectly treats --quiet as a session-start indicator;
change the logic to only use args.session_start (i.e., remove args.quiet from
session_gated) so the check against sync_cfg.get("on_session_start") only
short-circuits when args.session_start is true; keep the existing sys.exit(0)
behavior when on_session_start is False, and if any external hooks relied on the
old behavior update them to pass --session-start explicitly.
- Around line 189-198: The failure branches that handle pull_result being None
and pull_result.returncode != 0 should mark that a change occurred so summaries
are shown under --quiet; inside the block where pull_result is None and inside
the block after checking pull_result.returncode != 0 (the branches that append
"{name} (sync failed ...)" and set total_delta[name]), set any_changes = True so
failed syncs are not treated as "no changes" (these are the same branches you
see around the pull_result checks in sync.py).

In `@platform-integrations/codex/plugins/evolve-lite/skills/publish/SKILL.md`:
- Around line 38-69: Step 3 currently only selects and binds {repo} from the
repos: list but Step 6 references {branch}; update the Step 3 selection logic to
also extract and bind the chosen repo's branch (e.g., read the selected entry's
branch field) so {branch} is available for later, or alternatively change Step 6
to push without an explicit refspec (use the tracking push) — modify the
narrative and any examples in SKILL.md so the chosen repo's branch is captured
when selecting {repo} (or document the push-without-refspec behavior) so
{branch} is no longer undefined when running the git push in Step 6.

In `@platform-integrations/codex/plugins/evolve-lite/skills/sync/scripts/sync.py`:
- Around line 170-193: The two sync implementations diverge: Codex sets
any_changes = True on timeout and git-error branches while Claude does not; pick
the Codex behavior and make the Claude implementation match. In the Claude sync
script, update the timeout branch (where pull_result is None) and the git-error
branch (where pull_result.returncode != 0) to set any_changes = True (in
addition to appending to summaries and setting total_delta), so both syncs
behave identically for failures; reference the variables/blocks pull_result is
None, pull_result.returncode != 0, summaries, total_delta and any_changes when
making the changes.

---

Outside diff comments:
In `@platform-integrations/claude/plugins/evolve-lite/README.md`:
- Around line 244-266: The plugin README's "Plugin Structure" tree is out of
date and omits the new sharing skills; update the tree under the evolve/ example
(in README.md) to include the new skills directories referenced elsewhere—add
entries for skills/subscribe/, skills/publish/, skills/sync/, and
skills/unsubscribe/ (each with SKILL.md and any scripts/ subfolders as
appropriate) so the diagram matches the documented Sharing section and actual
filenames like subscribe, publish, sync, unsubscribe.

In
`@platform-integrations/claude/plugins/evolve-lite/skills/subscribe/scripts/subscribe.py`:
- Around line 83-109: After running the git clone (clone_cmd / subprocess.run)
the code calls set_repos(cfg, repos) and save_config(cfg, project_root) but does
not clean up the newly created dest if persistence fails; wrap the post-clone
steps (appending to repos, set_repos and save_config) in a try/except/finally so
that on any exception you remove the on-disk clone at dest (e.g., rmtree dest)
and re-raise the error, ensuring dest is only left behind when
set_repos/save_config succeed.

In `@tests/platform_integrations/test_sync.py`:
- Around line 128-132: The test and SKILL doc disagree on the no-subscriptions
message: update the script logic in sync.py (where it prints the
no-subscriptions message used by test_no_subscriptions_exits_cleanly) to emit
the SKILL.md phrasing "All repos are up to date." (or alternatively change
SKILL.md to match the current message); if you choose to change the code, locate
the print/log statement in sync.py that outputs "No subscriptions..." and
replace the text with "All repos are up to date.", then update the test
assertion in test_no_subscriptions_exits_cleanly to expect the new string.

---

Nitpick comments:
In `@platform-integrations/claude/plugins/evolve-lite/lib/config.py`:
- Around line 336-349: The set_repos function duplicates the coerce+dedupe logic
already implemented in normalize_repos; refactor set_repos to reuse
normalize_repos so the sanitization is single-sourced: keep the initial cfg type
check, call normalize_repos(repos) (which internally uses _coerce_repo and
dedupes) and assign its result to cfg["repos"], then return cfg — this removes
the manual loop and keeps behavior consistent with normalize_repos.

In `@platform-integrations/claude/plugins/evolve-lite/lib/entity_io.py`:
- Around line 78-87: The function find_recall_entity_dirs is dead code now
because it only returns the same path as find_entities_dir and has no non-test
callers; remove the function definition for find_recall_entity_dirs from
entity_io.py and delete any associated tests that target it, and update any
references (none expected) to use find_entities_dir instead; ensure imports and
module exports are adjusted so removing find_recall_entity_dirs does not leave
unused imports or break test discovery.

In
`@platform-integrations/claude/plugins/evolve-lite/skills/publish/scripts/publish.py`:
- Around line 26-59: The two helper functions _resolve_source and
_select_target_repo are duplicated from the Codex publish script; extract them
into a shared module (e.g., lib/publish_target.py or add to lib/config.py) and
update this file to import them instead of defining them locally, ensuring the
functions keep the same names and signatures so callers (e.g., publish.py)
continue to work; update the Codex plugin to import the same shared module,
remove the duplicated definitions here, and run/adjust any tests or imports that
reference _resolve_source or _select_target_repo to confirm behavior stays
identical (and fix any utcnow/other inconsistencies centrally).

In
`@platform-integrations/claude/plugins/evolve-lite/skills/subscribe/scripts/subscribe.py`:
- Around line 59-70: The error messages in subscribe.py use outdated wording
"subscription name" — update both prints that reference args.name to say "repo
name" (or consistent repo vocabulary used elsewhere) so the check in
is_valid_repo_name(args.name) and the path-safety check comparing dest against
subscribed_base produce messages like "Error: invalid repo name: {args.name!r}".
Change both occurrences (the validation block using is_valid_repo_name and the
path check that compares dest.is_relative_to(subscribed_base) or dest ==
subscribed_base) and make the same wording change in the corresponding
unsubscribe.py to keep vocabulary consistent across the repo.

In `@platform-integrations/codex/plugins/evolve-lite/README.md`:
- Around line 12-14: Update the README bullet that says "publish private
guidelines to your public repo" to reflect the unified repos model: change
wording to indicate publishes target the configured write-scope repo(s) rather
than a single public repo (reference the `evolve-lite:publish` skill and the
`repos:` model), e.g. "publish private guidelines to the configured write-scope
repo(s)". Ensure the other bullets remain consistent with this terminology.

In
`@platform-integrations/codex/plugins/evolve-lite/skills/publish/scripts/publish.py`:
- Around line 33-59: The two helpers _select_target_repo and _resolve_source are
duplicated; move their implementations into a shared lib module (e.g., create
lib/publish_target.py or add to lib/config.py) and export them there, then
replace the duplicated bodies in both Claude's and Codex's publish.py with thin
wrappers that import and call lib._select_target_repo and lib._resolve_source
(or re-exported names) — update import paths accordingly and ensure any
references to write_repos, get_repo, normalize_repos remain resolvable from the
new module or passed in as parameters.

In
`@platform-integrations/codex/plugins/evolve-lite/skills/subscribe/scripts/subscribe.py`:
- Around line 63-85: The clone step can leave dest (the path
`.evolve/entities/subscribed/{name}/`) on disk if subsequent config writes
(set_repos/save_config) fail; wrap the subprocess.run(clone_cmd, check=True) +
repos append + set_repos(cfg, repos) + save_config(cfg, project_root) sequence
in a try/except (or try/finally) that on exception removes the created
destination directory (use shutil.rmtree(dest)) and then re-raises the error so
callers see the failure; reference the existing symbols clone_cmd,
subprocess.run, dest, repos, set_repos, and save_config to locate where to add
the rollback.

In
`@platform-integrations/codex/plugins/evolve-lite/skills/unsubscribe/scripts/unsubscribe.py`:
- Around line 54-60: Move the validation of the subscription name to occur
before building the destination path: first check name with
is_valid_repo_name(name) and the special-case comparison against
subscribed_base, then only construct dest = (evolve_dir / "entities" /
"subscribed" / name).resolve() afterwards; update the conditional that currently
uses dest to instead use the pre-checks so the sequence reads validate name →
build dest → ensure dest.is_relative_to(subscribed_base).

In `@tests/platform_integrations/test_subscribe.py`:
- Around line 87-110: The new test test_write_scope_recorded_in_repos_list only
exercises SUBSCRIBE_SCRIPT (Claude) but should run against both implementations;
change the test to be parametrized over SUBSCRIBE_SCRIPT_VARIANTS (or otherwise
iterate the variants) so the same happy-path assertions (using run_script,
checking cfg_module.write_repos) execute for each variant; update references in
the test to accept a parameter like subscribe_script (or loop over
SUBSCRIBE_SCRIPT_VARIANTS) and call run_script(subscribe_script, ...) so
Codex-side regressions for --scope/--notes are caught as well.

In `@tests/platform_integrations/test_sync.py`:
- Around line 38-58: The test test_sync_handles_malformed_repo_entries only
checks that "alice" wasn't written, which doesn't validate the other
parametrized configs; update the parametrization to include an expected-missing
repo name/path alongside each config_text (e.g., add a tuple like (config_text,
expected_missing_dir_name) for the three cases) and change the final assertion
to check that (evolve_dir / "entities" / "subscribed" /
expected_missing_dir_name).exists() is false; modify uses of
SYNC_SCRIPT_VARIANTS and the test signature to accept the extra parameter and
update the assertion so each case verifies the correct directory was not
created.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c380b0fc-4c1c-4cdb-810e-2fb86417a11b

📥 Commits

Reviewing files that changed from the base of the PR and between cd4204c and 5750065.

📒 Files selected for processing (30)
  • platform-integrations/claude/plugins/evolve-lite/README.md
  • platform-integrations/claude/plugins/evolve-lite/lib/config.py
  • platform-integrations/claude/plugins/evolve-lite/lib/entity_io.py
  • platform-integrations/claude/plugins/evolve-lite/skills/publish/SKILL.md
  • platform-integrations/claude/plugins/evolve-lite/skills/publish/scripts/publish.py
  • platform-integrations/claude/plugins/evolve-lite/skills/recall/SKILL.md
  • platform-integrations/claude/plugins/evolve-lite/skills/recall/scripts/retrieve_entities.py
  • platform-integrations/claude/plugins/evolve-lite/skills/subscribe/SKILL.md
  • platform-integrations/claude/plugins/evolve-lite/skills/subscribe/scripts/subscribe.py
  • platform-integrations/claude/plugins/evolve-lite/skills/sync/SKILL.md
  • platform-integrations/claude/plugins/evolve-lite/skills/sync/scripts/sync.py
  • platform-integrations/claude/plugins/evolve-lite/skills/unsubscribe/SKILL.md
  • platform-integrations/claude/plugins/evolve-lite/skills/unsubscribe/scripts/unsubscribe.py
  • platform-integrations/codex/plugins/evolve-lite/README.md
  • platform-integrations/codex/plugins/evolve-lite/skills/publish/SKILL.md
  • platform-integrations/codex/plugins/evolve-lite/skills/publish/scripts/publish.py
  • platform-integrations/codex/plugins/evolve-lite/skills/recall/SKILL.md
  • platform-integrations/codex/plugins/evolve-lite/skills/recall/scripts/retrieve_entities.py
  • platform-integrations/codex/plugins/evolve-lite/skills/subscribe/SKILL.md
  • platform-integrations/codex/plugins/evolve-lite/skills/subscribe/scripts/subscribe.py
  • platform-integrations/codex/plugins/evolve-lite/skills/sync/SKILL.md
  • platform-integrations/codex/plugins/evolve-lite/skills/sync/scripts/sync.py
  • platform-integrations/codex/plugins/evolve-lite/skills/unsubscribe/SKILL.md
  • platform-integrations/codex/plugins/evolve-lite/skills/unsubscribe/scripts/unsubscribe.py
  • tests/platform_integrations/test_codex_sharing.py
  • tests/platform_integrations/test_config.py
  • tests/platform_integrations/test_entity_io.py
  • tests/platform_integrations/test_publish.py
  • tests/platform_integrations/test_subscribe.py
  • tests/platform_integrations/test_sync.py

Comment thread platform-integrations/claude/plugins/evolve-lite/lib/config.py Outdated
illeatmyhat and others added 2 commits April 23, 2026 11:53
The unified repos model (issue #217) has no `.evolve/public/guideline/`
directory — published entities now live under the write-scope repo's
clone at `.evolve/entities/subscribed/{repo}/guideline/` and are
annotated `[from: {repo}]` by recall. The `test_annotates_subscribed_
entities_with_from_source` test already covers that behavior.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- config: drop repo entries with unknown scope (after strip) and warn on
  stderr instead of silently coercing to read, matching normalize_repos
  docstring.
- publish (claude): replace deprecated datetime.utcnow() with
  timezone-aware datetime.now(timezone.utc); aligns with codex.
- sync (claude): stop gating on --quiet — only --session-start honors
  sync.on_session_start=false, matching codex behavior.
- sync (claude): mark any_changes=True on timeout / git-error branches so
  --quiet still surfaces failed syncs, matching codex.
- publish SKILL (claude/codex): bind {branch} alongside {repo} in Step 3
  so the Step 6 push refspec resolves for non-main branches.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
platform-integrations/claude/plugins/evolve-lite/skills/sync/scripts/sync.py (1)

172-181: ⚠️ Potential issue | 🟡 Minor

Uncaught TimeoutExpired on clone breaks the whole sync loop.

Unlike _git(), the subprocess.run(clone_cmd, ..., timeout=_GIT_TIMEOUT) call here does not catch subprocess.TimeoutExpired. If a single repo hangs on clone (slow mirror, auth prompt, large shallow fetch), the exception propagates, aborting the entire for repo in repos: loop — remaining repos are never synced, no summary is printed, and audit_append is skipped. The earlier timeout branch for fetch/rebase/reset handles this gracefully via _git() returning None; the clone path should do the same.

🔧 Proposed fix
             repo_path.parent.mkdir(parents=True, exist_ok=True)
             clone_cmd = ["git", "clone", remote, str(repo_path), "--branch", branch]
             if scope == "read":
                 clone_cmd += ["--depth", "1"]
-            clone_result = subprocess.run(
-                clone_cmd,
-                capture_output=True,
-                text=True,
-                timeout=_GIT_TIMEOUT,
-            )
-            if clone_result.returncode != 0:
+            try:
+                clone_result = subprocess.run(
+                    clone_cmd,
+                    capture_output=True,
+                    text=True,
+                    timeout=_GIT_TIMEOUT,
+                )
+            except subprocess.TimeoutExpired:
+                summaries.append(f"{name} (re-clone failed — timeout)")
+                total_delta[name] = {"added": 0, "updated": 0, "removed": 0}
+                any_changes = True
+                continue
+            if clone_result.returncode != 0:
                 summaries.append(f"{name} (re-clone failed: {clone_result.stderr.strip()})")
                 total_delta[name] = {"added": 0, "updated": 0, "removed": 0}
+                any_changes = True
                 continue

Note the any_changes = True on the clone-failure branch for consistency with the fetch/rebase failure handling below — otherwise --quiet still silences a failed first clone.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@platform-integrations/claude/plugins/evolve-lite/skills/sync/scripts/sync.py`
around lines 172 - 181, The clone subprocess.run call can raise
subprocess.TimeoutExpired and currently aborts the whole sync; wrap the clone
invocation in a try/except for subprocess.TimeoutExpired and handle it the same
way the fetch/rebase path handles timeouts (i.e., record a summary entry for
"{name} (re-clone timed out)", set total_delta[name] =
{"added":0,"updated":0,"removed":0}, set any_changes = True for consistency, and
continue the loop), mirroring the behavior of _git() so a single timed-out clone
doesn't stop processing the remaining repos.
platform-integrations/claude/plugins/evolve-lite/skills/publish/scripts/publish.py (1)

109-139: ⚠️ Potential issue | 🟡 Minor

Guard against publishing into a non-git directory before consuming the source entity.

dest_dir.mkdir(parents=True, exist_ok=True) at line 111 will happily create .evolve/entities/subscribed/{repo}/guideline/ even if the parent .evolve/entities/subscribed/{repo}/ is not a git clone (e.g., if SKILL.md Step 5 was skipped, or the script is invoked outside the skill flow). In that case:

  1. tmp_path.replace(dest_path) moves the stamped entity into a plain directory.
  2. src_path.unlink() at line 139 deletes the private source.
  3. The subsequent git -C ... add/commit/push in SKILL.md Step 7 fails — but the private guideline has already been consumed and there's nothing to push. The user silently loses the entity from .evolve/entities/guideline/.

A cheap defensive check (before mkdir or before the file move) avoids the data-loss window and keeps publish.py robust to direct invocation.

🛡️ Proposed guard
     # Destination: the local clone of the target write-scope repo.
     dest_dir = evolve_dir / "entities" / "subscribed" / target["name"] / "guideline"
+    clone_root = evolve_dir / "entities" / "subscribed" / target["name"]
+    if not (clone_root / ".git").exists():
+        print(
+            f"Error: target repo clone not found at {clone_root}. "
+            f"Run /evolve-lite:subscribe --scope write first (or ensure the clone exists).",
+            file=sys.stderr,
+        )
+        sys.exit(1)
     dest_dir.mkdir(parents=True, exist_ok=True)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@platform-integrations/claude/plugins/evolve-lite/skills/publish/scripts/publish.py`
around lines 109 - 139, The code currently creates dest_dir and writes the new
entity before verifying that the target subscribed repo is actually a git
working tree, risking silent loss when tmp_path.replace(dest_path) and
src_path.unlink() run; change the flow in publish.py to first determine the
subscribed repo root (the directory corresponding to target["name"] under
evolve_dir/entities/subscribed), run a lightweight git check (e.g.,
subprocess.run(["git","-C", repo_root, "rev-parse","--is-inside-work-tree"],
check=False) or verify a .git dir) and abort with an error if it is not a git
repo, and only then proceed to create dest_dir, move tmp_path to dest_path
(tmp_path.replace) and unlink src_path; also ensure src_path.unlink() happens
only after the git add/commit/push succeeds (or at least after verifying the
repo), so reference symbols to change: dest_dir/dest_base/dest_path creation and
the sequence around tmp_path.replace(dest_path) and src_path.unlink().
🧹 Nitpick comments (7)
platform-integrations/codex/plugins/evolve-lite/skills/publish/SKILL.md (1)

42-44: Consider removing duplicated zero-entry handling between Step 1 and Step 3.

Step 1 already gates on missing write-scope repos, so repeating the same stop condition here adds minor workflow redundancy.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@platform-integrations/codex/plugins/evolve-lite/skills/publish/SKILL.md`
around lines 42 - 44, Remove the duplicated zero-entry check from Step 3 of
SKILL.md: Step 1 already aborts when there are no write-scope repos, so delete
the “Zero entries → stop and tell the user to subscribe to a write-scope repo”
branch from Step 3 and ensure Step 3 only handles the “Exactly one entry” and
“Multiple entries” cases (the numbered list/notes flow). Verify any references
to the removed stop condition in Step 3 text are cleaned up so the workflow
reads sequentially from Step 1’s gate into Step 3’s selection logic.
tests/platform_integrations/test_config.py (4)

267-275: Minor gap: get_repo against a config missing repos entirely.

TestGetRepo covers an empty repos list but not the {} / repos key absent case, which is the common shape on fresh configs. A one-line assertion keeps behavior pinned.

🧪 Suggested addition
     def test_returns_none_when_missing(self):
         assert cfg_module.get_repo({"repos": []}, "missing") is None
+        assert cfg_module.get_repo({}, "missing") is None
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/platform_integrations/test_config.py` around lines 267 - 275, Add a
test case in TestGetRepo that calls cfg_module.get_repo on a config dict missing
the "repos" key (e.g., cfg_module.get_repo({}, "missing")) and assert it returns
None; this ensures get_repo handles configs without a "repos" entry and pins the
expected behavior.

278-285: Consider broadening is_valid_repo_name negative cases.

The rejection set is solid for common traversal/whitespace attacks, but a few adjacent hazards aren't asserted and are easy to regress on given this helper guards clone destinations: leading-dash names (could be interpreted as CLI flags by git), backslashes (alice\bob — path separator on Windows), and non-string types besides None (e.g., 0, []). Not blocking, just inexpensive hardening for a safety-critical helper.

🧪 Suggested addition
     def test_rejects_unsafe_names(self):
-        for name in ["", ".", "..", "alice/bob", "alice bob", "alice:bob", "../evil", None]:
+        for name in [
+            "", ".", "..", "alice/bob", "alice bob", "alice:bob", "../evil",
+            "-rf", "alice\\bob", None, 0, [],
+        ]:
             assert not cfg_module.is_valid_repo_name(name)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/platform_integrations/test_config.py` around lines 278 - 285, Extend
the negative test cases in TestIsValidRepoName to cover additional hazardous
inputs for cfg_module.is_valid_repo_name: add examples like names starting with
a dash ("-evil" or "-alice"), names containing backslashes ("alice\\bob"), and
non-string types beyond None (e.g., 0 and []), so the test_rejects_unsafe_names
loop asserts these are rejected as well; update the test_rejects_unsafe_names
method to include these values in its for-loop.

170-231: Missing coverage: scope defaulting to "read" when omitted.

The PR summary calls out that normalize_repos should default a missing scope to read, but none of the TestNormalizeRepos cases exercise an entry without a scope key and assert the defaulted value. Consider adding an explicit assertion so a regression that silently drops or mis-defaults scope would fail here.

🧪 Suggested addition
     def test_scope_whitespace_tolerated(self):
         cfg = {"repos": [{"name": "x", "scope": " write ", "remote": "git@x:y/z.git"}]}
         repos = cfg_module.normalize_repos(cfg)
         assert len(repos) == 1
         assert repos[0]["scope"] == "write"

+    def test_missing_scope_defaults_to_read(self):
+        cfg = {"repos": [{"name": "x", "remote": "git@x:y/z.git"}]}
+        repos = cfg_module.normalize_repos(cfg)
+        assert len(repos) == 1
+        assert repos[0]["scope"] == "read"
+
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/platform_integrations/test_config.py` around lines 170 - 231, Add a
test case in TestNormalizeRepos that verifies normalize_repos defaults a missing
"scope" to "read": create a cfg entry with "name" and "remote" but no "scope",
call cfg_module.normalize_repos(cfg), and assert the resulting repo has "scope"
== "read" (and still preserves name/remote); reference the normalize_repos
function to locate where behavior is implemented and ensure the test fails if
defaulting is removed or changed.

8-12: sys.path mutation for import is fragile but pre-existing.

Inserting the plugin lib/ onto sys.path at import time means any other test module that does the same for a different plugin's config.py (e.g., a Codex variant) can shadow this one depending on collection order, since both register under the unqualified config module name. Not changed by this PR — flagging only because the new helpers expand this file's surface area; a follow-up using importlib.util.spec_from_file_location with a namespaced module name would make the suite more robust.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/platform_integrations/test_config.py` around lines 8 - 12, Replace the
fragile sys.path.insert import pattern and the bare "import config as
cfg_module" with a file-based import using
importlib.util.spec_from_file_location to load the plugin's config.py under a
unique, namespaced module name (e.g.,
"platform_integrations.claude.evolve_lite.config"); locate the config file via
Path(__file__).parent.../"platform-integrations/claude/plugins/evolve-lite/lib/config.py",
create a spec from that path, load the module from the spec, and assign it to
cfg_module so tests no longer rely on mutating sys.path or the unqualified
"config" name.
platform-integrations/claude/plugins/evolve-lite/skills/sync/scripts/sync.py (1)

169-169: Optional: use -- to separate options from positional remote.

remote and str(repo_path) are placed before --branch, so a repo whose remote happens to begin with - (e.g., a malformed or adversarial config) would be interpreted by git as an option. Not a concern for normal configs, but since remote comes from user-edited YAML, inserting -- after the options is cheap defense-in-depth.

♻️ Suggested shape
-            clone_cmd = ["git", "clone", remote, str(repo_path), "--branch", branch]
-            if scope == "read":
-                clone_cmd += ["--depth", "1"]
+            clone_cmd = ["git", "clone", "--branch", branch]
+            if scope == "read":
+                clone_cmd += ["--depth", "1"]
+            clone_cmd += ["--", remote, str(repo_path)]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@platform-integrations/claude/plugins/evolve-lite/skills/sync/scripts/sync.py`
at line 169, The git clone command builds clone_cmd in sync.py with remote and
repo_path before the --branch option; to prevent a remote that starts with '-'
being parsed as an option, update the clone_cmd construction (the variable named
clone_cmd in this file) to insert the argument separator '--' between the clone
options and the positional arguments so that remote and repo_path are treated
strictly as positional parameters.
platform-integrations/claude/plugins/evolve-lite/skills/publish/SKILL.md (1)

92-100: Nit: consider fetch+rebase before push to avoid rejected pushes from co-writers.

Step 7 goes straight from commit to push origin "{branch}" without fetching first. In a multi-writer write-scope repo (the exact scenario the PR enables), a co-writer's push between the user's last sync and this publish will reject this push and leave the skill in a partial state (entity already committed locally, not on the remote). sync handles the rebase path, but publish itself doesn't require a prior sync.

A minimal defensive tweak is git pull --rebase origin "{branch}" between commit and push (or a retry hint in the docs). Non-blocking — feel free to defer if you expect low contention.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@platform-integrations/claude/plugins/evolve-lite/skills/publish/SKILL.md`
around lines 92 - 100, Step 7 currently commits then pushes without syncing
remote changes; update the sequence around the commit/push in SKILL.md to add a
fetch+rebase (e.g. run git pull --rebase origin "{branch}" or an equivalent
fetch+rebase step) after the commit and before git -C
".evolve/entities/subscribed/{repo}" push origin "{branch}" so the commit
message build {filenames_list} is rebased onto the latest remote and avoids
rejected pushes from co-writers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In
`@platform-integrations/claude/plugins/evolve-lite/skills/publish/scripts/publish.py`:
- Around line 109-139: The code currently creates dest_dir and writes the new
entity before verifying that the target subscribed repo is actually a git
working tree, risking silent loss when tmp_path.replace(dest_path) and
src_path.unlink() run; change the flow in publish.py to first determine the
subscribed repo root (the directory corresponding to target["name"] under
evolve_dir/entities/subscribed), run a lightweight git check (e.g.,
subprocess.run(["git","-C", repo_root, "rev-parse","--is-inside-work-tree"],
check=False) or verify a .git dir) and abort with an error if it is not a git
repo, and only then proceed to create dest_dir, move tmp_path to dest_path
(tmp_path.replace) and unlink src_path; also ensure src_path.unlink() happens
only after the git add/commit/push succeeds (or at least after verifying the
repo), so reference symbols to change: dest_dir/dest_base/dest_path creation and
the sequence around tmp_path.replace(dest_path) and src_path.unlink().

In
`@platform-integrations/claude/plugins/evolve-lite/skills/sync/scripts/sync.py`:
- Around line 172-181: The clone subprocess.run call can raise
subprocess.TimeoutExpired and currently aborts the whole sync; wrap the clone
invocation in a try/except for subprocess.TimeoutExpired and handle it the same
way the fetch/rebase path handles timeouts (i.e., record a summary entry for
"{name} (re-clone timed out)", set total_delta[name] =
{"added":0,"updated":0,"removed":0}, set any_changes = True for consistency, and
continue the loop), mirroring the behavior of _git() so a single timed-out clone
doesn't stop processing the remaining repos.

---

Nitpick comments:
In `@platform-integrations/claude/plugins/evolve-lite/skills/publish/SKILL.md`:
- Around line 92-100: Step 7 currently commits then pushes without syncing
remote changes; update the sequence around the commit/push in SKILL.md to add a
fetch+rebase (e.g. run git pull --rebase origin "{branch}" or an equivalent
fetch+rebase step) after the commit and before git -C
".evolve/entities/subscribed/{repo}" push origin "{branch}" so the commit
message build {filenames_list} is rebased onto the latest remote and avoids
rejected pushes from co-writers.

In
`@platform-integrations/claude/plugins/evolve-lite/skills/sync/scripts/sync.py`:
- Line 169: The git clone command builds clone_cmd in sync.py with remote and
repo_path before the --branch option; to prevent a remote that starts with '-'
being parsed as an option, update the clone_cmd construction (the variable named
clone_cmd in this file) to insert the argument separator '--' between the clone
options and the positional arguments so that remote and repo_path are treated
strictly as positional parameters.

In `@platform-integrations/codex/plugins/evolve-lite/skills/publish/SKILL.md`:
- Around line 42-44: Remove the duplicated zero-entry check from Step 3 of
SKILL.md: Step 1 already aborts when there are no write-scope repos, so delete
the “Zero entries → stop and tell the user to subscribe to a write-scope repo”
branch from Step 3 and ensure Step 3 only handles the “Exactly one entry” and
“Multiple entries” cases (the numbered list/notes flow). Verify any references
to the removed stop condition in Step 3 text are cleaned up so the workflow
reads sequentially from Step 1’s gate into Step 3’s selection logic.

In `@tests/platform_integrations/test_config.py`:
- Around line 267-275: Add a test case in TestGetRepo that calls
cfg_module.get_repo on a config dict missing the "repos" key (e.g.,
cfg_module.get_repo({}, "missing")) and assert it returns None; this ensures
get_repo handles configs without a "repos" entry and pins the expected behavior.
- Around line 278-285: Extend the negative test cases in TestIsValidRepoName to
cover additional hazardous inputs for cfg_module.is_valid_repo_name: add
examples like names starting with a dash ("-evil" or "-alice"), names containing
backslashes ("alice\\bob"), and non-string types beyond None (e.g., 0 and []),
so the test_rejects_unsafe_names loop asserts these are rejected as well; update
the test_rejects_unsafe_names method to include these values in its for-loop.
- Around line 170-231: Add a test case in TestNormalizeRepos that verifies
normalize_repos defaults a missing "scope" to "read": create a cfg entry with
"name" and "remote" but no "scope", call cfg_module.normalize_repos(cfg), and
assert the resulting repo has "scope" == "read" (and still preserves
name/remote); reference the normalize_repos function to locate where behavior is
implemented and ensure the test fails if defaulting is removed or changed.
- Around line 8-12: Replace the fragile sys.path.insert import pattern and the
bare "import config as cfg_module" with a file-based import using
importlib.util.spec_from_file_location to load the plugin's config.py under a
unique, namespaced module name (e.g.,
"platform_integrations.claude.evolve_lite.config"); locate the config file via
Path(__file__).parent.../"platform-integrations/claude/plugins/evolve-lite/lib/config.py",
create a spec from that path, load the module from the spec, and assign it to
cfg_module so tests no longer rely on mutating sys.path or the unqualified
"config" name.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 385a9c7d-6ee6-4ae7-ba01-8454574ab55a

📥 Commits

Reviewing files that changed from the base of the PR and between 5750065 and 85b16bf.

📒 Files selected for processing (7)
  • platform-integrations/claude/plugins/evolve-lite/lib/config.py
  • platform-integrations/claude/plugins/evolve-lite/skills/publish/SKILL.md
  • platform-integrations/claude/plugins/evolve-lite/skills/publish/scripts/publish.py
  • platform-integrations/claude/plugins/evolve-lite/skills/sync/scripts/sync.py
  • platform-integrations/codex/plugins/evolve-lite/skills/publish/SKILL.md
  • tests/platform_integrations/test_config.py
  • tests/platform_integrations/test_retrieve.py
💤 Files with no reviewable changes (1)
  • tests/platform_integrations/test_retrieve.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • platform-integrations/claude/plugins/evolve-lite/lib/config.py

illeatmyhat and others added 3 commits April 24, 2026 10:35
Bugs flagged as "outside diff range":
- sync (claude+codex): wrap subprocess.run(clone_cmd) in try/except
  TimeoutExpired so a single hung clone no longer aborts the whole sync
  loop; also set any_changes=True on clone failures for --quiet parity.
- publish (claude+codex): refuse to move the source entity into the
  target subscribed repo if it's not a git clone (no .git dir). Without
  this guard, running publish before subscribe silently consumed the
  private guideline with nothing to push. Added
  test_publish_errors_when_target_clone_missing to pin the behavior.

Defense-in-depth:
- sync (claude+codex): use `--` separator in git clone so a remote
  string beginning with `-` cannot be parsed as a git option.
- config: is_valid_repo_name now rejects leading `-` so repo names
  can't masquerade as git CLI flags when interpolated into commands.

Doc / test cleanup (nitpicks):
- codex publish SKILL: drop duplicated zero-entry guard in Step 3 —
  Step 1 already aborts for the no-write-repo case.
- claude sync SKILL: align displayed phrasing with the script's actual
  stdout ("No subscriptions configured...") instead of inventing
  "All repos are up to date." — which test_sync.py already pins.
- claude README: add subscribe/publish/sync/unsubscribe to the
  "Plugin Structure" tree so it reflects the shipped sharing skills.
- tests: cover get_repo({}, name), missing-scope default-to-read,
  and hardened is_valid_repo_name negative inputs (leading dash,
  backslash, non-string types).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Keep the publish happy path local-only (commit → push, no preemptive
fetch), but teach both SKILL.mds a Step 6a/7a that triggers when push
is rejected non-fast-forward:

1. fetch origin/{branch} + rebase
2. rebase clean → push once more, done
3. rebase conflicted → rebase --abort and surface an explicit manual
   recovery path to the user. Local commit is preserved; nothing is
   auto-resolved, nothing is left mid-rebase.

Other push failures (auth, network, missing ref) surface git's error
as-is — rebase wouldn't help and trying it masks the real cause.

This handles the common "co-writer pushed first" case without paying
the fetch cost on every publish and without forcing the user into a
mid-rebase state the skill can't drive.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Expand Step 7a (claude) / Step 6a (codex) so the rebase-conflict branch
attempts a best-effort resolution first, then hands the user a review
gate before continuing. Abort is still reachable — it's now the second
option rather than the default.

Flow when rebase conflicts:
1. `git status --porcelain` to list conflicted files. If any are
   UD/DU/binary, skip straight to abort — those aren't safe to merge.
2. For each UU/AA file, read the markers (noting that in a rebase
   HEAD is the remote and the incoming side is our publish change),
   write an intent-preserving resolution without `git add`.
3. Show the user per-file strategy + `git diff HEAD -- {file}` and
   ask for an explicit continue/abort.
4. Continue → add resolved files, `rebase --continue`, push. Loops if
   a later commit conflicts (unusual for publish's single commit).
5. Abort → `rebase --abort`, preserve the local commit, hand off
   existing manual recovery instructions.

Key invariants:
- Nothing auto-advances past the conflict without explicit user
  approval.
- Abort is always reachable and leaves the user in the same state as
  if the recovery attempt never ran (local commit intact, unpushed).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
platform-integrations/codex/plugins/evolve-lite/skills/sync/scripts/sync.py (1)

196-206: ⚠️ Potential issue | 🟡 Minor

Fresh clones are invisible under --quiet because delta reports 0/0/0.

After a successful clone, the code falls through to sync_read_only/sync_writable, which immediately fetch + reset/rebase. On a just-cloned repo where origin hasn't moved, git diff HEAD@{1} HEAD returns empty, so delta = {0,0,0}, any_changes stays False, and --quiet exits silently at line 205 — even though a new repo was just populated on disk. Consider flipping any_changes = True (or a dedicated cloned flag) along the successful-clone path so the user sees the new subscription appear in the summary.

💡 Proposed tweak
             if clone_result.returncode != 0:
                 summaries.append(f"{name} (re-clone failed: {clone_result.stderr.strip()})")
                 total_delta[name] = {"added": 0, "updated": 0, "removed": 0}
                 any_changes = True
                 continue
+            # Fresh clone is itself a noteworthy change to report.
+            any_changes = True
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@platform-integrations/codex/plugins/evolve-lite/skills/sync/scripts/sync.py`
around lines 196 - 206, The summary shows freshly cloned repos as unchanged
because count_delta returns zeros after a clone followed by fetch/reset, so
any_changes remains False and --quiet exits; update the successful-clone path
(in the clone logic inside sync_read_only/sync_writable where the repository is
freshly populated) to set any_changes = True (or set a dedicated cloned flag and
propagate it back to the caller) so that after audit_append and before the
args.quiet check the code treats a fresh clone as a change; ensure the symbol
names involved are total_delta, any_changes (or cloned), summaries and
count_delta so the caller can still append the human-readable summary and not
exit silently under args.quiet.
♻️ Duplicate comments (1)
platform-integrations/codex/plugins/evolve-lite/skills/publish/SKILL.md (1)

84-85: ⚠️ Potential issue | 🟡 Minor

Make the retry push command explicit with -C to avoid cwd ambiguity.

Step 6 uses git -C, but Step 6a retry is prose-only and can be executed in the wrong directory by agents.

Suggested doc tweak
-- Rebase clean → retry `git push origin "{branch}"` once, then Step 7.
+- Rebase clean → retry once:
+  `git -C ".evolve/entities/subscribed/{repo}" push origin "{branch}"`,
+  then Step 7.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@platform-integrations/codex/plugins/evolve-lite/skills/publish/SKILL.md`
around lines 84 - 85, Update the retry instruction in SKILL.md so the retry push
is an explicit git command with -C to avoid cwd ambiguity: replace the prose
"retry `git push origin \"{branch}\"` once" with a clear instruction to run `git
-C "{repo_dir}" push origin "{branch}"` once (or equivalent text mentioning git
-C and the repository directory variable) so agents will execute the push in the
correct repo directory before proceeding to Step 7; edit the line in the Rebase
clean / Rebase conflicted section accordingly.
🧹 Nitpick comments (2)
platform-integrations/codex/plugins/evolve-lite/skills/sync/scripts/sync.py (2)

151-176: Clone timeout may be tight for full (write-scope) clones.

_GIT_TIMEOUT = 30 is reused here for git clone. Shallow read-scope clones (--depth 1) will usually fit, but full write-scope clones of sizeable memory repos over slow links can blow this budget and surface as re-clone failed - timeout on the first sync after subscribe. Consider a longer clone-specific timeout (e.g., 120s) or making it configurable via env.

⏱️ Suggested diff
+_CLONE_TIMEOUT = int(os.environ.get("EVOLVE_CLONE_TIMEOUT", "120"))
@@
             try:
                 clone_result = subprocess.run(
                     clone_cmd,
                     capture_output=True,
                     text=True,
-                    timeout=_GIT_TIMEOUT,
+                    timeout=_CLONE_TIMEOUT,
                 )
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@platform-integrations/codex/plugins/evolve-lite/skills/sync/scripts/sync.py`
around lines 151 - 176, The current git clone uses the shared _GIT_TIMEOUT (30s)
which is too short for full write-scope clones; change the clone timeout used in
the subprocess.run call to a larger, clone-specific timeout (e.g., 120s) or make
it configurable via an environment variable, e.g., introduce a new
_GIT_CLONE_TIMEOUT or read os.environ for a CLONE_TIMEOUT and pass that to
subprocess.run where clone_cmd is executed (the block that constructs clone_cmd
and calls subprocess.run and checks clone_result.returncode), leaving the
existing _GIT_TIMEOUT for other git operations.

134-148: User-facing wording still says "subscription" in the new repos model.

The changed line 135 (and the neighboring unchanged lines 140 and 148) still emit (skipped - invalid subscription name) / (skipped - invalid subscription config). Since this PR renames the model from subscriptions to a unified repos: list, it'd be consistent to say "repo" here. Low priority.

✏️ Proposed wording tweak
         if not is_valid_repo_name(raw_name):
-            summaries.append(f"{raw_name!r} (skipped - invalid subscription name)")
+            summaries.append(f"{raw_name!r} (skipped - invalid repo name)")
             continue
         name = raw_name.strip()

         if not isinstance(branch, str) or not branch.strip():
-            summaries.append(f"{raw_name!r} (skipped - invalid subscription config)")
+            summaries.append(f"{raw_name!r} (skipped - invalid repo config)")
             continue
         branch = branch.strip()

         subscribed_base = (evolve_dir / "entities" / "subscribed").resolve()
         repo_path = (evolve_dir / "entities" / "subscribed" / name).resolve()

         if repo_path == subscribed_base or not repo_path.is_relative_to(subscribed_base):
-            summaries.append(f"{name!r} (skipped - invalid subscription name)")
+            summaries.append(f"{name!r} (skipped - invalid repo name)")
             continue
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@platform-integrations/codex/plugins/evolve-lite/skills/sync/scripts/sync.py`
around lines 134 - 148, Update the user-facing messages to refer to the new
"repo" model instead of "subscription": in the validation blocks where
is_valid_repo_name(raw_name) and the branch check append to summaries
(referencing variables raw_name, name, branch, and the summaries list), replace
"(skipped - invalid subscription name)" with "(skipped - invalid repo name)" and
"(skipped - invalid subscription config)" with "(skipped - invalid repo
config)"; also update the final repo_path check that currently appends "(skipped
- invalid subscription name)" to use "(skipped - invalid repo name)" so
messaging is consistent with the unified repos: model.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@platform-integrations/claude/plugins/evolve-lite/skills/publish/SKILL.md`:
- Around line 75-77: The documented git clone command should be hardened against
remotes starting with '-' by moving option flags before positional args and
adding the end-of-options marker --; update the clone invocation (the git clone
line in SKILL.md) to place --branch "{branch}" before the repo and insert --
before the "{remote}" argument so the command becomes: git clone --branch
"{branch}" -- "{remote}" ".evolve/entities/subscribed/{repo}".

---

Outside diff comments:
In `@platform-integrations/codex/plugins/evolve-lite/skills/sync/scripts/sync.py`:
- Around line 196-206: The summary shows freshly cloned repos as unchanged
because count_delta returns zeros after a clone followed by fetch/reset, so
any_changes remains False and --quiet exits; update the successful-clone path
(in the clone logic inside sync_read_only/sync_writable where the repository is
freshly populated) to set any_changes = True (or set a dedicated cloned flag and
propagate it back to the caller) so that after audit_append and before the
args.quiet check the code treats a fresh clone as a change; ensure the symbol
names involved are total_delta, any_changes (or cloned), summaries and
count_delta so the caller can still append the human-readable summary and not
exit silently under args.quiet.

---

Duplicate comments:
In `@platform-integrations/codex/plugins/evolve-lite/skills/publish/SKILL.md`:
- Around line 84-85: Update the retry instruction in SKILL.md so the retry push
is an explicit git command with -C to avoid cwd ambiguity: replace the prose
"retry `git push origin \"{branch}\"` once" with a clear instruction to run `git
-C "{repo_dir}" push origin "{branch}"` once (or equivalent text mentioning git
-C and the repository directory variable) so agents will execute the push in the
correct repo directory before proceeding to Step 7; edit the line in the Rebase
clean / Rebase conflicted section accordingly.

---

Nitpick comments:
In `@platform-integrations/codex/plugins/evolve-lite/skills/sync/scripts/sync.py`:
- Around line 151-176: The current git clone uses the shared _GIT_TIMEOUT (30s)
which is too short for full write-scope clones; change the clone timeout used in
the subprocess.run call to a larger, clone-specific timeout (e.g., 120s) or make
it configurable via an environment variable, e.g., introduce a new
_GIT_CLONE_TIMEOUT or read os.environ for a CLONE_TIMEOUT and pass that to
subprocess.run where clone_cmd is executed (the block that constructs clone_cmd
and calls subprocess.run and checks clone_result.returncode), leaving the
existing _GIT_TIMEOUT for other git operations.
- Around line 134-148: Update the user-facing messages to refer to the new
"repo" model instead of "subscription": in the validation blocks where
is_valid_repo_name(raw_name) and the branch check append to summaries
(referencing variables raw_name, name, branch, and the summaries list), replace
"(skipped - invalid subscription name)" with "(skipped - invalid repo name)" and
"(skipped - invalid subscription config)" with "(skipped - invalid repo
config)"; also update the final repo_path check that currently appends "(skipped
- invalid subscription name)" to use "(skipped - invalid repo name)" so
messaging is consistent with the unified repos: model.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: aa227a77-b02d-442e-8021-a73e4b53747e

📥 Commits

Reviewing files that changed from the base of the PR and between 85b16bf and d43d6c1.

📒 Files selected for processing (11)
  • platform-integrations/claude/plugins/evolve-lite/README.md
  • platform-integrations/claude/plugins/evolve-lite/lib/config.py
  • platform-integrations/claude/plugins/evolve-lite/skills/publish/SKILL.md
  • platform-integrations/claude/plugins/evolve-lite/skills/publish/scripts/publish.py
  • platform-integrations/claude/plugins/evolve-lite/skills/sync/SKILL.md
  • platform-integrations/claude/plugins/evolve-lite/skills/sync/scripts/sync.py
  • platform-integrations/codex/plugins/evolve-lite/skills/publish/SKILL.md
  • platform-integrations/codex/plugins/evolve-lite/skills/publish/scripts/publish.py
  • platform-integrations/codex/plugins/evolve-lite/skills/sync/scripts/sync.py
  • tests/platform_integrations/test_config.py
  • tests/platform_integrations/test_publish.py
✅ Files skipped from review due to trivial changes (2)
  • platform-integrations/claude/plugins/evolve-lite/lib/config.py
  • platform-integrations/claude/plugins/evolve-lite/skills/publish/scripts/publish.py
🚧 Files skipped from review as they are similar to previous changes (5)
  • platform-integrations/claude/plugins/evolve-lite/skills/sync/SKILL.md
  • tests/platform_integrations/test_publish.py
  • platform-integrations/claude/plugins/evolve-lite/README.md
  • platform-integrations/claude/plugins/evolve-lite/skills/sync/scripts/sync.py
  • platform-integrations/codex/plugins/evolve-lite/skills/publish/scripts/publish.py

Apply the same `--` end-of-options fix to the Step 5 clone snippet in
claude publish SKILL.md that the sync script already uses. Options now
precede positional args and `--` separates them so a remote value
beginning with `-` can't be parsed as a git option.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@platform-integrations/claude/plugins/evolve-lite/skills/publish/SKILL.md`:
- Around line 97-100: Replace the risky repository-wide staging step "git -C
\".evolve/entities/subscribed/{repo}\" add ." with a targeted staging command
that only adds the selected guideline files (expand {selected-guideline-paths}
to the concrete paths under guideline/ for the current selection) before
committing and pushing; update the SKILL.md lines around the commit/push
sequence (the git -C commands shown) to use "git -C
\".evolve/entities/subscribed/{repo}\" add {selected-guideline-paths}" so only
intended guideline files are staged and published.

In `@platform-integrations/codex/plugins/evolve-lite/skills/publish/SKILL.md`:
- Line 97: The doc uses the incorrect git working directory in the diff command
("git -C {repo} diff HEAD -- {file}") which doesn't match where repos are
actually cloned; update that command to use the actual clone location (replace
the {repo} path with .evolve/entities/subscribed/{repo} or a {clone_path}
placeholder consistent with other docs) so the referenced command (the "git -C
{repo} diff HEAD -- {file}" line) points at ".evolve/entities/subscribed/{repo}"
and will succeed during normal runs.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: aa3ef906-5266-429c-85ed-a8a7ffedc791

📥 Commits

Reviewing files that changed from the base of the PR and between d43d6c1 and 47efc90.

📒 Files selected for processing (2)
  • platform-integrations/claude/plugins/evolve-lite/skills/publish/SKILL.md
  • platform-integrations/codex/plugins/evolve-lite/skills/publish/SKILL.md

Comment on lines +97 to 100
git -C ".evolve/entities/subscribed/{repo}" add .
git -C ".evolve/entities/subscribed/{repo}" commit -m "[evolve] publish: {filenames_list}"
git -C ".evolve/entities/subscribed/{repo}" push origin "{branch}"
```
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Stage only selected guideline files, not the whole repo clone.

git add . can accidentally include unrelated local changes inside .evolve/entities/subscribed/{repo} and publish unintended content. Limit staging to the selected files you just published.

Suggested doc fix
-git -C ".evolve/entities/subscribed/{repo}" add .
+git -C ".evolve/entities/subscribed/{repo}" add -- {selected-guideline-paths}
 git -C ".evolve/entities/subscribed/{repo}" commit -m "[evolve] publish: {filenames_list}"
 git -C ".evolve/entities/subscribed/{repo}" push origin "{branch}"

(Where {selected-guideline-paths} should expand to the concrete guideline file paths under guideline/ for the current selection.)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@platform-integrations/claude/plugins/evolve-lite/skills/publish/SKILL.md`
around lines 97 - 100, Replace the risky repository-wide staging step "git -C
\".evolve/entities/subscribed/{repo}\" add ." with a targeted staging command
that only adds the selected guideline files (expand {selected-guideline-paths}
to the concrete paths under guideline/ for the current selection) before
committing and pushing; update the SKILL.md lines around the commit/push
sequence (the git -C commands shown) to use "git -C
\".evolve/entities/subscribed/{repo}\" add {selected-guideline-paths}" so only
intended guideline files are staged and published.

section under the commit sha is the **publish change** being
replayed (opposite of a regular merge). Write an
intent-preserving resolution; don't `git add` yet.
3. Show the user the diff (`git -C {repo} diff HEAD -- {file}`) per
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fix incorrect git -C path in conflict diff command

Line 97 uses git -C {repo} ..., but the clone location everywhere else is .evolve/entities/subscribed/{repo}. As written, this command can fail in normal runs.

Suggested doc fix
-  3. Show the user the diff (`git -C {repo} diff HEAD -- {file}`) per
+  3. Show the user the diff (`git -C ".evolve/entities/subscribed/{repo}" diff HEAD -- {file}`) per
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
3. Show the user the diff (`git -C {repo} diff HEAD -- {file}`) per
3. Show the user the diff (`git -C ".evolve/entities/subscribed/{repo}" diff HEAD -- {file}`) per
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@platform-integrations/codex/plugins/evolve-lite/skills/publish/SKILL.md` at
line 97, The doc uses the incorrect git working directory in the diff command
("git -C {repo} diff HEAD -- {file}") which doesn't match where repos are
actually cloned; update that command to use the actual clone location (replace
the {repo} path with .evolve/entities/subscribed/{repo} or a {clone_path}
placeholder consistent with other docs) so the referenced command (the "git -C
{repo} diff HEAD -- {file}" line) points at ".evolve/entities/subscribed/{repo}"
and will succeed during normal runs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory Sharing Repo

1 participant