fix(core): keep projects under a shared root top-level - #1598
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8aa632d212
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # it with the project, would then reach into the nested one. | ||
| # Outcome: projects under a shared root stay one directory deep. Local | ||
| # projects choose their own paths and may still use '/' in their names. | ||
| if top_level and "/" in permalink: |
There was a problem hiding this comment.
When the project name is . (or otherwise normalizes to .), this check accepts it because the permalink contains no slash. With a configured project root, (base_path / ".").resolve() is the shared root itself, so the project does not own a top-level child directory; removing it with delete_notes=True recursively deletes the shared root and any canonical note content stored there. Reject . and .., or validate that the resolved project path is a strict direct child of the root.
AGENTS.md reference: AGENTS.md:L156-L160
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in e0c9830. generate_permalink keeps periods and only strips hyphens from segment edges, so . and .. passed the empty-segment check, and under a project root root / "." resolves to the root itself. Every segment now has to contain something other than dots and hyphens, which is the rule the existing error message already stated. ., .. and a/.. are added to the rejection test, and reverting the check fails exactly those three cases. This predates this PR, but it belongs with the top-level rule.
|
|
||
| # If project_root is set, constrain all projects to that directory | ||
| project_root = self.config_manager.config.project_root | ||
| name_permalink = project_permalink(name, top_level=project_root is not None) |
There was a problem hiding this comment.
When BASIC_MEMORY_PROJECT_ROOT is present but empty, the config value is "": this passes top_level=True here, while the subsequent if project_root branch treats the root as disabled and uses the caller's ordinary local path. Such local projects therefore unexpectedly reject names like Research/2026 even though no shared-root layout is active. Pass top_level=bool(project_root) so validation matches the path-selection logic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in e0c9830. project_root is Optional[str] with no validator, so an empty env var gives "", which path selection treats as no root. top_level=bool(project_root) now matches it. The new test adds a local Research/2026 project with BASIC_MEMORY_PROJECT_ROOT="" and fails with the old is not None.
Pull the project-name rules out of ProjectService.add_project into project_permalink(name, *, top_level), so every runtime that creates projects applies the same rules. Cloud inserts projects through its own race-safe INSERT ... ON CONFLICT path and could not reuse add_project, so it had silently dropped these rules. The new rule: under a shared root (a configured project root, and Cloud's tenant bucket) a name whose permalink contains '/' is refused. The permalink becomes the project's directory, so 'Research/2026' would live inside 'Research', and anything that treats a project directory as the project's own, such as deleting it with the project, would reach into the nested one. Local projects choose their own paths and keep accepting '/' in names. Refs basicmachines-co/basic-memory-cloud#2102 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015y5WHPT9CNQp1RAttVFuFT Signed-off-by: phernandez <paul@basicmachines.co>
Two review findings on project_permalink: - Periods survive generate_permalink, so '.' and '..' passed the empty-segment check. As a directory under a project root they name the root itself or its parent, and deleting that project's files would delete the root. Every segment must now contain something other than dots and hyphens, which is the rule the error message already stated. - An empty BASIC_MEMORY_PROJECT_ROOT is no root for path selection, but was passed as top_level=True. Use bool(project_root) so local projects keep '/' names. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015y5WHPT9CNQp1RAttVFuFT Signed-off-by: phernandez <paul@basicmachines.co>
e0c9830 to
aaf0721
Compare
1 participant
Why
Cloud stores each project under its own top-level prefix in the tenant bucket, and deletes a project by purging that prefix. The prefix is
generate_permalink(name), andgenerate_permalinkkeeps/. So a project namedResearch/2026would sit insideresearch/, and purging "Research" would delete it too.Core already prevents overlapping project directories:
add_projectrefuses nested paths. Cloud can't calladd_project, though. It needs a race-safeINSERT ... ON CONFLICT, an inactive insert for invite-only projects, a caller-supplied external ID for keyed retries, the OKF seed in the same transaction, and no localconfig.jsonwrites (see basic-memory-cloud#1763). So Cloud copied the insert and dropped the rules.What changed
project_permalink(name, *, top_level)inservices/project_service.pyreturns the permalink that addresses a new project, or raisesValueError.top_level=True, a permalink containing/is refused.add_projectcalls it withtop_level=set by whether a project root is configured. UnderBASIC_MEMORY_PROJECT_ROOTthe permalink is already the project's directory name, so this mode was meant to be flat ("ensures flat structure"). Local projects without a project root still accept/in names.Testing
project_permalink: a single-segment name passes top-level, a slash name passes locally, and a slash name is refused top-level.add_projectunder a realBASIC_MEMORY_PROJECT_ROOTrefusesResearch/2026and creates nothing.just fast-checkpasses. Project service and API project tests: 123 passed. Project-root, index, and CLI suites: 1182 passed.🤖 Generated with Claude Code
https://claude.ai/code/session_015y5WHPT9CNQp1RAttVFuFT