Fix: PLAIN_TEMP_PATH is cwd-relative, so per-checkout facts can only be fixed inside plain-dev - #121
Open
pullapprove5[bot] wants to merge 3 commits into
Open
Fix: PLAIN_TEMP_PATH is cwd-relative, so per-checkout facts can only be fixed inside plain-dev#121pullapprove5[bot] wants to merge 3 commits into
pullapprove5[bot] wants to merge 3 commits into
Conversation
added 3 commits
September 5, 2026 22:01
![pullapprove5-fix[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…be fixed inside plain-dev Confirmed the finding: PLAIN_TEMP_PATH (plain/plain/runtime/__init__.py) was `Path.cwd() / ".plain"`, purely cwd-relative, while plain-dev's pidfile_path (plain-dev/plain/dev/process.py) was already project-root-keyed via find_project_root+checkout_state_path. I reproduced all three consequences directly: (b) OxcTool.get_version_from_config() returned "" from a subdirectory even with a pyproject.toml one level up (os.path.dirname(PLAIN_TEMP_PATH) == cwd, not the real root); (c) DevSupervisor.log_dir (cwd-keyed) and DevSupervisor.pidfile_path() (project-root-keyed) resolved to two completely different trees when cwd was a subdirectory; (a) plain-portal's socket/lock lived under .plain/portal, which — unlike the project-root-keyed pidfile — collides if two checkouts share one .plain (e.g. via symlink), since it's an artifact path rather than a fact path. Fix: made PLAIN_TEMP_PATH computed via a new `find_project_root()` walk (same logic plain-dev already had) instead of raw cwd, and lifted `find_project_root`, `checkout_id`, and `checkout_state_path` into plain.runtime so any package can use the "facts vs artifacts" split, not just plain-dev. plain-dev/plain/dev/state.py now re-exports these three from plain.runtime instead of defining its own copies (sanitize/short_digest stay local — still used elsewhere for DB naming). plain-portal/plain/portal/local.py now derives its socket/lock directory from checkout_state_path(find_project_root(...)) instead of PLAIN_TEMP_PATH, so it can't collide across checkouts that share a .plain. Also updated plain-dev/tests/conftest.py's isolated_checkout_state fixture, which monkeypatched `plain.dev.state.PLAIN_CACHE_PATH` — that attribute no longer exists there since checkout_state_path moved, so it now patches `plain.runtime.PLAIN_CACHE_PATH` (the actual global the function reads). Verified: oxc/tailwind need no changes (they derive project root from os.path.dirname(PLAIN_TEMP_PATH), which is now correct automatically), and plain-dev's log_dir/other PLAIN_TEMP_PATH consumers likewise benefit without further edits. No uv/postgres/docker preinstalled in this sandbox — bootstrapped by `pip install uv`, `uv sync`, and `pip install 'psycopg[binary]'` into the workspace venv (package registries reachable, matches sandbox network rules). Real Postgres (docker or local server) was not available, so DB-backed tests (plain-postgres, the example app's own test suite) could not run; everything else did.
Reproduced and fixed all three findings on top of PR #121's PLAIN_TEMP_PATH/facts-vs-artifacts fix. (1) & (2) — missing minimum plain versions: `plain.dev.state` and `plain.portal.local` both import `checkout_id`/`checkout_state_path`/`find_project_root` from `plain.runtime`, but these names were added to `plain.runtime` only in the still-unreleased parent commit (25dde35) — every previously released `plain`, including the currently-published 0.163.0, lacks them. plain-dev's floor was `plain>=0.161.0` and plain-portal's had no floor at all (`plain<1.0.0`), so both packages declared themselves compatible with releases that would ImportError. I confirmed this by reverting `plain/plain/runtime/__init__.py` to its pre-fix (0.163.0-era) content and importing `plain.dev.state` / calling `plain.portal.local._portal_dir()` — both raised `ImportError: cannot import name '...' from 'plain.runtime'`. Fixed by bumping both packages' dependency floor to `plain>=0.164.0` (the next plain release, which will carry these APIs — confirmed via the repo's changelog that each release increments the middle version number by one, and via `packaging.specifiers` that the new constraint excludes 0.163.0 and admits 0.164.0). Verified with `uv sync` that the workspace still resolves fine locally (workspace members bypass the version specifier for path-based dev resolution) and that both packages' full test suites still pass. (3) — Unix socket path length: `_portal_dir()` built the socket/lock directory via `checkout_state_path()`, which embeds the checkout's full sanitized directory name plus an 8-char digest under `PLAIN_CACHE_PATH`. I reproduced this directly: for a 65-character checkout basename, the computed socket path was 127 bytes — over both the ~104-byte (macOS) and ~108-byte (Linux) `sockaddr_un` limit that `asyncio.start_unix_server()` enforces — where the pre-PA-11 path (`<project>/.plain/portal/portal.sock`) would have been shorter for the same checkout since it only grew with the project's *location*, not its *name*. Fixed by keying the portal directory on a hash of `checkout_id()` alone (dropping the human-readable name component, which sockets don't need since they aren't inspected by hand), so the path length is now constant regardless of checkout name. Added `TestPortalDir` to `plain-portal/tests/public/test_portal.py` pinning that the directory length doesn't grow with checkout name and that different checkouts still get different directories. Ran `./scripts/fix` (clean). Could not run the full `./scripts/test` suite — no Postgres/Docker available in this sandbox (matching the parent PR's own note), so DB-backed suites (plain-postgres, the example app) couldn't execute. Ran the plain-dev and plain-portal package test suites directly via `uv run --isolated python -m pytest` (bootstrapping `uv` via `pip install uv` and `psycopg[binary]` the same way the parent PR did) — both fully pass, including the 2 new tests, with no DB dependency in either suite. Three commits, one per finding: 26a009dc0b (plain-dev floor), f18e75048e (plain-portal floor), 72ec621d58 (socket path fix + test).
The finding targets plain-portal/plain/portal/local.py's _portal_dir(), which builds the Unix-socket directory for the portal's local connect process. A previous implement-finding run (now HEAD~0's parent, commit 2c9ecd7) had already fixed the earlier version of this bug — where a long checkout directory name pushed the socket path over the kernel's sockaddr_un limit — by hashing checkout_id() into a fixed 16-char digest instead of using checkout_state_path()'s human-readable directory name. But that fix still joined the digest onto PLAIN_CACHE_PATH (os.path.join(PLAIN_CACHE_PATH, "portal", digest)), and PLAIN_CACHE_PATH is itself user-configurable via the PLAIN_CACHE_PATH env var or XDG_CACHE_HOME. I reproduced the finding directly: setting PLAIN_CACHE_PATH to a 77-character path (plausible for a deep CI workspace or XDG_CACHE_HOME) produced a 113-byte socket path, and asyncio.start_unix_server failed with "AF_UNIX path too long" — matching the finding's stated 68-72+ char threshold almost exactly. Fixed by rooting the portal directory at tempfile.gettempdir() instead of PLAIN_CACHE_PATH, since the socket/lock are ephemeral facts about a live process rather than cache artifacts and have no reason to depend on cache-path configuration; the system temp dir is short, OS-controlled, and the conventional home for this kind of IPC socket (same as ssh-agent, X11, etc.). Added a test (test_socket_path_length_does_not_grow_with_cache_path) mirroring the existing checkout-name-length test, confirming the socket dir length no longer grows with PLAIN_CACHE_PATH. Ran ./scripts/fix (clean) and the plain-portal package suite directly (uv run --isolated --package plain-portal python -m pytest from plain-portal/tests, since Postgres/Docker aren't available in this sandbox, matching both prior runs' notes) — all 67 tests pass, including the 2 length-pinning tests. Did not re-run Postgres-backed suites (plain-postgres, example app) for the same reason as the two prior runs on this PR. Committed as 01bc847ee8, one commit for the one finding in scope.
Author
PENDING: 1 review scope pending, 1 agent approved
✅ codex approved Next steps:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Confirmed the finding: PLAIN_TEMP_PATH (plain/plain/runtime/init.py) was
Path.cwd() / ".plain", purely cwd-relative, while plain-dev's pidfile_path (plain-dev/plain/dev/process.py) was already project-root-keyed via find_project_root+checkout_state_path. I reproduced all three consequences directly: (b) OxcTool.get_version_from_config() returned "" from a subdirectory even with a pyproject.toml one level up (os.path.dirname(PLAIN_TEMP_PATH) == cwd, not the real root); (c) DevSupervisor.log_dir (cwd-keyed) and DevSupervisor.pidfile_path() (project-root-keyed) resolved to two completely different trees when cwd was a subdirectory; (a) plain-portal's socket/lock lived under .plain/portal, which — unlike the project-root-keyed pidfile — collides if two checkouts share one .plain (e.g. via symlink), since it's an artifact path rather than a fact path.Fix: made PLAIN_TEMP_PATH computed via a new
find_project_root()walk (same logic plain-dev already had) instead of raw cwd, and liftedfind_project_root,checkout_id, andcheckout_state_pathinto plain.runtime so any package can use the "facts vs artifacts" split, not just plain-dev. plain-dev/plain/dev/state.py now re-exports these three from plain.runtime instead of defining its own copies (sanitize/short_digest stay local — still used elsewhere for DB naming). plain-portal/plain/portal/local.py now derives its socket/lock directory from checkout_state_path(find_project_root(...)) instead of PLAIN_TEMP_PATH, so it can't collide across checkouts that share a .plain. Also updated plain-dev/tests/conftest.py's isolated_checkout_state fixture, which monkeypatchedplain.dev.state.PLAIN_CACHE_PATH— that attribute no longer exists there since checkout_state_path moved, so it now patchesplain.runtime.PLAIN_CACHE_PATH(the actual global the function reads).Verified: oxc/tailwind need no changes (they derive project root from os.path.dirname(PLAIN_TEMP_PATH), which is now correct automatically), and plain-dev's log_dir/other PLAIN_TEMP_PATH consumers likewise benefit without further edits.
No uv/postgres/docker preinstalled in this sandbox — bootstrapped by
pip install uv,uv sync, andpip install 'psycopg[binary]'into the workspace venv (package registries reachable, matches sandbox network rules). Real Postgres (docker or local server) was not available, so DB-backed tests (plain-postgres, the example app's own test suite) could not run; everything else did.Opened by a PullApprove implementation run (implement-finding v4) for:
Merging this is what closes them as fixed.