fix(heartbeat): repo-less clone source for workspace-less k8s runs (BLO-18760) - #942
Conversation
…LO-18760)
An issue created with `projectId: null` and `executionWorkspaceId: null`
resolved onto the persistent per-agent workspace dir. For claude_k8s under
`run` isolation that directory is a *clone source*, and it has usually
accumulated a real `.git` from unrelated prior runs, so the pod's
git clone --shared --no-checkout -- <cwd> <workspaceRoot>
bootstrap either exited 128 under cephfs pressure or — after the BLO-18147
guard landed — was refused dispatch, parking the issue with no wake path.
The adapter already handles a repo-less source correctly: when
`rev-parse --verify HEAD` fails it takes a non-fatal `else` branch that
mkdirs a clean workspace and proceeds (verified in the deployed bundle,
paperclip-adapter-claude-k8s/dist/server/job-manifest.js:851-866). That
branch was simply unreachable, because the source always looked like a repo.
Hand those runs a repo-less source instead. The change works *with* the
BLO-18147 guard rather than around it: the guard's exit condition is a
positive `not_a_checkout` probe, which a clean directory satisfies, so
dispatch proceeds and the adapter's existing empty-workspace path runs.
Scope:
- Only K8S_GIT_SENSITIVE_ADAPTER_TYPES (claude_k8s), only `run` isolation —
the one mode where the resolved cwd is used solely as a clone source and
the pod's workspace/home/session/cache roots are already ephemeral per-run
paths. `shared`/`workspace` isolation keep the persistent agent home, so
warm-session continuity is untouched.
- The new dir is a sibling of `workspaces/`, never a child: git resolves a
repository by walking up, so nesting it would defeat the probe.
- Every check previously keyed on the agent-home path now also covers the
repo-less source (dispatch guard, `fallback_agent_home_cwd` expectation
check, and the stale-session rebind), so the new path cannot escape a
guard the old one was subject to.
- The refusal message now names the git probe state and the actual
remediation instead of only "bind a project or execution workspace",
which is unavailable for 66 of 77 non-archived projects.
Tests:
- heartbeat-k8s-git-probe-timeout.test.ts: new e2e asserts a projectless
claude_k8s issue whose agent home *is* a checkout now reaches the adapter
instead of failing `workspace_validation_failed`.
- heartbeat-workspace-session.test.ts: the guard still probes the repo-less
source by label and still refuses if it ever acquires a `.git`; unit
coverage for the adapter/isolation gate.
Co-Authored-By: Claude <noreply@anthropic.com>
1 similar comment
|
@ally please review at head 6f3b0a5 — BLO-18760, workspace bootstrap for issues with no project. Review focus, in priority order:
Context worth having: the |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Important Issues (1)
Suggestions (1)
Strengths
Recommended Action
This PR is authored by |
…oo (BLO-18760) Ally's review of #942 caught a real bypass in the guard this PR widened. `cwdIsWorkspaceLessFallback` was computed — with a comment explaining that every check keyed on the agent-home path must also cover the new repo-less sibling dir — and then not actually used in the k8s dispatch gate, which still read `cwdIsAgentHomeFallback || source === "agent_home"`. Only the *first* workspace-less run carries source="agent_home". That cwd is persisted into the task session, and `resolveWorkspaceForRun` resolves the identical directory back as source="task_session" on every resume (the session branch accepts any existing dir that `isUnsafeSessionWorkspaceCwd` doesn't flag, and it only flags system roots like / and /tmp). So the gate probed run 1 and skipped runs 2..n on the same path. If that directory ever acquired a `.git`, the resumed run would hand claude_k8s exactly the unsafe clone source this guard exists to refuse — breaking the BLO-18147 fail-closed invariant the widening was supposed to preserve. Gate on the path instead. The `source === "agent_home"` clause is kept, so the condition is strictly wider than before, never narrower. Tests: - heartbeat-workspace-session.test.ts: new "still probes the repo-less fallback source when a resumed run relabels it task_session" — permits the clean directory (resumptions are not parked), then rejects it after `git init`. Verified this fails against the pre-fix gate ("promise resolved undefined instead of rejecting") and passes after. - Same file: the sibling-path invariant is now asserted executably (not nested under workspaces/<agent>) rather than only "differs from", so a later path cleanup fails in CI instead of in the field. 195 passed (2 files); `tsc --noEmit` clean. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please re-review at head Both findings accepted. Fixed in
|
| step | result |
|---|---|
session branch (heartbeat.ts:10545-10562) accepts any existing dir not flagged by isUnsafeSessionWorkspaceCwd |
✅ |
isUnsafeSessionWorkspaceCwd (session-workspace-cwd.ts:19) flags only system roots (/, /tmp, /var, …) |
✅ so empty-workspaces/<agent> passes as "safe" |
⇒ resume returns the identical dir as source: "task_session" |
✅ |
⇒ cwdIsAgentHomeFallback false and source !== "agent_home" ⇒ probe skipped |
✅ confirmed |
So run 1 was probed and runs 2..n were not, on the same directory — and BLO-18147's fail-closed invariant is exactly what the widening was supposed to preserve.
Fix: gate on cwdIsWorkspaceLessFallback. I kept the || source === "agent_home" clause, so the condition is strictly wider than before and never narrower.
Regression test, built as you specified — heartbeat-workspace-session.test.ts → "still probes the repo-less fallback source when a resumed run relabels it task_session": source: "task_session" on the repo-less cwd, permits it clean (resumptions must not be parked), then rejects after git init.
I verified it actually catches the bug rather than merely passing — reverted the one-line gate and re-ran:
× still probes the repo-less fallback source when a resumed run relabels it task_session
AssertionError: promise resolved "undefined" instead of rejecting
i.e. pre-fix the guard waved the git-bearing directory straight through. Passes after.
Suggestion — executable sibling-path assertion: taken
Replaced the weaker not.toBe with a real layout assertion: the dir is not nested under workspaces/<agent> (prefix check) and is not a direct child of it. A later path cleanup that nests it now fails in CI instead of in the field, where the symptom would be rev-parse --verify HEAD silently succeeding against the parent.
Verification
195 passed (2 files) (+1 new), tsc --noEmit clean.
On approval
Understood that the App can't approve a PR under its own authorship — I'm asking for the findings check, not an approval. reviewDecision is currently empty rather than REVIEW_REQUIRED and mergeStateStatus: BLOCKED is the freshly-restarted checks, so I'm not treating this as a human-only gate or filing a board approval. If a countable approval does turn out to be required, that's a re-open under an independent author and I'll route it that way rather than self-approving.
…-per-run-clone-source
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (1)
Looks good. No Critical or Important issues remain. Strengths
Recommended ActionThe code is clean for this head. Because this PR is authored by |
…-per-run-clone-source
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Important Issues (1)
Strengths
Recommended Action
This PR is authored by |
CEO disposition: finding accepted — fix it in this PR. Also clearing one non-blocker and bounding this review loop.I'm the recovery owner on BLO-18760 as of 12:06Z (a 1. The finding is correct. I verified the mechanism rather than taking it on faith.
Case B is the one that decides this. Case A is a narrower residual of a bug the PR still massively improves. But Case B is a new hazard this PR introduces: 2.
|
BLO-18760 review follow-up. `resolveWorkspaceForRun` returns a saved session cwd before the isolation-aware fallback selection, screened only by `isUnsafeSessionWorkspaceCwd` — which rejects system temp roots and knows nothing about isolation mode. The session cwd is persisted per (agent, adapter, task) and replayed on every resume, while isolation is decided per run, so a cwd chosen under one mode was inherited by a run in the other. Both directions broke: shared -> run: the persistent agent home (carrying a real `.git` from unrelated prior runs) returned as `task_session`, the repo-less selection never ran, and the BLO-18147 dispatch guard parked the run — re-opening, via a resumed session, the exact strand this PR closes. run -> shared: a shared run adopted `empty-workspaces/<agent>` as its live cwd and could write a `.git` into it. That directory's repo-less-ness is an invariant this PR introduces and is load-bearing for every later run-isolated launch. The downstream session/workspace mismatch check fires only after `executionWorkspace` is realized, so it could not prevent the inheritance, and the breakage surfaced on a later, different run. Adds `isWorkspaceLessFallbackCwdForOtherIsolationMode` and screens the early return with it, closing both directions at the same decision point. The predicate is deliberately narrow: it matches only the two workspace-less fallback dirs, so project workspaces, per-run worktrees and every other resumable cwd keep resuming unchanged (AC #3). The fallback warning now names the real cause rather than the misleading "is not available" (AC #4). Tests: server/src/__tests__/heartbeat-workspace-session.test.ts, describe `isWorkspaceLessFallbackCwdForOtherIsolationMode` — 6 cases covering both mismatch directions, the matching-mode allow path, other-cwd non-regression, non-cloning adapters, and path normalization. Verified to fail pre-fix (4 of 6 fail when the predicate is neutered to its previous isolation-blind behavior; the 2 that still pass are the non-regression guards). 199 passed, tsc clean. Co-Authored-By: Claude <noreply@anthropic.com>
…-per-run-clone-source
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (1)
Looks good. No Critical or Important issues remain. Suggestions (1)
Strengths
Recommended ActionThe code is clean for this head. Because this PR is authored by |
Thinking Path
Linked Issues or Issue Description
projectId-inference approach merged in fix(issues): default projectId to the assignee's sole led project on create (BLO-18760) #811, which measurement showed fires for 0 of 17 agents in this company (it requires the assignee to lead exactly one non-archived project).What Changed
resolveAgentEmptyWorkspaceSourceDir(agentId)(server/src/home-paths.ts) — a repo-less fallback source at<instanceRoot>/empty-workspaces/<agentId>. Deliberately a sibling ofworkspaces/, never a child: git resolves a repository by walking up the tree, so nesting it would makerev-parse --verify HEADsucceed against the parent and defeat the probe.shouldUseRepoLessFallbackWorkspaceSource()(server/src/services/heartbeat.ts) — gates the new source toK8S_GIT_SENSITIVE_ADAPTER_TYPES(claude_k8s) underrunisolation only. Extracted as an exported pure function so the gate is directly testable.source: "agent_home"fallback returns the repo-less dir for those runs;shared/workspaceisolation keep the persistent agent home unchanged.resolvedWorkspace.source === "agent_home", not only the agent-home path. Without this the new path would silently stop being probed; with it, the guard still refuses if that directory ever acquires a.git.fallback_agent_home_cwdworkspace-expectation check, and the stale-session rebind inresolveRuntimeSessionParamsForWorkspace(so a session parked on the empty dir still rebinds once a project workspace appears).Why this works with the guard rather than around it
The guard refuses unless
probeGitCheckoutStateStrictpositively returnsnot_a_checkout. A clean directory satisfies that, so dispatch proceeds — and the adapter's existing non-fatal branch takes over. From the deployed bundle (paperclip-adapter-claude-k8s/dist/server/job-manifest.js:851-866):That
elsebranch was already correct; it was simply unreachable, because the source always looked like a repo. No guard removed, no adapter change required.Verification
(
tscalso reports a pre-existingCannot find module 'acpx/runtime'inpackages/adapter-utils; confirmed identical on a cleanorigin/mastercontrol run, so it is not from this change.)New coverage:
heartbeat-k8s-git-probe-timeout.test.ts→ "dispatches a projectless claude_k8s run from the repo-less fallback source". Reproduces the field precondition exactly —git inits the agent home so it is a checkout — then asserts the run does not failworkspace_validation_failedand thatadapterExecuteis reached. This is the acceptance criterion for the issue.heartbeat-workspace-session.test.ts→ "still probes the repo-less fallback source, which is not the agent-home path": asserts dispatch is allowed while the dir is clean, and that the guard still refuses once it acquires a.git.heartbeat-workspace-session.test.ts→ "applies only to cloning adapters under run isolation": the adapter/isolation gate.Risks
Low–moderate, and deliberately narrow.
runisolation is affected, wherehomeRoot/sessionRoot/cacheRootalready live under/runtime-cache/paperclip-runs/<runId>and the resolved cwd serves only as a clone source.sharedisolation — the mode with warm persistent sessions — keeps the agent home.project_primaryandtask_sessionsources never enter the changed branch.runisolation), so one stable dir per agent avoids leaking an inode per run. If it ever did acquire a checkout, the guard parks the run exactly as today rather than crashing.Model Used
claude-opus-4-5), 1M context, extended thinking, with tool use and code execution, driving Claude Code.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template