feat(sandbox): pin git identity to driver#53
Conversation
Mingholy
left a comment
There was a problem hiding this comment.
Code Review: PR #53
Summary
This PR reads the sandbox driver's personal .gitconfig (from $LOOPAT_HOME/personal/<user>/.gitconfig), parses the [user] section for name and email, and injects GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, GIT_COMMITTER_NAME, GIT_COMMITTER_EMAIL into the container environment at podman create time. This ensures git commits made by the AI inside the sandbox carry the driving user's identity instead of a generic or empty author.
Implementation Quality
Architecture fit: good. The injection point (buildContainerEnv) is correct — env vars baked at container create time are inherited by every podman exec (both SDK and PTY), so the identity is consistent across all sandbox processes. Placing it before the extraEnv loop (lines 419-421) means callers can still override if needed, preserving the "caller env wins" contract already tested.
Parser scope: appropriate. The hand-rolled INI parser only targets [user] name/email — no need for a full gitconfig library. It correctly handles:
- Multi-section files (tracks
inUserSectionstate) - Comments (
#,;) - Quoted values (single/double, stripped)
- Subsections like
[user "foo"]are correctly rejected by the=== "user"check
All-or-nothing return: good. Requiring both name and email (returning null otherwise) prevents half-configured identity where git would still prompt or fall back.
Test coverage: solid. Tests cover:
- Happy path with realistic multi-section
.gitconfig - Missing file → null
- Incomplete config (name without email) → null
- Integration via
buildContainerEnv— both present and absent cases
Code Quality
Clean, readable, well-placed. No style issues. The function is exported and tested independently, which is good practice.
Two minor observations (non-blocking):
-
include/includeIfdirectives: The parser doesn't follow git's[include] path = ...or[includeIf ...]directives. This is fine for the use case — the personal.gitconfigis a loopat-managed file, not the user's system~/.gitconfig, so conditional includes are unlikely. Worth a one-line comment if you want to make the limitation explicit. -
Staleness window: Because env is deliberately excluded from
hashCreateArgs(line 546-551 comment — correct design to avoid the PTY exit-137 regression), changing.gitconfigafter a container is created won't take effect until the container is recreated for another reason (image drift, vault change, etc.). This matches how all other create-time env vars behave, so it's consistent — just worth being aware of. A future enhancement could detect identity drift in the exec-time path if this becomes a pain point, but that's out of scope here.
Security
No concerns.
- The file read is bounded to
personalDir(opts.createdBy)— a controlled path under$LOOPAT_HOME/personal/<user>/. No path traversal possible;personalDiris computed from the authenticatedcreatedByfield. - Values are passed via
podman create --env K=V, not shell-interpolated. Even if a.gitconfigcontained adversarial content in the name/email fields, it would only affect the user's own sandbox container. This is a self-injection scenario with no privilege escalation. - The personal dir is already bind-mounted into the container (both virtual and host-absolute paths), so reading
.gitconfigfrom it at container-build time doesn't expand the attack surface.
Verdict
LGTM. Clean feature, correct integration point, good test coverage, no security issues. The two observations above are informational, not blocking.
Summary
.gitconfigidentity and inject it into sandbox process environment when available.Test plan
cd web && bunx tsc -bbun run buildgit diff --checkweb, andserverpackage manifests do not define alintscript.Screenshots
No before/after screenshot is attached because this is backend/container environment behavior with no user-facing UI state.