fix(workspace): provision a per-agent git identity in agent checkouts (BLO-29050) - #1445
Conversation
CHECKPOINT ONLY — UNVERIFIED. Do not review this as a proposal and do not ship it.
The agent that wrote this was killed mid-run by provider capacity (429) before it
could run `pnpm typecheck` or any test, and before any of the three review lenses
executed. Nothing here has been compiled, executed, or reviewed. It is committed
solely so ~185 lines of work survive a worktree prune.
What it is meant to do: paperclip creates git checkouts for agent runs but never
provisions a git author identity in them, so commits are either unattributed or
stamped with the shared `allyblockcast[bot]` App identity. A 2026-08-10 sweep of
71 checkouts under /paperclip/work found 11 App-stamped and 18 with none.
Design decided before implementation:
- identity = `<normalizeAgentUrlKey(agent.name)>@paperclip.blockcast.net` + agent.name.
`normalizeAgentUrlKey` (packages/shared) was chosen over `normalizeAgentNameKey`
because the latter is only trim+lowercase, preserves spaces, and is therefore not
a legal email local-part.
- must NOT stamp the App email — paperclip's `policy` job treats that exact address
on a non-merge commit as a violation (BLO-21416).
- must be idempotent and must repair ALREADY-EXISTING checkouts, not just fresh
clones. That is the entire defect: heartbeat.ts's provisioning helper returns
early on several already-exists paths, and the 29 broken checkouts all take one.
- deliberately touches neither `check-commit-author-attribution.mjs` nor its test:
that gate has no local-part opinion, and branch `blockcast/blo-23894` is
rewriting both files.
What still has to happen before this is worth anything:
1. typecheck + tests actually run and pass
2. re-derive every early return in `ensureManagedProjectWorkspace` from source and
prove each reaches the identity step — a prior enumeration was judged incomplete
3. confirm the workspace-runtime seams are the shared tails (3991 / 4279), not the
four `worktree add` lines, and that the `reuseExistingWorktree` path is covered
4. confirm no write is scoped off `repoRoot` and every write passes `{ cwd }` so it
can never reach global/system config
Context: penstock #1414 (merged) pins why this matters downstream — a commit authored
by the `allyblockcast` User seat gets that seat's approval demoted as a self-review,
and it is the only identity that can green `review/ally-complete` on an App-authored
PR, so the PR deadlocks with no recovery.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… (BLO-29050)
paperclip creates git checkouts for agent runs but never provisions a git
author identity in them. A 2026-08-10 sweep of 71 checkouts under
/paperclip/work found 11 stamped with the shared `allyblockcast[bot]` App
identity and 18 with no local identity at all; AGENTS.md compensated by asking
every agent to run `git config` by hand.
`server/src/services/git-checkout-identity.ts` is the shared provisioning step,
imported by both heartbeat.ts and workspace-runtime.ts. Identity is
`<normalizeAgentUrlKey(agent.name)>@paperclip.blockcast.net` + `agent.name`,
via `deriveAgentUrlKey` so an all-punctuation name falls back to the agent id
rather than producing a bare `@paperclip.blockcast.net`.
Wiring covers the already-exists paths, which is the entire defect — a fix that
only ran after a successful clone would never reach any of the 29 broken
checkouts:
- heartbeat.ts: `ensureManagedProjectWorkspace` is split into an inner
resolver (4 returns + 1 throw) and a single-exit wrapper that provisions.
A future early return in the resolver inherits provisioning; it has no way
to return anywhere else.
- workspace-runtime.ts: `provisionExecutionWorktree` is the shared tail all
four worktree paths funnel through (create, attach-existing-branch
fallback, `reuseExistingWorktree`, persisted restore), and its reuse call
is now unconditional rather than gated on a configured provisionCommand.
`project_primary` — the *default* strategy — bypasses that funnel entirely
and gets its own `stampCheckoutIdentity` seam on all three of its returns.
Provisioning only rewrites addresses paperclip owns: unset, either App form, or
its own @paperclip.blockcast.net namespace. `git config --local` inside a linked
worktree resolves through the gitdir pointer to the *parent repository's*
config, so an unconditional write would retarget a developer's `user.email`
repo-wide — a worse bug than the misattribution being fixed.
It never throws (failures surface as warnings on a provisioning path), no-ops
when the path carries no git metadata (`git config` discovers upwards, so a
repo-less managed dir would stamp an ancestor), and probes with
lstat + isDirectory()||isFile() so a worktree's `.git` FILE is not mistaken for
"not a checkout".
Never writes the App email: `scripts/check-commit-author-attribution.mjs` treats
it as a violation on a non-merge commit (BLO-21416). That script and its test
are deliberately untouched — the gate has no local-part opinion, and branch
blockcast/blo-23894 is rewriting both files.
Verification:
pnpm typecheck exit 0
vitest run server/src/__tests__/git-checkout-identity.test.ts 26 passed
vitest run server/src/__tests__/workspace-runtime.test.ts 126 passed
vitest run server/src/__tests__/heartbeat-workspace-session.test.ts 231 passed
node scripts/check-no-git-push.mjs exit 0
node scripts/check-test-undefined-symbols.mjs exit 0
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 similar comment
Two findings from adversarial review — please read before mergingThis PR passed review with no critical findings, but two important ones survived verification and they bear on whether BLO-29050 is actually closed. Filing them here so they are not lost. 1. Under
|
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 2304d22
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/workspace-runtime.ts:3779—project_primarystamps the shared checkout identity immediately before handing it to a run, butensureCheckoutGitIdentitywritesgit config --localatserver/src/services/git-checkout-identity.ts:280-281; for linked worktrees, that local config is shared by the common repository as documented atserver/src/services/git-checkout-identity.ts:27-35. Concurrent runs can therefore overwrite each other’s identity, causing a later commit from Agent A to be authored as Agent B.- Recommendation: serialize identity-sensitive commits for shared/project-primary checkouts, or enable worktree-specific config and write the identity there. Do not rely on “last writer is about to commit” without an enforced lock spanning stamping through commit.
Suggestions (0)
Strengths
- Centralizes identity provisioning and covers fresh, existing, reused, and persisted checkout paths.
- Correctly avoids rewriting foreign human identities and keeps failures fail-open with warnings.
- Adds focused tests for App-stamped, unset, foreign, reused, and linked-worktree cases.
Recommended Action
- Fix the Important issue before merge.
- Re-run the workspace concurrency tests against the chosen serialization or worktree-specific configuration.
- Consider the existing focused test coverage and passing CI as strong regression protection.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 777becb
Prior Findings Dispositioned (1)
- prior:2304d22 critical 1 — fixed —
server/src/services/heartbeat.ts:23675— the final runtime configuration overlay now applies per-agentGIT_AUTHOR_*andGIT_COMMITTER_*values immediately before adapter dispatch, so linked-worktree commits no longer depend on the shared local config that caused the prior concurrent identity race.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Applies the identity overlay at the final dispatch boundary, after user, project, routine, scratch, and isolation environment composition.
- Correctly treats linked worktrees as shared-config hazards and avoids mutating their common repository config.
- Covers fresh, reused, persisted, project-primary, and managed-workspace paths with focused tests and structural call-site guards.
- Sanitizes generated author names and keeps provisioning failures fail-open with surfaced warnings.
Recommended Action
- No Critical or Important code issues found; the PR is approved.
- CI policy currently fails on a stale suite-duration manifest (
397 of 442suites covered), which appears unrelated to this diff; regenerate or rerun the policy lane if the repository requires a green check before merge.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3328783
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Centralizes per-agent Git identity generation and keeps local checkout provisioning fail-open.
- Applies the four process-level Git identity variables at the final adapter boundary, after lower-trust environment overlays.
- Correctly avoids shared local-config writes for linked worktrees while passing the identity into provision commands and
claude_k8sJob containers. - Adds focused coverage for App-stamped, unset, foreign, reused, persisted, linked-worktree, sanitizer, precedence, and real commit attribution paths.
Recommended Action
- No Critical or Important code issues found; approve.
- Allow the remaining CI checks to complete.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3328783
Prior Findings Dispositioned (1)
- prior:2304d22 important 1 — fixed —
server/src/services/workspace-runtime.ts:3785— project_primary now applies the per-agent Git identity at the direct-checkout return, while linked worktrees skip shared local-config writes and receive the authoritative per-run identity overlay at dispatch inserver/src/services/heartbeat.ts:24550.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Centralizes per-agent Git identity generation and keeps local checkout provisioning fail-open.
- Applies all four process-level Git identity variables after lower-trust environment overlays.
- Correctly avoids shared local-config writes for linked worktrees while passing identity into provision commands and adapter execution.
- Adds focused coverage for direct, reused, persisted, linked-worktree, sanitizer, precedence, and commit-attribution paths.
Recommended Action
- No Critical or Important code issues found; approve.
- Allow the remaining CI checks to complete.
1 participant
Thinking Path
Linked Issues or Issue Description
git pushcommits are unattributed or App-stamped.Related PRs found while searching for duplicates:
What Changed
ensureCheckoutGitIdentity()for idempotent, fail-open provisioning of standalone checkouts. It rewrites only unset, shared-App, or Paperclip-owned identities and preserves foreign developer identities.project_primarystrategy, fresh and reused Git worktrees, and persisted workspace restoration.GIT_AUTHOR_NAME,GIT_AUTHOR_EMAIL,GIT_COMMITTER_NAME, andGIT_COMMITTER_EMAILhelpers.git config --localfrom linked worktrees because it mutates shared parent config; those runs use the process-level identity instead.claude_k8s, whose Job manifest injectsconfig.envinto the container that performs the in-pod clone and commits.Verification
tsc --noEmit): exit 0 with no diagnostics.git diff --check: exit 0.Risks
Model Used
claude-opus-5[1m], 1M context) via Claude Code with extended reasoning, filesystem, shell, GitHub API, and Paperclip tools.Checklist