Skip to content

Commit 33eb3cd

Browse files
committed
Fix ABCA-815 stacked-child lost work: 4-layer workspace-integrity defense
A aws-samples#247 stacked child's code never reached its branch, yet the task reported COMPLETED/build_passed:true/pr_url:null — a silent success that poisoned the integration (the child's feature was absent and its successor had nothing to stack on). Root cause (confirmed live on backgroundagent-dev, both substrates): the agent sometimes runs its OWN `gh repo clone` into a sibling dir (/workspace/<repo-name>) and commits/opens its PR there, leaving the platform-tracked repo_dir empty. Non-deterministic — the same task delivers cleanly on a retry — so a prompt tweak alone can't be relied on. Defense-in-depth (all per-task; the same path a single task takes): 1. Clone-guard (hooks.py pre_tool_use_hook): deny a Bash re-clone of the task's OWN repo before Cedar eval, with a redirect to work in place. Scoped to the task repo (dependency clones pass); fires even under bypassPermissions; command-position + free-text-arg aware so a PR body quoting "gh repo clone" is not blocked. repo_url threaded via build_hook_matchers. 2. Prompt (prompts/base.py): state the repo is already cloned+checked out at cwd and forbid re-cloning / cd-ing elsewhere. 3. reconcile_agent_branch (post_hooks.py, before ensure_committed): if HEAD is on a branch other than the platform branch with commits, re-point the platform branch (git branch -f + checkout) so delivery runs on the tracked branch. 4. Delivery gate (pipeline.py _apply_delivery_gate): a create-strategy new-work task that ends success with no PR AND no commit is FAILED, not COMPLETED — deliverable=lost (nothing landed) vs deliverable=no_pr (commit landed, PR failed). Exempts read_only/artifact/needs_input/non-create. error-classifier maps both to retryable TRANSIENT with honest "change was not saved" copy. Also repo.py: clear pre-existing workspace residue before clone + assert .git is at repo_dir root after clone (fail loudly instead of silently working a nested tree). Verified live: gate caught the rogue clone loudly (2/3 pre-guard runs); guard+prompt then delivered 9/9 stacked children across three A->B->C chains.
1 parent 19e5b86 commit 33eb3cd

13 files changed

Lines changed: 859 additions & 2 deletions

File tree

agent/src/hooks.py

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,86 @@ def _allow_response(reason: str = "permitted") -> dict:
244244
}
245245

246246

247+
# ABCA-815 layer 1 (workspace-integrity guard). The repo is ALREADY cloned and
248+
# checked out at the agent's cwd (``repo_dir``); the platform tracks that dir for
249+
# commit/branch/PR/push. Live on backgroundagent-dev the agent sometimes ran its
250+
# OWN ``gh repo clone``/``git clone`` of the SAME repo into a sibling dir
251+
# (``/workspace/<repo-name>``) and did all its work there, leaving repo_dir empty
252+
# → the platform saw no commits (delivery gate: ``deliverable=lost``) and, for a
253+
# #247 stacked child, the successor had no branch to stack on. The industry norm
254+
# is a filesystem sandbox that makes a divergent clone impossible; lacking that
255+
# (raw Bash + bypassPermissions + open GitHub egress), a harness-enforced deny of
256+
# the re-clone is the closest robust analog — and Claude Code hook denies fire
257+
# even under bypassPermissions and survive prompt-injection. We match ONLY a
258+
# re-clone of the TASK's OWN repo (not legitimate dependency/submodule clones),
259+
# keyed off the ``owner/repo`` slug in any of the URL forms gh/git accept.
260+
#
261+
# The clone verb must appear as an actual COMMAND — at the start of the string or
262+
# right after a shell separator (``&&``, ``||``, ``|``, ``;``, newline, ``(``) —
263+
# NOT as text buried inside a quoted argument. Live-caught on ABCA-856: a
264+
# ``gh pr create --body "…do not run gh repo clone…"`` was wrongly blocked because
265+
# the phrase appeared inside the PR body. Anchoring to command position (and, in
266+
# ``_is_self_reclone``, truncating at the first argument that carries free text)
267+
# removes that false positive while still catching every real re-clone.
268+
_CLONE_CMD_RE = re.compile(
269+
r"(?:^|[;&|\n(]|&&|\|\|)\s*(?:gh\s+repo\s+clone|git\s+(?:-C\s+\S+\s+)?clone)\b",
270+
re.IGNORECASE,
271+
)
272+
273+
# Arguments after which the rest of the command is free-form TEXT (a PR/issue/
274+
# commit body or title), where a mention of "gh repo clone" is prose, not a
275+
# command. We stop scanning for the clone verb at the first of these so a body
276+
# quoting the clone command can't trip the guard (ABCA-856 false positive).
277+
_FREE_TEXT_ARG_RE = re.compile(
278+
r"(?:^|\s)(?:-m|--message|--body|--body-file|-b|--title|-t|-F|--field|--raw-field)\b",
279+
re.IGNORECASE,
280+
)
281+
282+
283+
def _repo_slug_variants(repo_url: str) -> list[str]:
284+
"""Return the ``owner/repo`` slug forms a clone command could name.
285+
286+
``config.repo_url`` is the canonical ``owner/repo``. A clone command may
287+
reference it as ``owner/repo``, ``https://github.com/owner/repo(.git)``, or
288+
``git@github.com:owner/repo(.git)`` — all normalize to the same slug, so we
289+
just need the bare ``owner/repo`` (with and without ``.git``) to substring-
290+
match against the command text after normalization."""
291+
slug = (repo_url or "").strip().removesuffix(".git").strip("/").lower()
292+
if not slug:
293+
return []
294+
return [slug, f"{slug}.git"]
295+
296+
297+
def _is_self_reclone(command: str, repo_url: str) -> bool:
298+
"""True when *command* clones the task's OWN repo (any URL form).
299+
300+
Deliberately narrow on two axes so legitimate commands pass:
301+
* the clone verb must be in COMMAND position (start / after a shell
302+
separator), not inside a quoted argument — see ``_CLONE_CMD_RE``; and
303+
* the scan stops at the first free-text argument (``--body``/``-m``/…), so a
304+
PR/commit body that merely QUOTES "gh repo clone" is not treated as a
305+
clone (ABCA-856); and
306+
* a clone of a DIFFERENT repo (dependency, fixture, example) is allowed —
307+
only a re-clone of the TASK repo threatens the workspace invariant."""
308+
if not command or not repo_url:
309+
return False
310+
# Only consider the segment BEFORE any free-text argument — a --body/-m value
311+
# that quotes the clone command is prose, not an executed clone.
312+
m = _FREE_TEXT_ARG_RE.search(command)
313+
scan = command[: m.start()] if m else command
314+
if not _CLONE_CMD_RE.search(scan):
315+
return False
316+
variants = _repo_slug_variants(repo_url)
317+
if not variants:
318+
return False
319+
# Normalize the command's URL punctuation to the bare slug space: drop the
320+
# scheme/host and the scp-style ``git@host:`` prefix so ``.../owner/repo.git``
321+
# and ``git@github.com:owner/repo`` both reduce to a substring carrying
322+
# ``owner/repo``. Search the SAME pre-free-text segment.
323+
haystack = scan.lower().replace("git@github.com:", "github.com/")
324+
return any(v in haystack for v in variants)
325+
326+
247327
async def pre_tool_use_hook(
248328
hook_input: Any,
249329
tool_use_id: str | None,
@@ -255,6 +335,7 @@ async def pre_tool_use_hook(
255335
user_id: str | None = None,
256336
progress: Any = None,
257337
task_state_module: Any = None,
338+
repo_url: str | None = None,
258339
) -> dict:
259340
"""PreToolUse hook: three-outcome Cedar policy enforcement (§6.5).
260341
@@ -303,6 +384,28 @@ async def pre_tool_use_hook(
303384
log("WARN", f"PreToolUse hook received non-dict tool_input — denying {tool_name}")
304385
return _deny_response("tool input is not an object")
305386

387+
# ABCA-815 layer 1: block a Bash re-clone of the task's OWN repo before Cedar
388+
# eval. The repo is already checked out at the agent's cwd; a self-reclone
389+
# into a sibling dir strands the work off the platform-tracked branch (the
390+
# delivery gate then fails it as ``deliverable=lost``). Deny it here with a
391+
# redirect so the agent works in place instead. Scoped to the task repo only
392+
# (dependency/fixture clones pass). Fires even under bypassPermissions.
393+
if tool_name == "Bash" and repo_url:
394+
command = tool_input.get("command", "")
395+
if isinstance(command, str) and _is_self_reclone(command, repo_url):
396+
reason = (
397+
f"The repository '{repo_url}' is ALREADY cloned and checked out in "
398+
"your working directory — do NOT clone it again or work in another "
399+
"directory. Your commits must land in the current working directory "
400+
"so the platform can open the pull request on the correct branch. "
401+
"Remove the clone command and work in place (edit, commit, and push "
402+
"from where you already are)."
403+
)
404+
log("POLICY", f"DENIED self-reclone of {repo_url}: {command[:160]}")
405+
if trajectory:
406+
trajectory.write_policy_decision("Bash", False, "self_reclone_blocked", 0)
407+
return _deny_response(reason)
408+
306409
decision = engine.evaluate_tool_use(tool_name, tool_input)
307410

308411
# Telemetry: ALLOW "permitted" is the quiet happy path; everything else
@@ -1588,6 +1691,7 @@ def build_hook_matchers(
15881691
task_id: str = "",
15891692
progress: Any = None,
15901693
user_id: str = "",
1694+
repo_url: str = "",
15911695
) -> dict:
15921696
"""Build hook matchers dict for ClaudeAgentOptions.
15931697
@@ -1638,6 +1742,7 @@ async def _pre(
16381742
task_id=task_id or None,
16391743
user_id=user_id or None,
16401744
progress=progress,
1745+
repo_url=repo_url or None,
16411746
)
16421747
except Exception as exc:
16431748
log(

agent/src/pipeline.py

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
_extract_agent_notes,
3838
ensure_committed,
3939
ensure_pr,
40+
reconcile_agent_branch,
4041
verify_build,
4142
verify_lint,
4243
)
@@ -629,6 +630,94 @@ def _resolve_overall_task_status(
629630
return "error", err
630631

631632

633+
def _branch_has_new_commits(repo_dir: str, default_branch: str) -> bool:
634+
"""True if HEAD carries commits beyond ``origin/<default_branch>``.
635+
636+
The same ``origin/<default_branch>..HEAD`` diff ``ensure_pr`` consults to
637+
decide whether there is anything to open a PR from (see
638+
``post_hooks._ensure_pr``). Used by the ABCA-815 delivery gate to tell
639+
"a commit landed but the PR failed to open" (recoverable) from "no commit
640+
ever reached the branch — the work was lost". For a #247 A4 stacked child
641+
``default_branch`` IS its predecessor branch (``config.base_branch``), so
642+
this asks the right question: did this child add anything on top of its
643+
base? Best-effort — a git failure returns ``False`` (assume nothing landed,
644+
the conservative read for a gate whose job is to catch a lost deliverable)."""
645+
try:
646+
res = subprocess.run(
647+
["git", "log", f"origin/{default_branch}..HEAD", "--oneline"],
648+
cwd=repo_dir,
649+
check=False,
650+
capture_output=True,
651+
text=True,
652+
timeout=60,
653+
)
654+
except (OSError, subprocess.SubprocessError):
655+
return False
656+
return res.returncode == 0 and bool(res.stdout.strip())
657+
658+
659+
def _apply_delivery_gate(
660+
overall_status: str,
661+
result_error: str | None,
662+
*,
663+
workflow_read_only: bool,
664+
artifact_workflow: bool,
665+
needs_input: bool,
666+
ensure_pr_strategy: str,
667+
pr_url: str | None,
668+
commit_landed: bool,
669+
) -> tuple[str, str | None]:
670+
"""ABCA-815: fail a new-work coding task that reported success but shipped
671+
nothing (no PR AND no commit on its branch) — the deliverable was lost.
672+
673+
Live cause: a #247 A4 stacked child edited its files in a NESTED working tree
674+
(persistent-storage residue left the clone one level deep inside repo_dir),
675+
so every pipeline git op ran against the outer clean tree —
676+
``ensure_committed`` saw nothing, ``ensure_pr`` found no commits and skipped —
677+
yet the task reported COMPLETED/build_passed. That silent success poisoned the
678+
integration: the child's feature never reached the branch its successor
679+
stacked on. This gate turns that into a loud, retryable FAILED.
680+
681+
Mirrors the repo-LESS artifact gate (``run_task`` arm 2): a sanctioned no-op
682+
is EXPECTED to ship nothing, so this fires ONLY for create-strategy new work —
683+
* read-only (pr_review) / artifact (decompose) ship no PR by design;
684+
* push_resolve / resolve (pr_iteration) legitimately add no NEW PR, and a
685+
question-only iteration is handled via the ``code_changed=False``
686+
"answered" path — none MUST open a PR;
687+
* clarify-and-hold (needs_input) is the one intentional new_task no-op, and
688+
the caller forces it to success right after this gate.
689+
690+
``commit_landed`` (from :func:`_branch_has_new_commits`) distinguishes
691+
"a commit reached the branch but the PR failed to open" (``deliverable=no_pr``
692+
— recoverable, the work is safe on the branch) from "no commit ever landed"
693+
(``deliverable=lost`` — the work is gone) so the failure copy is honest.
694+
Returns the (possibly unchanged) ``(overall_status, result_error)``.
695+
"""
696+
delivery_expected = (
697+
not workflow_read_only
698+
and not artifact_workflow
699+
and not needs_input
700+
and ensure_pr_strategy == "create"
701+
)
702+
if not (overall_status == "success" and delivery_expected and not pr_url):
703+
return overall_status, result_error
704+
if commit_landed:
705+
reason = (
706+
"Task did not succeed (agent_status=success, deliverable=no_pr): a "
707+
"commit reached the branch but no PR was opened — the change is on the "
708+
"branch but was not delivered."
709+
)
710+
else:
711+
reason = (
712+
"Task did not succeed (agent_status=success, deliverable=lost): the "
713+
"coding task reported success but no commit reached the branch and no "
714+
"PR was opened — the agent's changes did not land in the task's "
715+
"repository."
716+
)
717+
log("WARN", f"ABCA-815 delivery gate: {reason}")
718+
return "error", reason
719+
720+
632721
def _compute_turns_completed(
633722
agent_status: str,
634723
turns_attempted: int | None,
@@ -1281,6 +1370,17 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None:
12811370
)
12821371
post_span.set_attribute("artifact.uri", artifact_uri or "")
12831372
else:
1373+
# ABCA-815 root cause: if the agent switched off the platform
1374+
# branch (it sometimes runs `git checkout -b <own-branch>` and
1375+
# commits/opens its PR there — live-caught on backgroundagent-dev),
1376+
# re-point the platform branch at the agent's HEAD so the
1377+
# safety-net commit, build verify, PR, and push below all run on
1378+
# the branch the platform tracks. No-op on the healthy case
1379+
# (agent stayed on the platform branch). Skip for read-only
1380+
# (no commit/PR to deliver). The delivery gate stays as the
1381+
# backstop if reconcile can't recover the work.
1382+
if not workflow_read_only:
1383+
reconcile_agent_branch(setup.repo_dir, setup.branch)
12841384
# Safety net: commit any uncommitted tracked changes (skip read-only tasks)
12851385
safety_committed = (
12861386
False if workflow_read_only else ensure_committed(setup.repo_dir)
@@ -1396,6 +1496,35 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None:
13961496
build_timed_out=build_timed_out,
13971497
build_infra_failed=build_infra_failed,
13981498
)
1499+
# ABCA-815 delivery gate: a create-strategy new-work task that
1500+
# reported success but opened no PR AND landed no commit shipped
1501+
# nothing — fail it loudly (retryable) instead of a false COMPLETED
1502+
# that would poison a #247 stacked DAG. The branch-diff is only run
1503+
# when the gate is actually in play (success + no pr_url) so a normal
1504+
# PR-producing run pays nothing. See :func:`_apply_delivery_gate`.
1505+
gate_in_play = (
1506+
overall_status == "success"
1507+
and not pr_url
1508+
and not workflow_read_only
1509+
and not artifact_workflow
1510+
and not needs_input
1511+
and ensure_pr_strategy == "create"
1512+
)
1513+
commit_landed = (
1514+
_branch_has_new_commits(setup.repo_dir, setup.default_branch)
1515+
if gate_in_play
1516+
else False
1517+
)
1518+
overall_status, result_error = _apply_delivery_gate(
1519+
overall_status,
1520+
result_error,
1521+
workflow_read_only=workflow_read_only,
1522+
artifact_workflow=artifact_workflow,
1523+
needs_input=needs_input,
1524+
ensure_pr_strategy=ensure_pr_strategy,
1525+
pr_url=pr_url,
1526+
commit_landed=commit_landed,
1527+
)
13991528
# Clarify-before-spend: a hold-for-input run is a SUCCESSFUL outcome
14001529
# (the agent did the right thing by asking), not a failure — the
14011530
# deliverable is the question. Force success + clear any error so the

agent/src/post_hooks.py

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,97 @@ def ensure_committed(repo_dir: str) -> bool:
324324
return False
325325

326326

327+
def _current_branch(repo_dir: str) -> str | None:
328+
"""Return the checked-out branch name, or None if detached / git fails."""
329+
try:
330+
res = subprocess.run(
331+
["git", "rev-parse", "--abbrev-ref", "HEAD"],
332+
cwd=repo_dir,
333+
check=False,
334+
capture_output=True,
335+
text=True,
336+
timeout=30,
337+
)
338+
except (OSError, subprocess.SubprocessError):
339+
return None
340+
if res.returncode != 0:
341+
return None
342+
name = res.stdout.strip()
343+
# "HEAD" is git's sentinel for a detached HEAD — not a branch name.
344+
return name or None if name != "HEAD" else None
345+
346+
347+
def reconcile_agent_branch(repo_dir: str, branch: str) -> bool:
348+
"""ABCA-815 root cause: make delivery tolerant of the agent switching off the
349+
platform-provided branch.
350+
351+
The platform checks out ``branch`` (``config.branch_name``, e.g.
352+
``bgagent/<task_id>/<slug>``) BEFORE the agent runs, and every delivery git op
353+
(ensure_committed / ensure_pr's commit-diff / ensure_pushed) is keyed to it.
354+
But the agent doesn't always stay there: live on backgroundagent-dev it
355+
sometimes ran ``git checkout -b <its-own-short-branch>``, committed + opened
356+
its own PR there, and left the platform branch empty. ensure_pr then saw no
357+
commits on ``branch`` → skipped → the task looked delivered-nothing (the
358+
ABCA-815 signature), and for a #247 stacked child the successor had no branch
359+
to stack on. It is NON-deterministic (the same task succeeds on a retry when
360+
the agent happens to stay put), so a prompt tweak alone can't be relied on.
361+
362+
This reconciles deterministically: if HEAD is on a DIFFERENT branch than the
363+
platform ``branch`` and that HEAD carries commits, fast-forward the platform
364+
branch to the agent's HEAD (``git branch -f`` + checkout) so all downstream
365+
delivery runs against the branch the platform tracks. The agent's commits are
366+
preserved verbatim — we only re-point the platform branch label at them.
367+
368+
Returns True if it moved the platform branch, False otherwise (already on it,
369+
detached HEAD with no branch to adopt, or a git failure — all handled by the
370+
existing ensure_committed/ensure_pr/delivery-gate chain). Best-effort: never
371+
raises, so a reconcile failure degrades to the pre-existing behavior."""
372+
head_branch = _current_branch(repo_dir)
373+
if head_branch is None or head_branch == branch:
374+
# On the platform branch already (the common, healthy case) or detached
375+
# with nothing to adopt — nothing to reconcile.
376+
return False
377+
log(
378+
"POST",
379+
f"Agent left the platform branch: HEAD is on '{head_branch}', expected "
380+
f"'{branch}'. Reconciling the platform branch to the agent's commits so "
381+
"the work is delivered on the tracked branch (ABCA-815).",
382+
)
383+
try:
384+
# Re-point the platform branch at the agent's HEAD (force: the platform
385+
# branch was created empty at setup, so this only ever fast-forwards it
386+
# to the work the agent actually did). Then check it out so ensure_committed
387+
# / ensure_pr / ensure_pushed all operate on the tracked branch.
388+
force_res = run_cmd(
389+
["git", "branch", "-f", branch, "HEAD"],
390+
label="reconcile-branch-force",
391+
cwd=repo_dir,
392+
check=False,
393+
)
394+
if force_res.returncode != 0:
395+
stderr = (force_res.stderr or "").strip()[:200]
396+
log("WARN", f"reconcile: git branch -f failed (exit {force_res.returncode}): {stderr}")
397+
return False
398+
checkout_res = run_cmd(
399+
["git", "checkout", branch],
400+
label="reconcile-branch-checkout",
401+
cwd=repo_dir,
402+
check=False,
403+
)
404+
if checkout_res.returncode != 0:
405+
stderr = (checkout_res.stderr or "").strip()[:200]
406+
log(
407+
"WARN",
408+
f"reconcile: checkout '{branch}' failed (exit {checkout_res.returncode}): {stderr}",
409+
)
410+
return False
411+
except (OSError, subprocess.SubprocessError) as e:
412+
log("WARN", f"reconcile: git op raised {type(e).__name__}: {e}")
413+
return False
414+
log("POST", f"Reconciled: platform branch '{branch}' now points at the agent's work")
415+
return True
416+
417+
327418
def ensure_pushed(repo_dir: str, branch: str) -> bool:
328419
"""Push the branch if there are unpushed commits."""
329420
result = subprocess.run(

0 commit comments

Comments
 (0)