refactor: adopt canonical Remote Dev data layout - #72
Conversation
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
Summary by CodeRabbit
WalkthroughThe change adopts a role-neutral ChangesCanonical runtime layout
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 review |
✅ Action performedReview finished.
|
![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: ca55f59513
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/releases.md`:
- Around line 69-76: Align all three documentation statements with the
implemented authentication behavior: in docs/releases.md lines 69-76, qualify
the no-mount check to allow the optional authentication-secret mount; in
CHANGELOG.md lines 15-16, scope the authentication guard to agent terminals; and
in CHANGELOG.md lines 48-50, state that the launcher receives no agent password
by default while permitting the documented launcher-auth exception.
In `@scripts/test_canonical_data_layout.py`:
- Around line 154-168: Update validate_repository_has_no_legacy_data_root to
scan only files tracked by Git instead of every path under ROOT.rglob("*"),
while preserving the existing binary-file filtering and legacy-variable/path
assertions for each readable tracked file.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 27b4a576-d750-426f-9c63-cf5742d2a972
📒 Files selected for processing (14)
.env.example.github/workflows/build-amd64.ymlCHANGELOG.mdMakefilePROJECT_STATUS.mdREADME.es.mdREADME.mdcompose/docker-compose.ymlcompose/truenas.ymldocs/architecture.mddocs/releases.mddocs/security.mdscripts/test_canonical_data_layout.pyscripts/test_single_stack_compose.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: Review GitHub Actions for least-privilege permissions, safe event triggers, untrusted input handling,
accidental publication, tag-channel correctness, secret exposure, reproducible builds, SBOM/provenance
generation and appropriate validation before pushing images.
Files:
.github/workflows/build-amd64.yml
**/*.{yml,yaml,Dockerfile,sh,bash,js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{yml,yaml,Dockerfile,sh,bash,js,ts,tsx}: Never use privileged containers, Docker or Podman sockets, SYS_ADMIN, host-root mounts, or broad persistence mounts such as /root, /home, /opt, /usr/local, or a parent data root.
Preserve mandatory ttyd authentication for agent terminals, origin checking for all web endpoints, tmux reconnect behavior, image identity checks, and existing Codex login, start, and resume behavior.
Files:
compose/truenas.ymlcompose/docker-compose.yml
**/*.{yml,yaml,js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{yml,yaml,js,ts,tsx}: The launcher role is navigation only: it may link only to fixed, validated stack services and must not execute agents or relay/proxy agent terminal HTTP or WebSocket traffic without an explicit threat-model review.
Keep the unauthenticated private-network default, optional launcher authentication, origin checks, CSP, method restrictions, and secret-free health behavior covered by tests.
Files:
compose/truenas.ymlcompose/docker-compose.yml
**/*.{yml,yaml,html,js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never embed credentials in launcher URLs, HTML, JavaScript, logs, or diagnostics.
Files:
compose/truenas.ymlcompose/docker-compose.yml
**/*.{yml,yaml}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{yml,yaml}: By default, the launcher receives only web/routing configuration and no password secret; it must never receive agent workspaces, agent state, GitHub/Git/SSH mounts, agent secrets, or the Docker socket.
Launcher and agent services must use the same final image reference or digest while retaining separate container roles and state boundaries.
Files:
compose/truenas.ymlcompose/docker-compose.yml
**/*.{yml,yaml,sh,bash}
📄 CodeRabbit inference engine (AGENTS.md)
Implement issue
#25in separately reviewed slices: role-neutral commands and validation, configurable Codex approval modes, canonical image and variable naming with time-bounded aliases, shared image digest, Compose and state migration, and outer hardening with cross-service canaries.
Files:
compose/truenas.ymlcompose/docker-compose.yml
**/*.{sh,bash,js,ts,tsx,yml,yaml}
📄 CodeRabbit inference engine (AGENTS.md)
Run the narrowest relevant tests during development and complete required CI before merge; preserve or extend coverage for role/start-mode validation, wrappers, launcher security, mounts, image identity, health checks, Codex flows, credential hardening, ttyd, tmux, provenance, notices, SBOM, Trivy, and the critical-vulnerability gate.
Files:
compose/truenas.ymlcompose/docker-compose.yml
compose/**
⚙️ CodeRabbit configuration file
compose/**: Review Docker Compose files for TrueNAS and generic Docker compatibility.
Flag privileged mode, host networking, Docker socket mounts, overly broad host mounts,
missing authentication, unsafe public bindings and persistence paths that could lose credentials or workspaces.
Require launcher and agent services to reference one image while keeping agent state out of the launcher.
Files:
compose/truenas.ymlcompose/docker-compose.yml
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Check that documentation matches the implemented behavior and clearly distinguishes experimental edge builds
from stable releases. Flag outdated versions, unsafe deployment guidance and claims not proven by CI or testing.
Files:
README.mddocs/security.mdPROJECT_STATUS.mdCHANGELOG.mddocs/architecture.mddocs/releases.mdREADME.es.md
scripts/test_single_stack_compose.py
⚙️ CodeRabbit configuration file
scripts/test_single_stack_compose.py: Check that topology assertions are deterministic and really prove one image reference, fixed roles,
no launcher access to agent state, no Docker socket/privilege escalation and preserved Codex compatibility.
Files:
scripts/test_single_stack_compose.py
🪛 ast-grep (0.45.0)
scripts/test_canonical_data_layout.py
[error] 41-59: Command coming from incoming request
Context: subprocess.run(
[
"docker",
"compose",
"--env-file",
empty_env.name,
"-f",
str(path),
"config",
"--format",
"json",
],
cwd=ROOT,
env=compose_environment(),
text=True,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
scripts/test_single_stack_compose.py
[info] 176-176: use jsonify instead of json.dumps for JSON output
Context: json.dumps(launcher, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 5-5: [UnorderedKey] The REMOTE_DEV_DATA_ROOT key should go before the REMOTE_DEV_IMAGE key
(UnorderedKey)
🪛 LanguageTool
docs/security.md
[style] ~69-~69: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tests or rendered environment output. - Never mount agent data or credentials into th...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~70-~70: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ata or credentials into the launcher. - Never mount Docker or Podman sockets. - Never...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~71-~71: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...Never mount Docker or Podman sockets. - Never use privileged: true, host PID, host ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~72-~72: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ost networking or added capabilities. - Never mount the parent data root, host root, ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
CHANGELOG.md
[style] ~39-~39: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... around role-neutral implementations. - Changed the normal TrueNAS portal from the Code...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~40-~40: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ing independent Codex authentication. - Changed the stateless launcher to require no pa...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 Ruff (0.16.0)
scripts/test_canonical_data_layout.py
[warning] 27-27: Boolean-typed positional argument in function definition
(FBT001)
[warning] 42-60: Prefer capture_output over sending stdout and stderr to PIPE
Replace with capture_output keyword argument
(UP022)
[error] 42-42: subprocess call: check for execution of untrusted input
(S603)
[error] 43-53: Starting a process with a partial executable path
(S607)
[warning] 62-62: Avoid specifying long messages outside the exception class
(TRY003)
scripts/test_single_stack_compose.py
[error] 260-260: Possible hardcoded password assigned to: "WEB_PASSWORD_FILE"
(S105)
🔇 Additional comments (12)
docs/security.md (1)
3-82: LGTM!Also applies to: 91-106
docs/releases.md (1)
7-41: LGTM!Also applies to: 54-56, 58-68, 77-81, 83-93
.env.example (1)
4-6: LGTM!compose/docker-compose.yml (2)
121-121: LGTM!
84-108: 🗄️ Data Integrity & Integration | 🏗️ Heavy liftConfirm
create_host_path: falseis actually enforced by the Compose version in use; the whole "fail instead of silently create" contract depends on it.Docker Compose has an open, currently-unresolved issue (docker/compose#13602) where
bind.create_host_path: falseis kept in the rendered config for the affected mount, so the value appears to be parsed but not enforced at runtime. The Compose spec itself documents that the option "Creates a directory at the source path on host if there is nothing present" and "Defaults to true", confirming that explicitfalseis required to opt out of auto-creation — exactly what this PR relies on. If the Compose binary used in production or CI has this bug, every mount in both Compose files will silently auto-create a missing host directory instead of failing, undermining the core safety claim repeated across the docs.
compose/docker-compose.yml#L84-L108: verify the Docker Compose version used to run this file honorscreate_host_path: false; if not guaranteed, add a preflight check (e.g., a small script or Makefile target) that asserts each${REMOTE_DEV_DATA_ROOT}child path exists beforedocker compose up.compose/truenas.yml#L84-L114: apply the same verification and compensating control for the TrueNAS bind mounts, including the password file bind.scripts/test_canonical_data_layout.py#L77-L89: tightenvalidate_bind_mountto assertbind.get("create_host_path") is Falseinstead ofis not True, and update or remove the comment claiming Compose "omits" the explicitfalsevalue, since current evidence contradicts it.Once confirmed, documentation claims that restate this guarantee (
docs/architecture.mdline 143,PROJECT_STATUS.mdline 25,README.mdline 89,README.es.mdline 89) should be revisited if the underlying Compose behavior cannot be trusted without a compensating check.docs/architecture.md (1)
1-193: LGTM!PROJECT_STATUS.md (1)
21-25: LGTM!Also applies to: 35-45
README.es.md (1)
3-170: LGTM!README.md (1)
3-6: LGTM!Also applies to: 16-129, 138-170
scripts/test_single_stack_compose.py (1)
16-17: LGTM!Also applies to: 27-27, 84-88, 119-139, 158-206, 208-274, 284-297
Makefile (1)
14-19: LGTM!.github/workflows/build-amd64.yml (1)
34-34: LGTM!Also applies to: 59-59
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5340e5a3a2
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 846115b401
ℹ️ 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".
|
Addressed the remaining Codex review findings on the final branch:
The complete CI run for the final head is in progress. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! 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
Closes #70
Summary
REMOTE_DEV_DATA_ROOTcontractworkspaces, private per-rolestate, andsecretscreate_host_path: falseas defense-in-depth rather than relying on inconsistent Compose runtime enforcementManual validation
Deferred until the related implementation slices are ready, following the maintainer decision to consolidate real TrueNAS validation. Do not create the new TrueNAS datasets or deploy this YAML before the PR is merged and its edge image is published.
Out of scope
WEB_PASSWORDandWEB_PASSWORD_FILE: docs/test: document and validate Codex web-password sources on TrueNAS #69Validation plan