fix(provider): use init-files for gh config to fix .config ownership

[dpup]

Summary

• Root cause: Docker creates missing parent directories as root:root when setting up bind mounts. The GitHub provider's bind mount to ~/.config/gh caused ~/.config to be owned by root, breaking any CLI that needed to write config files inside the container.
• Fix: Convert GitHub provider from ContainerMounts (bind mount) to ContainerInitFiles (matching the Graphite provider pattern). Config is passed via MOAT_INIT_FILES env var and written by moat-init.sh with correct ownership.
• Security hardening: Sanitize gh config YAML to strip embedded credentials (hosts, oauth_token) before passing to container. Unset MOAT_INIT_FILES env var after processing.
• Defense-in-depth: Pre-create ~/.config in the Dockerfile with correct moatuser ownership.

Test plan

• go test -race ./internal/providers/github/ — all tests pass
• go test -race ./internal/deps/ — all relevant tests pass (pre-existing GitHub URL 401s unrelated)
• make lint — 0 issues
• Manual: moat run --grant github — verify ls -la /home/moatuser/ shows .config owned by moatuser
• Manual: verify gh CLI works inside container (aliases, preferences preserved)
• Manual: verify a CLI that writes to ~/.config/ succeeds inside the container

🤖 Generated with Claude Code

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @dpup's task in 2m 17s —— View job

---

Code Review

• Gather context and understand the PR
• Read changed files
• Review security aspects (credential sanitization)
• Review implementation correctness
• Review tests
• Post final review

---

The fix is well-reasoned and consistent with the Graphite provider pattern. Root cause is correctly identified and the defense-in-depth (~/.config pre-created in Dockerfile) is a nice addition. A few things worth calling out:

Issues

1. Silent error swallowing in sanitizeGhConfig — no diagnostic emitted

internal/providers/github/provider.go:79-82

sanitized, err := sanitizeGhConfig(content)
if err != nil || sanitized == nil {
    return nil
}

If the user's ~/.config/gh/config.yml contains invalid YAML (e.g., anchors, unusual syntax, corrupted file), yaml.Unmarshal returns an error. That error is silently dropped — the container starts without gh preferences and the user gets no signal. A log.Debug at minimum would help diagnose "why is my gh alias not working inside the container" reports. Fix this →

2. YAML round-trip removes comments and reorders keys — not documented

sanitizeGhConfig round-trips through map[string]interface{} → yaml.Marshal. This silently strips all comments and alphabetically re-sorts all keys in the written config. A user with carefully-organized config.yml comments will lose them on next run. Worth a brief note in the function doc comment since this is a known, intentional limitation. Fix this →

Minor observations

3. No test for invalid YAML input

TestSanitizeGhConfig doesn't cover the yaml.Unmarshal error path. Given the silent-drop behavior (issue #1), a test like {name: "invalid yaml", input: ":\t:\n", wantNil: true} would at least document the behavior explicitly.

4. ContainerMounts stub

The stub at provider.go:108 is fine — the interface requires the method. The comment is clear. Just noting it's a (required) noop.

What's good

• Credential sanitization (delete(cfg, "hosts") + delete(cfg, "oauth_token")) covers both the hosts: section and the legacy top-level oauth_token field used by older gh versions.
• The path-safety check in manager.go:1397-1399 (filepath.IsAbs + HasPrefix(containerHome)) prevents init files from escaping the container home directory.
• unset MOAT_INIT_FILES in moat-init.sh is good hygiene — prevents config content from being visible to child processes.
• Dockerfile pre-creating ~/.config with correct ownership is solid defense-in-depth for future providers that might add bind mounts into other ~/.config subdirectories.
• Test coverage for sanitization edge cases (CredentialsOnly, StripsCredentials, NoConfig) is thorough.

The core fix is correct and ready to merge. Issues #1 and #2 are low-severity but would improve debuggability.

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @dpup's task in 2m 31s —— View job

---

Code Review

• Gather context and read changed files
• Review security aspects (credential sanitization)
• Review implementation correctness
• Review tests
• Post final review

---

The issues raised in the previous review are resolved: log.Debug is now emitted on sanitization failure (provider.go:81-83), the YAML round-trip caveat is documented in the function comment (provider.go:100), and the "invalid yaml" test case is present (provider_test.go:254-257). This second pass focuses on things not flagged before.

Issues

1. chmod 755 applied only to leaf directory — intermediate dirs created by mkdir -p get default umask permissions but not an explicit mode

internal/deps/scripts/moat-init.sh:199

dir=$(dirname "$filepath")
mkdir -p "$dir" && chmod 755 "$dir"

If both ~/.config/ and ~/.config/gh/ are absent, mkdir -p creates both. chmod 755 only touches the leaf (gh/). In practice this is safe because ~/.config/ is pre-created in the Dockerfile (dockerfile.go:319-320) before init runs, so mkdir -p never needs to create it — but there's a latent fragility if any future provider path doesn't have its parent pre-created. Worth a comment explaining the Dockerfile invariant this relies on, or applying the permissions to each directory in the loop that already fixes ownership. Not a current bug given the Dockerfile pre-creates ~/.config.

2. MOAT_INIT_FILES env var written to host-side proxyEnv and logged via --verbose — config YAML content is visible there

internal/run/manager.go:1419

proxyEnv = append(proxyEnv, "MOAT_INIT_FILES="+buf.String())

The sanitization correctly removes credentials from the YAML before this point, so actual tokens don't leak. But non-sensitive gh preferences (aliases, editor choice, etc.) do appear in proxyEnv. If proxyEnv is written to debug logs or the daemon's run-registration payload, this is a minor privacy concern. Worth a quick check that proxyEnv doesn't end up in any log output — if it does, a log.Debug note clarifying the content is sanitized would be helpful for auditors.

Minor observations

3. delete(cfg, "oauth_token") on the top-level map covers the legacy single-token format. Any token or _token aliases newer gh versions might introduce would not be stripped. This is fine today; worth noting in the comment that this covers the known formats as of the current gh spec.

4. sanitizeGhConfig returns ([]byte, error) but callers treat any err != nil as "skip silently" (now with debug log). The function's exported-style name and signature imply it could be a public API surface eventually. If it stays private, the current contract (nil = don't write) is clear. If it ever becomes exported, the nil-vs-empty distinction should be documented more precisely.

What's good

• The fix is architecturally correct and consistent with the Graphite provider pattern.
• unset MOAT_INIT_FILES in moat-init.sh:212 is solid hygiene.
• Path safety check in manager.go:1398 (IsAbs + HasPrefix(containerHome+sep)) correctly prevents escape from container home.
• The chown walk-up loop in moat-init.sh:206-209 correctly fixes ownership on all intermediate directories up to (but not including) $INIT_HOME, solving the root cause cleanly.
• Dockerfile pre-creation of ~/.config (dockerfile.go:319-320) provides good defense-in-depth.
• Credential sanitization deletes both hosts (covering nested oauth_token) and top-level oauth_token, covering both modern and legacy gh storage formats.
• Test coverage is thorough: preferences-only, strip-credentials, credentials-only (returns nil), no config, and invalid YAML paths are all exercised.

The core fix is correct and the previous review's issues are addressed. Issue #1 above is low severity and not a current bug. Ready to merge.

[claude]

Claude finished @dpup's task in 3m 38s —— View job

---

Code Review

• Gather context and understand the PR
• Read changed files
• Review security aspects (credential sanitization)
• Review implementation correctness
• Review tests
• Post final review

---

The previous two review passes addressed all issues raised (debug log on sanitization failure, YAML round-trip caveat in doc comment, invalid YAML test case). This pass focuses on anything not yet flagged.

Remaining open item from review #2 (not addressed)

chmod 755 "$dir" only touches the leaf directory

internal/deps/scripts/moat-init.sh:199

dir=$(dirname "$filepath")
mkdir -p "$dir" && chmod 755 "$dir"

If mkdir -p creates multiple intermediate directories, only the leaf gets chmod 755. Noted in review #2 as not a current bug because ~/.config/ is pre-created in the Dockerfile, but this is a latent fragility if a future provider registers an init file whose grandparent doesn't exist. Worth a comment explaining the invariant (e.g. # parent dirs are pre-created in Dockerfile; mkdir -p only needs to create the leaf). Fix this →

New observations

1. MOAT_INIT_FILES is NOT in debug logs — privacy concern from review #2 is unfounded

manager.go:601 logs providerEnv (the per-provider ContainerEnv output), not proxyEnv. The combined proxyEnv — which includes MOAT_INIT_FILES — is only passed to the container runtime as the container's Env, never logged. Issue #2 from review #2 is a false alarm.

2. sanitizeGhConfig nil-map handling is correct but unexercised

If the input YAML is null or a completely empty file, yaml.Unmarshal produces nil for cfg. delete(nil, ...) is safe in Go (no-op), len(nil) == 0 triggers the early return nil, nil. The result: no config file is written, which is the right behavior. No test covers this case, but it's a minor gap.

3. TestSanitizeGhConfig "invalid yaml" doesn't verify the error path

if tt.wantNil {
    if result != nil {
        t.Errorf(...)
    }
    return  // doesn't check err
}

The "invalid yaml" case returns (nil, err) from the function, but the test only verifies result == nil, not that err != nil. This means if a future refactor changed the function to return (nil, nil) for bad YAML (bypassing the log.Debug), the test would still pass. Minor, but worth a one-liner if err == nil { t.Error("expected error for invalid yaml") } to lock in the distinction between "parse error" and "credentials-only" code paths.

4. unset MOAT_INIT_FILES placement is correct

The unset at line 212 runs in the parent shell (not the pipe subshell), so the env var is cleared before exec gosu moatuser "$@" at line 369. Child processes won't inherit it. ✓

5. os.ReadFile errors other than "not found" are silently swallowed

provider.go:75-78 — if ~/.config/gh/config.yml exists but is unreadable (e.g., permissions issue on the host), os.ReadFile returns a non-IsNotExist error, and the function returns nil silently. This is a weaker form of the issue from review #1 (which was fixed for sanitize errors). Worth a log.Debug for non-IsNotExist read errors. Fix this →

What's good

• The core fix correctly eliminates the root-owned ~/.config problem with a clean approach.
• sanitizeGhConfig correctly handles nil maps (empty/null YAML input is safe).
• unset MOAT_INIT_FILES runs in the right shell scope — env is cleared before exec.
• The interface compliance check at compile time (_ provider.InitFileProvider = (*Provider)(nil)) catches future interface changes immediately.
• The "credentials-only" returns nil (not an empty map), so no empty config file is written.
• MOAT_INIT_FILES is not emitted to debug logs, so gh preferences (aliases, editor) don't appear in ~/.moat/debug/.

Issues #5 (silent read error) is the only new actionable finding. Issues are all low-severity; the fix is correct and ready to merge.