fix(issue-21): review R1-R3 - no env as agent arguments on a reused pane, one jobs dir for a relative MAM_JOBS_DIR, stop keeps the audit-log root
Review 9a1adb46 (planner-reviewer-codex-01): - R1 [P1] lib.sh new-session: a forwarded binding reached the agent as arguments. With env flags the idle-pane branch prefixed `env K=V ...` to the command that `herdr agent start ... --kind <agent> -- <args>` passes to the agent executable. An idle pane is now reused only when no environment has to be set; otherwise the pane is created by pane split / workspace create, which take --env. The dead `env` prefix code is removed (its send-text fallback would have typed the same line). - R2 [P2] runner: a relative MAM_JOBS_DIR is resolved once against the control root, the way the wrapper (cwd=workspace_root) reads it, and that absolute dir is used for delegation, wait, report lookup and cleanup. - R3 [P2] stop: the row's control_plane.delegate_job_logs_dir is restored (exported unless the caller set DELEGATE_JOB_LOGS_DIR), so the cancellation history lands in the row's audit-log root. Tests: T3-1 and T3-3 now assert that the binding arrives as --env on the pane-creation calls and that no env/K=V token follows `--` in agent start (fresh and reused pane); T3-4c (audit root on stop) and T2-15 (relative MAM_JOBS_DIR) are new. The design doc's compatibility table and a create comment no longer describe the removed idle-pane env path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017R3Eg7MEpSWnm1oB9ZPP6G
This commit is contained in:
1 parent
b61c8b177a
commit
af8a54c028
6 files changed
+113
-18
No files matched your search
@@ -775,14 +775,15 @@ except Exception:
|
||||
pass
|
||||
" 2>/dev/null || echo "")
|
||||
|
||||
if [ -n "$idle_pane" ]; then
|
||||
# An idle pane is reused only when no environment has to be set: the
|
||||
# pane's shell is already running, and `agent start ... -- <args>` hands
|
||||
# every token after -- to the agent executable (--kind), so an
|
||||
# `env K=V` prefix would reach the agent as arguments, not as its
|
||||
# environment (Issue #21 R1). With env flags, fall through to
|
||||
# pane split / workspace create, which take --env.
|
||||
if [ -n "$idle_pane" ] && [ -z "$env_flags" ]; then
|
||||
existing_ws="$cand_ws"
|
||||
target_pane="$idle_pane"
|
||||
if [ -n "$env_flags" ] && [ -n "$final_cmd" ]; then
|
||||
idle_env="${env_flags//--env /}"
|
||||
idle_env="${idle_env:+$idle_env }HERDR_WORKSPACE_ID=$existing_ws"
|
||||
final_cmd="env $idle_env $final_cmd"
|
||||
fi
|
||||
break
|
||||
fi
|
||||
|
||||
|
||||
@@ -64,7 +64,9 @@ class LoopRunner:
|
||||
# diff, base and head come from; everything else - wrapper, registry,
|
||||
# checkpoint, loop guard, promotion, cleanup - stays on the control
|
||||
# root (workspace_root).
|
||||
self.jobs_dir = Path(os.environ.get("MAM_JOBS_DIR") or self.mam_dir / "jobs")
|
||||
# A relative MAM_JOBS_DIR is taken against the control root, as the
|
||||
# wrapper (run with cwd=workspace_root) takes it.
|
||||
self.jobs_dir = (self.workspace_root / Path(os.environ.get("MAM_JOBS_DIR") or self.mam_dir / "jobs")).resolve()
|
||||
self.worktree_root: Path = self.workspace_root
|
||||
self.worktree_given = False
|
||||
self.base_ref: Optional[str] = None
|
||||
|
||||
@@ -205,7 +205,8 @@ fi
|
||||
# Issue #21 (item 3): the control-plane binding. Persisted on the row always;
|
||||
# forwarded to the pane (leading K=V tokens that lib.sh's new-session turns
|
||||
# into --env) only when it differs from what --workspace alone implies.
|
||||
# lib.sh splits those flags unquoted (and the idle-pane path evals them), so a
|
||||
# lib.sh splits those flags unquoted (and, with env flags, never reuses an
|
||||
# idle pane - `agent start --` would pass them to the agent as arguments), so a
|
||||
# binding with whitespace that would have to be forwarded is refused.
|
||||
CP_JSON="$(mam_control_plane_json "$WORKSPACE" 2>/dev/null || true)"
|
||||
CP_PREFIX=""
|
||||
|
||||
@@ -133,7 +133,8 @@ for s in d.get('herdr_sessions', []):
|
||||
cwd = (s.get('pane') or {}).get('cwd', '')
|
||||
jid = s.get('delegate_job_id', '') or ''
|
||||
cp = s.get('control_plane') or {}
|
||||
print(json.dumps({'cwd': cwd, 'job_id': jid, 'mam_jobs_dir': cp.get('mam_jobs_dir', '') or ''}))
|
||||
print(json.dumps({'cwd': cwd, 'job_id': jid, 'mam_jobs_dir': cp.get('mam_jobs_dir', '') or '',
|
||||
'delegate_job_logs_dir': cp.get('delegate_job_logs_dir', '') or ''}))
|
||||
sys.exit(0)
|
||||
sys.exit(7)
|
||||
") || {
|
||||
@@ -150,6 +151,12 @@ ROW_JOBS_DIR=$(printf '%s' "$MAPPED_DATA" | python3 -c 'import sys,json; print(j
|
||||
if [ -z "${MAM_JOBS_DIR:-}" ] && [ -n "$ROW_JOBS_DIR" ]; then
|
||||
export MAM_JOBS_DIR="$ROW_JOBS_DIR"
|
||||
fi
|
||||
# Same for the audit log (R3): the cancellation history belongs next to the
|
||||
# registry the row names, not under $WORKSPACE_ROOT/.mam/delegate_job_logs.
|
||||
ROW_LOGS_DIR=$(printf '%s' "$MAPPED_DATA" | python3 -c 'import sys,json; print(json.load(sys.stdin).get("delegate_job_logs_dir",""))')
|
||||
if [ -z "${DELEGATE_JOB_LOGS_DIR:-}" ] && [ -n "$ROW_LOGS_DIR" ]; then
|
||||
export DELEGATE_JOB_LOGS_DIR="$ROW_LOGS_DIR"
|
||||
fi
|
||||
DELEGATE_JOB_TOKEN=""
|
||||
DELEGATE_JOB_ATTEMPT=""
|
||||
if [ -n "$DELEGATE_JOB_ID" ]; then
|
||||
|
||||
Reference in new issue
Block a user