Remove Think's Git identity entirely - #34
Conversation
ensureGitRepo wrote user.name and user.email into whatever directory it was handed. That directory is not always a Think mind: point THINK_REPO_DIR at a source checkout, or run a test that resolves the repo root, and the checkout's own committer identity is silently replaced. The damage outlives the process. Every commit made by hand in that checkout afterwards is authored as think@local.invalid, a reserved .invalid address no forge can attribute to an account, so GitHub reports the commits as unverified with reason "no_user" even when the signature itself is good. A branch protection rule requiring signatures then blocks every pull request containing them. This was not hypothetical: it happened to this repository, and the fingerprint is still in its config history — the [user] section sits between two branch entries written during the work, alongside the core.fsmonitor value the same function sets. The identity is per-invocation state, so it now rides on THINK_GIT_CONFIG_ARGS as -c user.name / -c user.email. Those args already flow through withThinkGitConfig into both the direct runGit path and git-warp's runner, so both keep committing as the agent while nothing is persisted. Verified end to end with env -i and no stored identity: a real capture produced three warp commits authored and committed as think <think@local.invalid>, and the mind's config carries no user.* at all. test/ports/git-identity-isolation.test.js pins both halves. Before the change the host-repository test reported actual 'think' against expected 'Host Developer'.
Summary by CodeRabbit
WalkthroughGit identity configuration is centralized and passed to Git per invocation. ChangesGit identity isolation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Think
participant ensureGitRepo
participant Git
participant TargetRepository
Think->>ensureGitRepo: initialize target repository
ensureGitRepo->>Git: invoke with per-call identity configuration
Git->>TargetRepository: initialize or inspect repository
Think->>Git: run commit with shared identity environment
Git->>TargetRepository: create commit without persistent identity writes
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![chatgpt-codex-connector[bot]](https://avatars.github.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: 00a8503572
ℹ️ 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".
| const commit = spawnSync( | ||
| 'git', | ||
| ['-C', mindDir, 'commit', '-q', '-m', 'thought'], |
There was a problem hiding this comment.
This invokes plain git commit rather than the plumbing created above, so it never exercises the new THINK_GIT_CONFIG_ARGS path; moreover, the expected no-identity failure is treated as a successful test return, while a successful commit only has to use any identity other than the host fixture. Consequently, removing or breaking the runner's user.* arguments would still pass this regression test even though captures on machines without configured Git identity would fail. Drive an actual commit through Think's runner and assert exactly think <think@local.invalid>.
AGENTS.md reference: AGENTS.md:L53-L54
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@CHANGELOG.md`:
- Around line 18-19: Update the changelog text around the `think@local.invalid`
reference to distinguish account attribution from cryptographic validity: state
that the persisted commit email prevents forge account attribution for later
signed commits, unless a forge-configured no-reply address is used and supported
by the signing configuration.
In `@test/ports/git-identity-isolation.test.js`:
- Around line 91-124: Update the test around createThinkPlumbing to invoke
Think’s commit path through the returned plumbing instance instead of spawning
bare git commit. Require that operation to succeed without accepting or
returning on identity failure, then inspect the resulting commit and assert both
author and committer are exactly think <think@local.invalid>.
- Around line 78-82: Extend the assertions in the repository configuration test
to verify that readLocalConfig(mindDir, 'user.name') also returns null,
alongside the existing user.email assertion. Keep the test’s expected absence
behavior and explanatory assertion context consistent for both identity fields.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f60a29e2-7902-4c2d-b32f-6f1d550da459
📒 Files selected for processing (3)
CHANGELOG.mdsrc/git.jstest/ports/git-identity-isolation.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: test (22)
🧰 Additional context used
🪛 ast-grep (0.45.0)
test/ports/git-identity-isolation.test.js
[warning] 1-1: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process)
🔇 Additional comments (3)
src/git.js (2)
60-68: LGTM!
39-43: 🗄️ Data Integrity & IntegrationNo change needed. The default git-warp plumbing commit path sets Git author/committer identity through
GitPersistenceServicein its child environment, so-c user.namealone is not the only isolation mechanism.> Likely an incorrect or invalid review comment.test/ports/git-identity-isolation.test.js (1)
54-70: LGTM!
| const plumbing = createThinkPlumbing(mindDir); | ||
| assert.ok(plumbing, 'Expected Think plumbing for the mind.'); | ||
|
|
||
| // Drive a commit through the same runner Think uses, with no identity in the | ||
| // repository config and none inherited from the environment. | ||
| const result = spawnSync('git', ['-C', mindDir, 'add', '-A'], { encoding: 'utf8' }); | ||
| assert.equal(result.status, 0, `git add failed: ${result.stderr}`); | ||
|
|
||
| const commit = spawnSync( | ||
| 'git', | ||
| ['-C', mindDir, 'commit', '-q', '-m', 'thought'], | ||
| { | ||
| encoding: 'utf8', | ||
| env: stripIdentityEnv(process.env), | ||
| } | ||
| ); | ||
|
|
||
| // Without a stored identity and without env identity, git refuses to commit — | ||
| // which is exactly why the identity has to travel with each invocation. | ||
| if (commit.status !== 0) { | ||
| assert.match( | ||
| `${commit.stderr}`, | ||
| /Please tell me who you are|unable to auto-detect email/u, | ||
| `Unexpected commit failure: ${commit.stderr}` | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const author = git(mindDir, ['log', '-1', '--format=%an <%ae>']); | ||
| assert.notEqual( | ||
| author, | ||
| `${HOST_NAME} <${HOST_EMAIL}>`, | ||
| 'A mind commit must not be attributed to the host developer.' | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test the Think commit path instead of bare Git.
Lines 91-124 create plumbing but never use it. The test runs bare git commit, accepts the expected identity failure, and returns. It therefore passes without proving that Think can commit or that Think uses think <think@local.invalid>.
Create the commit through the createThinkPlumbing path. Require the commit to succeed. Assert the exact author and committer identities.
🤖 Prompt for 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.
In `@test/ports/git-identity-isolation.test.js` around lines 91 - 124, Update the
test around createThinkPlumbing to invoke Think’s commit path through the
returned plumbing instance instead of spawning bare git commit. Require that
operation to succeed without accepting or returning on identity failure, then
inspect the resulting commit and assert both author and committer are exactly
think <think@local.invalid>.
CodeRabbit was right about the third test: it drove a plain `git commit` rather than Think's runner, and treated the no-identity failure as a pass. Removing the runner's -c arguments would have left it green while every capture on a machine without a configured git identity broke. It now runs a real capture through the CLI with every ambient identity source removed — no repository config, GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM pointed at /dev/null, and GIT_AUTHOR_*/GIT_COMMITTER_* stripped — and asserts every resulting commit is exactly `think <think@local.invalid>` on both the author and committer side, with nothing persisted to the mind's config. Mutation-checked rather than assumed: with the -c arguments removed the new test fails and the old one passed. Also asserts user.name is absent alongside user.email, so persisting either half reintroduces the leak. The changelog conflated two different things. A wrong user.email does not invalidate a signature — the commits stay cryptographically valid. What breaks is attribution: `.invalid` is reserved, so no forge can match the address to an account and GitHub reports `verified: false` / `no_user`, which a rule requiring signed commits treats as unsigned. Said that way now.
flyingrobots
changed the title
1 participant
What this does
Think writes no Git objects.
commit,commit-tree,write-tree,hash-objectandupdate-refappear nowhere insrc/. Every mind commit is created by git-warp throughgit commit-tree, which takes no author argument and reads only ambient state.Supplying an identity was therefore never Think's job, and nothing reads one back — no surface queries
%an,%ae,%cnor%ce. So Think stops having one:THINK_GIT_CONFIG_ARGSreturns tocore.fsmonitor=falsealoneDEFAULT_GIT_ENVis deleted — dead weight regardless, sincerunGitonly ever runsinitandconfig, neither of which creates a commit, and it was never applied to the warp runner, so it never reached a single commit objectensureGitRepono longer writesuser.name/user.emailinto the target repositoryWhy the original defect mattered
ensureGitRepowrote identity into whatever directory it was handed. ATHINK_REPO_DIRaimed at a source checkout silently rewrote that repository's committer identity, and every commit made by hand there afterwards was authored asthink@local.invalid— a reserved TLD (RFC 2606) that no forge can attribute to an account.This was not hypothetical. It happened to this repository; the
[user]section sat in its config next to thecore.fsmonitorvalue the same function sets. It is the direct cause of everyBLOCKEDmerge here recently, because GitHub reported those commitsverified: false/no_userand therequired_signaturesrule treats that as unsigned.Blocked
test/ports/git-identity-isolation.test.jsasserts a capture carries the operator's configured identity. That currently fails — 187/188, both ratchets clean — and the failure is the acceptance gate, not a defect to work around.@git-stunts/plumbingfilters git's environment throughEnvironmentPolicy, whose allowlist contains noHOMEand noGIT_CONFIG_GLOBAL. Git cannot read~/.gitconfigand fabricatesuser@hostnameinstead. plumbing#13 fixes that and is approved, but unmerged and unpublished.Merging this before the fix ships would regress attribution from
think@local.invalidtojames@Jamess-MacBook-Pro-2.local— different garbage, not an improvement. Verified in both directions by patchingnode_modulesand restoring it.Merge order: plumbing#13 merged → plumbing published → Think's exact
3.0.3pin raised (see #37) → this branch green → merge.Correction
5d8c937's message claims that without an injected identity "git would refuse to commit and the capture would fail outright." That is false. Git auto-detects from the gecos field and hostname and commits successfully. The mutation check cited there did fail, but on the author string, not on the capture breaking — a mechanism asserted before it was checked. Corrected in94bb8e7, since commits are not amended here.Follow-ups filed
gitdirectly forinit,config,ls-remote,pushand context reads; this PR removes only the identity portion