Skip to content

fix(docker): make stage2-hook rootless-safe without regressing ownership bootstrap - #58203

Open
fisker086 wants to merge 1 commit into
NousResearch:mainfrom
fisker086:fix/stage2-hook-rootless
Open

fix(docker): make stage2-hook rootless-safe without regressing ownership bootstrap#58203
fisker086 wants to merge 1 commit into
NousResearch:mainfrom
fisker086:fix/stage2-hook-rootless

Conversation

@fisker086

Copy link
Copy Markdown

Summary

Make docker/stage2-hook.sh tolerate rootless Podman and K8s runAsUser startup
without regressing the normal Docker-as-root bootstrap path.

Under set -eu, a failing mkdir -p "$HERMES_HOME" currently aborts the entire
cont-init hook in environments where the data volume must be pre-created on the
host (rootless Podman). This change treats that mkdir failure as non-fatal while
preserving the #18482 / #18488 behavior for real root starts.

Root-only operations (UID/GID remap, docker socket group setup, targeted chown)
are guarded with if [ "$(id -u)" = 0 ] instead of removing them outright, so
#19788 / #19795 ownership reconciliation continues to work for the majority of
non-rootless Docker users.

What changed

What this PR does NOT change

  • No fork/hosting-specific bootstrap (SPARK_*, bootstrap_spark_runtime.py,
    sync_nexus_hermes_config.py, etc.).
  • No removal of targeted ownership handling for normal Docker users.
  • Official upstream main does not contain bash array syntax in this file; this
    PR is scoped to rootless / non-root startup safety only.

Related

Fixes #

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature
  • Security fix
  • Documentation update
  • Tests
  • Refactor
  • New skill

How to Test

Normal Docker (regression check)

docker run --rm -e HERMES_UID=$(id -u) -e HERMES_GID=$(id -g) \
  -v ~/.hermes-test:/opt/data <image>
# Expect: mkdir, UID remap, targeted chown all run; hook completes successfully.

@alt-glitch alt-glitch added type/bug Something isn't working area/docker Docker image, Compose, packaging sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades P2 Medium — degraded but workaround exists labels Jul 4, 2026

@tonydwb tonydwb 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.

Code Review Summary

Verdict: Comment (token read-only; formal approval not possible)

Makes docker/stage2-hook.sh rootless-Podman/K8s safe without regressing normal Docker-as-root bootstrap. mkdir failure is now non-fatal, root-only operations guarded with if [ "$(id -u)" = 0 ], preserving prior fixes for HERMES_HOME bootstrap and ownership reconciliation.

  • Well-scoped: only rootless safety, no removal of existing functionality
  • Good backward compatibility for majority Docker users
  • Detailed analysis of what changed vs. what was preserved

Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for preserving the root bootstrap and targeted ownership behavior while handling the failed-mkdir path.

Problems

  • The Kubernetes claim is broader than the image currently supports: docker/stage2-hook.sh:50-73 exits before this patch for any non-root UID other than hermes, and docker/main-wrapper.sh:31-50 mirrors that guard. This change can support the existing runAsUser: 10000 path, but not arbitrary runAsUser values.
  • Please add regression coverage for the rootless bootstrap scenario. Existing tests/docker/test_user_flag_guard.py:14-65 covers arbitrary UID rejection and the UID-10000 case, not a mapped-root failed mkdir plus writable pre-provisioned volume.

Suggested changes

  • Narrow the Kubernetes wording to the supported hermes UID case unless the deliberate arbitrary-UID policy is being changed separately.
  • Add an end-to-end rootless/mapped-root boot test that verifies cont-init completes and the runtime writes state.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/docker Docker image, Compose, packaging P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants