fix(runtime): keep UI writes out of disposable checkout#1149
Conversation
Why: settings, governance, skill, thread, and invocation flows could persist the runtime worktree or its descendants as project roots. That let page-driven mutations dirty the disposable checkout and made a normal restart fail closed. Centralize the persistent-path boundary, migrate legacy runtime paths to the workspace, and reject governance self-bootstrap descendants.
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
![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: a1617d792f
ℹ️ 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".
| const runtimeRoot = await validateProjectPathDetailed(runtimeRootRaw, options); | ||
| if (!runtimeRoot.ok) { |
There was a problem hiding this comment.
When legacy allowlist mode is enabled (PROJECT_ALLOWED_ROOTS) and CAT_CAFE_RUNTIME_ROOT points outside that allowlist, an otherwise valid external project passes the target validation but then fails here with runtime_root_invalid before the code can determine that the target is not under the runtime root. That makes routes migrated from validateProjectPath to resolvePersistentProjectPath reject allowed external projects unless the internal disposable runtime directory is also added to the user's project allowlist; the runtime root should be canonicalized independently, or only make the request fail after the target is known to be inside it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8abbd2b. The disposable runtime checkout is now canonicalized with realpath plus directory validation independently of PROJECT_ALLOWED_ROOTS; user-selected targets and the persistent workspace still pass normal project policy. The regression reproduced runtime_root_invalid for an allowed external project and now passes.
Why: cloud review found that stored game sentinels could be mistaken for runtime descendants, while legacy allowlists could reject valid external projects solely because the internal runtime checkout was not allowlisted. Preserve known sentinels and canonicalize the internal runtime root independently from user project policy. [小太阳·砚砚/GPT-5.6 Sol🐾]
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8abbd2bbad
ℹ️ 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".
| let threads = migratedProjectPath | ||
| ? await threadStore.listByProject(userId, migratedProjectPath) | ||
| : await threadStore.list(userId); |
There was a problem hiding this comment.
When a user filters GET /api/threads?projectPath=<runtime path>, this remaps the requested path to the persistent workspace before calling listByProject, but both in-memory and Redis thread stores compare t.projectPath === projectPath exactly. Existing threads that still have the old runtime-root projectPath are therefore excluded before migrateRuntimeProjectPath() can update them, so they disappear from project-filtered views instead of being migrated. Fetching all threads first or querying both the migrated and original path would preserve those legacy threads.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8c858f2. Project filtering now queries both the legacy requested coordinate and the migrated persistent coordinate, deduplicates by thread id, then migrates returned threads. The regression starts with a legacy runtime-owned thread and makes the filtered request first.
| export const skillsWriteRoutes: FastifyPluginAsync<SkillsWriteRouteOptions> = async (app, opts) => { | ||
| const CAT_CAFE_SKILLS_SRC = opts.skillsSourceDir ?? resolveSkillsSourceDir(); | ||
| const skillsRepoRoot = dirname(CAT_CAFE_SKILLS_SRC); | ||
| const globalProjectRoot = await redirectRuntimeProjectPath(opts.mainProjectRoot ?? skillsRepoRoot); |
There was a problem hiding this comment.
This computes the persistent global root for skill writes, but the sync handlers below still call readMountRules(projectRoot, opts.mainProjectRoot ?? resolveMainRepoPath()), which points back at the runtime checkout when CAT_CAFE_RUNTIME_ROOT is active. Since the mount-rules route now writes defaultMountRules under the persistent root, /api/skills/sync and /api/skills/sync-skill can miss operator-configured default mount rules and reconcile using built-in/runtime defaults instead. Use globalProjectRoot as the main root for these reads.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8c858f2. Both sync and sync-skill now pass globalProjectRoot to readMountRules. A shared regression stores custom defaultMountRules only in the persistent workspace and verifies both routes mount through that persistent configuration.
Why: Cloud review exposed four remaining coordinate mismatches: allowlist policy ran before runtime remapping, internal governance roots were treated as user projects, filtered legacy threads disappeared before migration, and skill sync read defaults from the disposable checkout. Canonicalize internal roots separately, query both legacy and persistent thread coordinates, and use the persistent global root for mount rules.\n\nTests: API build plus project-path, threads-endpoint, and skills-route regressions.\n\n[小太阳·砚砚/GPT-5.6 Sol🐾]
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c858f2cd6
ℹ️ 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".
| const catCafeRoot = opts?.catCafeRoot ?? findMonorepoRoot(process.cwd()); | ||
| if (validated === catCafeRoot) { | ||
| reply.status(400); | ||
| return { error: 'Cannot setup governance for Clowder AI itself' }; | ||
| const validatedResult = await validateExternalProjectPathDetailed(projectPath, catCafeRoot); |
There was a problem hiding this comment.
In the default production registration, projectSetupRoute is registered without opts.catCafeRoot, and runtime-worktree starts run with process.cwd() inside CAT_CAFE_RUNTIME_ROOT. This leaves catCafeRoot pointing at the disposable checkout even though the target project is now validated through the persistent resolver; the same value is later passed to GovernanceBootstrapService, whose registry writes .cat-cafe/governance-registry.json under catCafeRoot. A UI /api/projects/setup call for any external project can therefore still dirty the runtime worktree and break the next daemon start; resolve/redirect this root to CAT_CAFE_WORKSPACE_ROOT before validation/bootstrap.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 02f7fce. projectSetupRoute now redirects the default or explicit Clowder AI root through the persistent boundary at plugin startup, and validation plus GovernanceBootstrapService share that resolved workspace coordinate. The regression runs default registration from a temporary runtime cwd and proves governance-registry.json is written only under the persistent workspace.
Why: Current-head cloud review found two remaining default-root paths that still pointed at the disposable runtime checkout. Resolve the Skills main-rule root and project-setup registry root through the persistent boundary at plugin startup so reads and governance bootstrap share the workspace coordinate.\n\nTests: API build plus Skills GET/default-mount and Projects Setup registry-location regressions.\n\n[小太阳·砚砚/GPT-5.6 Sol🐾]
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
What
Establish one persistent project-path boundary for settings, governance, Skill, MCP/mount, thread, proposal, and invocation flows:
Why
Page-driven mutations could treat the disposable runtime checkout, including
packages/api, as a persistent project. Governance bootstrap then created tracked files there, and a legacy thread path let later agent work add more tracked changes. The next normal daemon start correctly failed closed on the dirty runtime worktree.Issue Closure
Original Requirements
Plan / ADR
Tips Contribution
tips_exempt:runtime safety bugfix; no new user action or discoverable capabilityTradeoff
The patch does not move, delete, stash, or overwrite existing dirty files because their keep/discard intent is unknown. It prevents new runtime writes and migrates stored path references. Missing external paths unrelated to runtime retain their legacy behavior.
Test Evidence
packages/api public test suite— 16,586 tests; 16,558 pass; 0 fail; 28 skippnpm check— exit 0pnpm lint— exit 0; baseline warnings onlypnpm -r run build— exit 0git diff --check— exit 0a1617d792f251794dc453601315ea4a92ddc7441Open Questions
本地 Review: [x] sonnet independently reviewed and approved exact HEAD
云端 Review: [ ] trigger after PR creation