Skip to content

fix(orchestrator): preserve shared artifact permissions - #35

Open
NeoTheCapt wants to merge 1 commit into
devfrom
fix/shared-artifact-permissions
Open

fix(orchestrator): preserve shared artifact permissions#35
NeoTheCapt wants to merge 1 commit into
devfrom
fix/shared-artifact-permissions

Conversation

@NeoTheCapt

Copy link
Copy Markdown
Owner

Summary

  • create atomic replacement files inside engagement directories instead of /tmp
  • preserve shared 0664 modes and parent group inheritance for UI-readable artifacts
  • mark generated workspaces setgid (2775) so container and host orchestrator share the same group
  • add regression coverage for artifact modes and workspace permissions

Root cause

The runtime container writes engagement artifacts as root while the host orchestrator runs as a non-root user. mktemp created 0600 root:root files under /tmp; moving them onto the bind mount preserved those permissions. Listing a run then failed with PermissionError while reading or updating scope.json and surfaces.jsonl.

Test plan

  • permission regression test fails before the fix and passes after it
  • launcher workspace permission test passes
  • backend Python compilation passes
  • frontend production build passes
  • ARM64 image regression test passes
  • production /projects/2/runs API returns 200 after the hotfix

Create atomic replacement files inside the engagement directory, preserve group-writable modes for host/container sharing, and mark generated workspaces setgid. Add regressions for artifact and workspace permissions.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a5925a036e

ℹ️ 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".

if workspace_root.exists():
shutil.rmtree(workspace_root, ignore_errors=True)
workspace_root.mkdir(parents=True, exist_ok=True)
workspace_root.chmod(0o2775)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Apply group-write defaults to generated workspace children

Setting only the workspace's setgid bit makes new children inherit its GID, but it does not make them group-writable. The container entrypoint leaves the default 022 umask in place, and engagement initialization creates scope.json and surfaces.jsonl with ordinary redirections, so these initial artifacts remain 0644 root:<shared-group>. The host-side heartbeat can then fail when normalize_active_scope() rewrites scope.json or appends process-log surface candidates before one of the newly changed atomic-replacement scripts happens to replace that file. Configure a shared umask/default ACL or explicitly initialize all container-created public artifacts as group-writable rather than relying on setgid alone.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant