Repository navigation
fix(publish): follow-ups from the #381 review - #421
Conversation
A symlinked entity file under entities/<type>/ that points outside its type dir must not be published: publish unlinks the resolved source, so it would delete data outside .evolve. Removing `not path.is_relative_to(type_dir)` left every publish test green; this test now fails without it. Refs #419
…tized match With entities/my_notes/x.md and entities/my-notes/x.md both present, every invocation reported x.md as ambiguous, including --type my_notes and --type my-notes, because both dirs sanitize to my-notes. An exact dir name now wins; the sanitized match is the fallback, and the requested value must still pass the sanitize_type validity check first. Refs #419
…e same name adapt-memory names entities after the native memory, so two teammates' user-role memories both map to <clone>/user/user-role.md. When the second one published, publish said "Unpublish it first or delete it manually", telling them to delete a teammate's entity. When the existing file's owner differs from the publishing user, publish now reports a name collision with that owner and asks the user to rename their memory or entity. The old message stays when the owner is the same or absent (see #400 item 1 for updating an entity you already published). Refs #419
…te clone dest_dir is <clone>/<type>, and the containment check compared dest_path against the resolved dest_dir, so a symlink committed into the shared repo (e.g. guideline -> ../../../../outside) redirected the write: publish wrote outside/x.md, removed the private copy and reported success. publish now refuses when the type dir in the clone is a symlink, and when it resolves outside the resolved clone root. A clone root that is itself a symlink still works. Refs #419
Publish wrote the shared copy and then unlinked the private one. When the unlink failed (e.g. a chmod 555 type dir), the result was a PermissionError traceback, two copies, no audit entry, and "already published" on every retry. publish now checks that the private copy's directory is writable before writing anything and refuses with a clear message if not. If the unlink still fails after the write, it keeps the published copy, names the leftover private copy to delete, and records the publish in the audit log with source_removed: false. Refs #419
The manual learn flow stores save_trajectory's resolved path in each entity's trajectory field, so publish shipped the publisher's home directory, username and project path to every subscriber (it already strips native_path). An absolute trajectory inside the project is now published relative to it (.evolve/trajectories/<file>, the form the stop-hook flow already writes); one outside the project is dropped, since it has no meaningful relative form. Relative values are unchanged. Refs #419
…sitive filesystems entities/feedback -> SUBSCRIBED/read/feedback names the subscribed clone on a case-insensitive filesystem, but resolve() keeps the spelling, so a string-prefix comparison would let it through. The identity check (_inside_any) refuses it; this test pins that and is skipped on case-sensitive filesystems. Refs #419
Publish handles every entity type since #381, but the Codex default prompt still said "Publish one of my Evolve guidelines", the Claude, Codex and Bob READMEs had a "Publishing Guidelines" heading, and the Claude, Bob and claw-code config examples said the identity stamps ownership on "published guidelines". Say entities instead, and re-render the platform copies. Refs #419
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 64.44% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 45 functions across 6 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title accurately identifies publish fixes resulting from the #381review. It is concise and relevant to the main changes.Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
jayaramkr
left a comment
jayaramkr
left a comment
There was a problem hiding this comment.
Verdict: approve. Reviewed at 9d3ad45. I ran everything below at 03d1c19. The commits since then only merge main (#360 in experiments/, #398 in deps and hooks tests), and test_publish.py (66 passed) and build_plugins.py check pass again at 9d3ad45.
All seven of the #419 items this PR claims (1–7) are fixed, and each fix has a test that fails on the commit before it. I found nothing new at medium or above. The low-severity leftovers are filed as #419 items 24–28, so none of them needs to land here.
Baseline: ruff, mypy and build_plugins.py check are clean. The platform tier is 723 passed at head and 704 at base. test_publish.py goes from 47 to 66, with nothing skipped on macOS. All 5 copies of publish.py are byte-identical.
#419 items 1–7
| Item | Status |
|---|---|
| 1. Name collision | Fixed. Owner alice with --user bob gives "name collision … by 'alice'". The same owner or no owner keeps the old hint. Binary, unreadable or directory destinations fall back cleanly, with no traceback. |
2. --type exact match first |
Fixed. With my_notes/ next to my-notes/, each name publishes from its own dir. No flag, or MY_NOTES, is still ambiguous. |
| 3. Symlinked type dir in the clone | Fixed. Symlinks pointing outside the clone, inside it, or nowhere are all refused, and nothing is written. On base these wrote outside the clone, wrote into another dir, and crashed, respectively. A symlinked clone root still publishes. |
| 4. Private copy can't be removed | Fixed. A chmod 555 type dir is refused before anything is created. A forced unlink failure gives exit 0, a warning on stderr, and source_removed: false in the audit. No false refusals for a read-only file, a sticky dir, or a symlinked writable dir. |
5. Absolute trajectory |
Fixed. An absolute path inside the project becomes .evolve/trajectories/<file>, including through /tmp vs /private/tmp, .. and a symlinked project dir. A path outside the project is dropped, and a relative one is unchanged. |
| 6a / 6b. Missing tests | Added, and each catches its regression. 6b runs on macOS only; see #419 item 26. |
| 7. Leftover wording | Fixed. No #publishing-guidelines links are left dangling. |
New, non-blocking (all in #419)
- Low, pre-existing: a committed symlink file in the write clone still redirects the write. (inline,
publish.py:237; #419 item 24) - Low, test quality:
assert "symlink" in result.stderrmatches the pytest temp-dir name. (inline,test_publish.py:207; #419 item 25) - Low, CI gap: the case-variant test skips on case-sensitive filesystems, so Linux CI never runs it, and on macOS it's the only thing catching a regression to string comparison (#419 item 26).
- Low, fails safe: with an
EVOLVE_DIRnot named.evolve, the trajectory link becomestrajectories/<file>, and a path under<proj>/.evolve/is dropped. Removing therealpathcall breaks no test (#419 item 27). - Nit: the owner comparison doesn't normalise
--user:" alice"andAliceboth count as different fromalice(#419 item 28).
Also verified
- No path prints "Published:" and then exits non-zero, and none exits 0 without publishing. The only success-with-warning case is the intended unlink failure.
- The unlink-failure test patches
Path.unlinkinside the subprocess, so it works the same as root. Thechmodtest skips under root and restores permissions in afinally. - The tests run the real script, with no over-mocking. 15 of 17 mutations are caught. The two survivors are the
realpathgap (item 27) and theis_relative_to(clone_root)guard, which is redundant once the symlink check has run. - The PR body's claims hold: each fix's new tests fail on its parent commit, and the render check catches hand-edited drift in rendered copies.
Thanks @gaodan-fang. This closes out the publish half of #419 cleanly.
| if not dest_base.is_relative_to(clone_root.resolve()): | ||
| print(f"Error: refusing to publish into {dest_dir}: it resolves outside the target repo clone.", file=sys.stderr) | ||
| sys.exit(1) | ||
| dest_path = (dest_dir / args.entity).resolve() |
There was a problem hiding this comment.
Low, pre-existing: item 3 covers a symlinked type dir, but a symlinked entity file in the clone still redirects the write.
If a teammate commits guideline/g.md -> y.md (dangling), publishing g.md writes y.md, prints Published: g.md -> …/guideline/y.md with exit 0, and deletes the private g.md. So the entity ends up under the wrong name. Base behaves the same.
The write can't leave the type dir: a target outside it is refused, though with the misleading invalid entity name: 'g.md'. With g.md -> nested/x.md, both base and head crash with a FileNotFoundError traceback. And with g.md -> z.md where z.md has owner alice, the new collision message names g.md but shows z.md's path.
Refusing when dest_dir / args.entity is a symlink, as item 3 does for the dir, would close it. Tracked as #419 item 24.
| result = run_publish(project_dir, ["--entity", "my-guideline.md"], expect_success=False) | ||
|
|
||
| assert result.returncode != 0 | ||
| assert "symlink" in result.stderr |
There was a problem hiding this comment.
Low (test quality): this assertion passes for the wrong reason in the [outside] case.
I removed the dest_dir.is_symlink() refusal, and [outside] still passed. The "resolves outside the target repo clone" guard refuses that case, and "symlink" matches the pytest temp-dir name in the path (…/test_refuses_a_symlinked_type_0/…), not the error. Only [inside] tests the refusal. Asserting on "is a symlink in the target repo clone" would fix it. Tracked as #419 item 25.
2 participants
Follow-up to #381. Fixes the three low-severity comments @jayaramkr left in the #381 approval, plus the rest of #419, which collects them with four more. One commit per item.
publish.pyis edited inplugin-source/, and the five platform copies are re-rendered withbuild_plugins.py render, socheckpasses.Fixes items 1–7 of #419 (the publish items). Items 8–14, the segmentation follow-ups from the #382 review, are handled separately.
Changes
#419 item 6a: test for a symlinked entity file pointing outside its type dir (review comment on line 96)
ln -s ../../../outside/native.md .evolve/entities/feedback/note.md, removingnot path.is_relative_to(type_dir)kept every publish test green. A real run then published the file and deletedoutside/native.md.--type. It asserts a non-zero exit, that the outside target is unchanged, that the symlink is still in place, and that nothing was written to the clone.assert result.returncode != 0(0 != 0; publish succeeded).#419 item 2:
--typepicks the exact dir name first (review comment on line 87)my_notes/x.mdandmy-notes/x.mdpresent, no flag,--type my_notesand--type my-notesall reportedambiguous.sanitize_typevalidity check first.--type My_Notes, are still ambiguous.#419 item 1: name collision with another owner (review comment on line 182)
user/user-role.mdand Alice's copy already existed:owneris the same or absent. Updating your own published copy is publish: update/unpublish for mirrored memories, provenance lookup and user/-type semantics after publishing #400 item 1.--as <name>rename option, which Tech debt from #381, #382, #383, #384, #385, #388, #398 and #421 reviews #419 floats as "consider". It changes the publish CLI and the skill flow, and it fits better with publish: update/unpublish for mirrored memories, provenance lookup and user/-type semantics after publishing #400's update/unpublish design.#419 item 3: symlinked type dir in the write clone
guideline -> ../../../../../outcommitted into the clone,Published: x.md -> <w>/out/x.md. That's exit 0, and the private copy was removed.Error: refusing to publish into .evolve/entities/subscribed/memory/guideline: it is a symlink in the target repo clone.That's exit 1, and nothing is written. publish also refuses a type dir that resolves outsideclone_root.resolve().#419 item 4: the private copy can't be removed
chmod 555, publish raised aPermissionErrortraceback and left both copies. There was no audit entry, and every retry saidalready published.source_removed: false.chmod 555pre-check: nothing is written and there's no audit entry;#419 item 5: absolute
trajectorypathtrajectory: /home/<user>/<project>/.evolve/trajectories/...was published as-is..evolve/trajectories/<file>). One outside the project is dropped. Relative values are unchanged.#419 item 6b: differently-cased symlink target
entities/feedback -> SUBSCRIBED/read/feedback, with and without--typeandEVOLVE_DIR.SUBSCRIBED -> subscribed. It couldn't be mutation-tested here.#419 item 7: wording
plugin.toml: "Publish one of my Evolve entities."plugin.json, were regenerated.Checks
tests/platform_integrations/test_publish.py: 65 passed, 1 skipped (item 6b on Linux).-m platform_integrations): 719 passed, 1 skipped.ruff format --check,ruff checkandmypy .: clean.build_plugins.py check: clean.Summary by CodeRabbit