From 2bcddd8fdf9ba15b3c924e93b48b0aacf2beb74e Mon Sep 17 00:00:00 2001 From: Jonathan Bragg Date: Thu, 9 Apr 2026 14:09:38 -0700 Subject: [PATCH 1/2] Fix pier verify silently dropping trajectory data --- README.md | 28 +++-- pier/cli.py | 202 +++++++++++++++++++++++-------- pier/harbor_bridge.py | 101 +++------------- pier/tests/test_harbor_bridge.py | 121 +----------------- pier/tests/test_pier_cli.py | 184 +++++++++++++++++++++++----- 5 files changed, 342 insertions(+), 294 deletions(-) diff --git a/README.md b/README.md index e00e0eb..9fad81a 100644 --- a/README.md +++ b/README.md @@ -33,7 +33,7 @@ pier skills # install task skills for your agent - `--agent` installs a [supported coding agent](https://github.com/laude-institute/harbor) and registers skills from `skills_dir` in task.toml. Optional — without it you get a plain workspace. To install multiple agents, run `pier start --agent ` again from the workspace. - `--image` accepts any Docker image — a stock OS, a project image with tools pre-installed, etc. -- `--no-mount` keeps files inside the container only (no bind-mount to host). `pier stop` copies files back. +- `--no-mount` keeps workspace files inside the container only (no bind-mount to host). `pier stop` copies files back. Note: Harbor's internal mounts (agent logs, verifier output) still write to the host under `.pier/`. - `-f` / `--force` allows starting in a non-empty directory. ### Work in the workspace @@ -67,8 +67,8 @@ pier verify # run tests/test.sh, print reward, save results and traj Without a Harbor task (e.g. `--image` mode), save the trajectory with `pier capture`: ```bash -pier capture # save trajectory to .pier/trials/ -pier capture --session-dir # from any agent session outside pier +pier capture # save trajectory to .pier/trials/ +pier capture --session-dir -a claude-code # from any agent session outside pier ``` Browse, export, and review: @@ -104,14 +104,14 @@ cp -r harbor/examples/tasks/hello-world ./tasks/my-task ### `pier capture` -Extract the agent's trajectory for the current workspace. No Harbor task required. +Extract the agent's trajectory (conversation, tool use, and cost data) for the current workspace. No Harbor task required. ```bash -pier capture --session-dir -pier capture --session-dir PATH -a claude-code # specify agent explicitly +pier capture # container mode with registered agent +pier capture --session-dir -a claude-code # host mode or external session ``` -In container mode (with `--agent` installed), the session directory is detected automatically — no `--session-dir` needed. Outside a container, `--session-dir` is required. The agent type is auto-detected from the session contents; use `-a` to override. +In container mode with a registered agent (`pier start --agent`), the trajectory is extracted automatically. Otherwise, pass `--session-dir` and `-a` to specify the session location and agent. In container mode, `--session-dir` refers to a path inside the container. ### `pier traces` @@ -149,7 +149,7 @@ pier start # restart a stopped container - `--mounts-json` adds volume mounts as a JSON array (e.g., `--mounts-json '["./skills:/opt/skills:ro"]'`). - `-e` passes container-mode environment variables in `KEY=VALUE` format (repeatable). Stored in the session and forwarded on every `pier exec`. - `--env-file` loads container-mode environment variables from a `.env` file. Same behavior as `-e` for each line. -- `--no-mount` keeps files inside the container only (no bind-mount to host). `pier stop` copies files back. +- `--no-mount` keeps workspace files inside the container only (no bind-mount to host). `pier stop` copies files back. Note: Harbor's internal mounts (agent logs, verifier output) still write to the host under `.pier/`. - `-f` / `--force` allows starting in a non-empty directory. - `--host` skips the container (workspace only). - `--agent` installs a coding agent. To install additional agents, run `pier start --agent ` again from the workspace. When `task_path` is omitted, it operates on the current workspace. @@ -173,11 +173,13 @@ pier exec -d -- quarto preview --port 8888 --host 0.0.0.0 --no-browse Run the verifier and report the reward (requires a Harbor task). Each run creates a timestamped directory under `/.pier/trials/` in Harbor's trial format. ```bash -pier verify +pier verify # uses agent from pier start --agent ``` -- **Container mode**: uses Harbor's `Verifier` (same verifier as `harbor run`). The agent's trajectory is extracted automatically when an agent is installed. -- **Host mode**: spins up a temporary container to run the verifier, then tears it down. Pass `--session-dir` to capture the trajectory. +The agent is inferred from the session (set by `pier start --agent`). If no agent is registered, the verifier still runs and saves the reward but the trajectory (conversation, tool use, and cost data) is skipped. + +- **Container mode**: uses Harbor's `Verifier` (same verifier as `harbor run`). The trajectory is captured automatically when an agent is registered. For unregistered agents (e.g. baked into the image), pass `--session-dir` (container path) and `-a`. +- **Host mode**: spins up a temporary container to run the verifier, then tears it down. Pass `--session-dir` and `-a` to capture the trajectory. ### `pier stop` @@ -221,6 +223,10 @@ Install task skills for your coding agent (host mode only). Reads `skills_dir` f ## Development +### Architecture + +Pier stays **agent-agnostic** — it should not need changes when Harbor adds a new agent. Agent-specific details (session layouts, env vars, detection heuristics) belong in Harbor, not pier. `pier/harbor_bridge.py` is the thin boundary between the two; when it must contain agent-specific code, it should be treated as temporary scaffolding until Harbor exposes the right API. `pier/cli.py` should never reference individual agent names. + ```bash git clone https://github.com/allenai/pier && cd pier make check # run tests, lint, and typecheck diff --git a/pier/cli.py b/pier/cli.py index 36b9c94..42681aa 100644 --- a/pier/cli.py +++ b/pier/cli.py @@ -424,6 +424,23 @@ def _print_reward(reward: dict) -> None: click.echo(f" {k}: {v}") +def _copy_session_from_container( + harbor_session_id: str, container_path: str, dest: Path +) -> None: + """Copy an agent session directory from a running container to the host.""" + container = harbor_bridge.get_container_name(harbor_session_id) + result = subprocess.run( + ["docker", "cp", f"{container}:{container_path}", str(dest)], + capture_output=True, + ) + if result.returncode != 0: + raise click.ClickException( + f"Failed to copy {container_path} from container.\n" + f" {result.stderr.decode().strip()}" + ) + click.echo("Copied agent session from container.") + + def _assemble_trial_output( trial_dir: Path, sess: dict, @@ -433,31 +450,39 @@ def _assemble_trial_output( workspace: Path, agent: str | None, session_dir: str | None, + *, + agent_context: dict | None = None, ) -> None: """Assemble trial directory with trajectory and optional agent logs.""" from pier.trajectory import assemble_trial - agent_context = None - - if session_dir: - # Auto-detect agent from session_dir if not specified + if agent_context: + click.echo("Agent trajectory extracted.") + elif session_dir: if not agent: - agent = harbor_bridge.detect_agent_from_session_dir(Path(session_dir)) - if agent: - click.echo(f"Detected agent: {agent}") - - if agent: - agent_context = harbor_bridge.extract_agent_logs( - agent, - Path(session_dir), - trial_dir / "agent", + raise click.ClickException( + "Pass -a/--agent with --session-dir to specify the agent." ) - if agent_context: - click.echo("Agent trajectory extracted.") - else: - click.echo("Warning: trajectory extraction returned no data.", err=True) + agent_context = harbor_bridge.extract_agent_logs( + agent, + Path(session_dir), + trial_dir / "agent", + ) + if agent_context: + click.echo("Agent trajectory extracted.") + else: + click.echo("Warning: trajectory extraction returned no data.", err=True) elif agent: - click.echo(f"Tip: pass --session-dir to extract {agent} trajectory.") + click.echo(f"Tip: pass --session-dir to capture {agent} trajectory.") + else: + msg = ( + "No agent specified — reward saved, but trajectory " + "(conversation, tool use, cost) was not captured.\n" + " To include it: pier verify --agent --session-dir " + ) + if sess.get("mode") == "container": + msg += "\n To auto-detect next time: pier start --agent " + click.echo(msg) label = _workspace_label(workspace) assemble_trial( @@ -703,7 +728,11 @@ def start( if host: if agent: - click.echo("Hint: --agent is for container mode.") + raise click.ClickException( + "--agent is for container mode (installs the agent and configures " + "session capture). In host mode, agents run directly on your machine.\n" + " To capture conversation data: pass --session-dir and -a on verify/capture" + ) _start_host(task_dir, workspace) else: _start_container( @@ -716,6 +745,11 @@ def start( no_mount=no_mount, ) + if not host and not agent: + click.echo( + "\nTip: pier start --agent to install an agent and enable trajectory capture." + ) + def _start_existing(agent: str | None) -> None: """Operate on an existing workspace (no task_path given). @@ -1092,8 +1126,7 @@ def _exec_host(sess: dict, workspace: Path, command: list[str]) -> None: @click.option( "--session-dir", default=None, - type=click.Path(exists=True, file_okay=False), - help="Agent session/log directory for trajectory extraction.", + help="Agent session/log directory (container path in container mode, host path otherwise).", ) def verify( trial_dir: str | None, @@ -1102,24 +1135,59 @@ def verify( ) -> None: """Run the verifier for the current workspace. - Executes the verifier and reports the reward. Optionally assembles - a trial directory with trajectory data. + Executes the verifier and reports the reward. When an agent is known + (from the session or ``--agent``), trajectory and cost data are captured. """ sess, ws = _resolve_workspace() + # When --session-dir is explicit, require -a (don't infer — the session + # files might be from a different agent than the one registered). + if session_dir and not agent: + agents = sess.get("agents", []) + msg = "Pass -a/--agent with --session-dir to specify the agent." + if len(agents) == 1: + msg += ( + f"\n Or omit --session-dir to use {agents[0]}'s session automatically." + ) + raise click.ClickException(msg) + # Default agent from session if not explicitly provided if not agent: agents = sess.get("agents", []) if len(agents) == 1: agent = agents[0] + click.echo(f"Using agent from session: {agent}") + elif agents: + names = ", ".join(agents) + raise click.ClickException( + f"Multiple agents registered ({names}). " + f"Pass -a/--agent to select one.\n" + f" e.g. pier verify --agent {agents[0]}" + ) + + # Validate --session-dir in non-container mode (host path must exist) + if session_dir and sess.get("mode") != "container": + p = Path(session_dir) + if not p.is_dir(): + raise click.ClickException( + f"--session-dir {session_dir!r} is not a directory." + ) if sess.get("mode") == "host": _verify_host( - sess, ws, trial_dir=trial_dir, agent=agent, session_dir=session_dir + sess, + ws, + trial_dir=trial_dir, + agent=agent, + session_dir=session_dir, ) else: _verify_container( - sess, ws, trial_dir=trial_dir, agent=agent, session_dir=session_dir + sess, + ws, + trial_dir=trial_dir, + agent=agent, + session_dir=session_dir, ) @@ -1179,15 +1247,26 @@ def _verify_container( _print_reward(reward) - # Container mode: auto-detect agent session dir from the mounted logs + # In container mode, agent logs are already in Harbor's expected layout + # under harbor_td/agent/ — let Harbor find sessions directly rather than + # reimplementing its detection logic in pier. + container_agent_context = None if agent and not session_dir: - container_session = harbor_bridge.find_container_agent_session_dir( + click.echo(f"Extracting {agent} trajectory from container logs.") + container_agent_context = harbor_bridge.extract_agent_context( agent, harbor_td / "agent" ) - if container_session: - session_dir = str(container_session) - click.echo(f"Extracting {agent} trajectory from container logs.") - click.echo(" (override with --session-dir to use a different session)") + if not container_agent_context: + click.echo( + "Warning: trajectory extraction returned no data. " + "Try --session-dir to specify the session location.", + err=True, + ) + elif agent and session_dir: + # --session-dir in container mode is a path inside the container. + host_session = verify_trial_dir / "agent_session" + _copy_session_from_container(hsid, session_dir, host_session) + session_dir = str(host_session) _assemble_trial_output( verify_trial_dir, @@ -1198,6 +1277,7 @@ def _verify_container( workspace, agent, session_dir, + agent_context=container_agent_context, ) @@ -1447,13 +1527,12 @@ def _install_skills_from_dir(skills_dir: Path, workspace: Path) -> None: "-a", "--agent", default=None, - help="Agent name (e.g. claude-code). Auto-detected if omitted.", + help="Agent name (e.g. claude-code). Inferred from session if omitted; required with --session-dir.", ) @click.option( "--session-dir", default=None, - type=click.Path(exists=True, file_okay=False), - help="Agent session/log directory. Auto-discovered if omitted.", + help="Agent session/log directory (container path in container mode, host path otherwise).", ) def capture(agent: str | None, session_dir: str | None) -> None: """Capture the agent's trajectory for the current workspace. @@ -1477,11 +1556,30 @@ def capture(agent: str | None, session_dir: str | None) -> None: # Discover session directory resolved_session_dir: Path | None = None + container_agent_context = None + _container_session_tmpdir: tempfile.TemporaryDirectory | None = None - if session_dir: - resolved_session_dir = Path(session_dir).resolve() + if session_dir and sess and sess.get("mode") == "container": + # --session-dir in container mode is a path inside the container. + if not agent: + raise click.ClickException( + "Pass -a/--agent with --session-dir to specify the agent." + ) + hsid = _get_hsid(sess, ws) + _container_session_tmpdir = tempfile.TemporaryDirectory() + host_session = Path(_container_session_tmpdir.name) / "session" + _copy_session_from_container(hsid, session_dir, host_session) + resolved_session_dir = host_session + elif session_dir: + p = Path(session_dir) + if not p.is_dir(): + raise click.ClickException( + f"--session-dir {session_dir!r} is not a directory." + ) + resolved_session_dir = p.resolve() elif sess and sess.get("mode") == "container": - # Container mode: look inside the container's agent log directory + # Container mode: agent logs are already in Harbor's expected layout. + # Let Harbor find sessions directly via extract_agent_context. if not agent: agents = sess.get("agents", []) if len(agents) == 1: @@ -1493,13 +1591,18 @@ def capture(agent: str | None, session_dir: str | None) -> None: ) if agent: harbor_td = _harbor_trial_dir(ws) - resolved_session_dir = harbor_bridge.find_container_agent_session_dir( + click.echo(f"Extracting {agent} trajectory from container logs.") + container_agent_context = harbor_bridge.extract_agent_context( agent, harbor_td / "agent" ) - if resolved_session_dir is None: + if not container_agent_context: + raise click.ClickException( + "Could not extract agent session from container.\n" + "Pass --session-dir to specify explicitly." + ) + else: raise click.ClickException( - "Could not find agent session files in container.\n" - "Pass --session-dir to specify explicitly." + "No agent registered. Pass -a/--agent to specify one." ) else: raise click.ClickException( @@ -1507,16 +1610,12 @@ def capture(agent: str | None, session_dir: str | None) -> None: " pier capture --session-dir ~/.claude/projects/" ) - # Auto-detect agent if not specified - if not agent: - agent = harbor_bridge.detect_agent_from_session_dir(resolved_session_dir) - if not agent: - raise click.ClickException( - f"Could not detect agent from {resolved_session_dir}. Specify with -a." - ) - click.echo(f"Detected agent: {agent}") + if not agent and resolved_session_dir: + raise click.ClickException( + "Pass -a/--agent with --session-dir to specify the agent." + ) - # Create trial directory and assemble (no reward — capture only) + # Create trial directory only after all validation passes. trial_dir = _new_trial_dir(ws) now = datetime.now(timezone.utc) fake_sess = sess or {"task_dir": str(ws), "task_ref": ws.name} @@ -1528,7 +1627,8 @@ def capture(agent: str | None, session_dir: str | None) -> None: now, ws, agent, - str(resolved_session_dir), + str(resolved_session_dir) if resolved_session_dir else None, + agent_context=container_agent_context, ) diff --git a/pier/harbor_bridge.py b/pier/harbor_bridge.py index a8470f8..309ac9c 100644 --- a/pier/harbor_bridge.py +++ b/pier/harbor_bridge.py @@ -643,7 +643,7 @@ def build_trial_result_json( agent_cfg = AgentConfig(name=agent_name) if agent_name else AgentConfig() config = TrialConfig(task=task_config, trial_name=session_name, agent=agent_cfg) - agent_info = AgentInfo(name=agent_name or "human", version="unknown") + agent_info = AgentInfo(name=agent_name or "unknown", version="unknown") verifier_result = VerifierResult(rewards=reward) if reward else None @@ -749,84 +749,6 @@ def _bridge_claude_code(session_dir: Path, logs_dir: Path) -> None: "claude-code": _bridge_claude_code, } -# Heuristics to detect which agent produced a session directory. -# Each entry maps an agent name to a predicate on the session_dir. -_AGENT_DETECT: dict[str, Callable[[Path], bool]] = { - "claude-code": lambda d: any(d.glob("*.jsonl")), -} - - -def detect_agent_from_session_dir(session_dir: Path) -> str | None: - """Guess the agent name from the contents of a session directory. - - Returns the agent name (e.g. ``"claude-code"``) or None if no known - pattern matches. - """ - for agent_name, check in _AGENT_DETECT.items(): - try: - if check(session_dir): - return agent_name - except OSError: - continue - return None - - -def find_container_agent_session_dir( - agent_name: str, harbor_agent_dir: Path -) -> Path | None: - """Find an agent's session directory within the container's mounted agent dir. - - When CLAUDE_CONFIG_DIR is set during ``pier exec``, Claude writes session - JSONL to ``harbor_agent_dir/sessions/projects//``. This function - finds that project directory so it can be passed to ``extract_agent_logs``. - - Returns None if the agent isn't supported or no unambiguous session is found. - """ - detect = _AGENT_DETECT.get(agent_name) - direct_log_agents = { - "cursor-cli": "cursor-cli.txt", - "gemini-cli": "gemini-cli.trajectory.json", - "kimi-cli": "kimi-cli.txt", - "opencode": "opencode.txt", - } - if agent_name == "claude-code" and detect: - projects_dir = harbor_agent_dir / "sessions" / "projects" - if projects_dir.is_dir(): - project_dirs = [ - d for d in projects_dir.iterdir() if d.is_dir() and detect(d) - ] - if len(project_dirs) == 1: - return project_dirs[0] - if len(project_dirs) > 1: - logger.warning( - "Multiple Claude session dirs found in %s; " - "pass --session-dir explicitly", - projects_dir, - ) - elif agent_name == "codex": - sessions_dir = harbor_agent_dir / "sessions" - if sessions_dir.is_dir(): - session_dirs = [d for d in sessions_dir.rglob("*") if d.is_dir()] - if session_dirs: - max_depth = max(len(d.parts) for d in session_dirs) - session_dirs = [d for d in session_dirs if len(d.parts) == max_depth] - if len(session_dirs) == 1: - return session_dirs[0] - if len(session_dirs) > 1: - logger.warning( - "Multiple Codex session dirs found in %s; " - "pass --session-dir explicitly", - sessions_dir, - ) - elif agent_name == "qwen-coder": - sessions_dir = harbor_agent_dir / "qwen-sessions" - if sessions_dir.is_dir() and any(sessions_dir.rglob("*.jsonl")): - return harbor_agent_dir - elif agent_name in direct_log_agents: - if (harbor_agent_dir / direct_log_agents[agent_name]).exists(): - return harbor_agent_dir - return None - def get_agent_exec_env(agent_name: str) -> tuple[dict[str, str], str]: """Return env vars and PATH prefix needed to run an agent interactively. @@ -954,10 +876,6 @@ def extract_agent_logs( Dict with cost_usd, n_input_tokens, etc. — or None if extraction failed. """ - from harbor.agents.factory import AgentFactory - from harbor.models.agent.context import AgentContext - from harbor.models.agent.name import AgentName - bridge = _AGENT_BRIDGE.get(agent_name) if bridge is not None: bridge(session_dir, logs_dir) @@ -970,6 +888,21 @@ def extract_agent_logs( if not link.exists(): link.symlink_to(item.resolve()) + return extract_agent_context(agent_name, logs_dir) + + +def extract_agent_context(agent_name: str, logs_dir: Path) -> dict | None: + """Extract trajectory and usage from an agent's logs directory. + + Expects ``logs_dir`` to already be in the layout Harbor's agent reader + expects (e.g. container mode where CLAUDE_CONFIG_DIR points into the + trial's agent dir). :func:`extract_agent_logs` calls this after first + bridging an external session directory into the right layout. + """ + from harbor.agents.factory import AgentFactory + from harbor.models.agent.context import AgentContext + from harbor.models.agent.name import AgentName + agent = AgentFactory.create_agent_from_name( AgentName(agent_name), logs_dir=logs_dir ) @@ -981,7 +914,7 @@ def extract_agent_logs( logger.warning( "Failed to extract logs for %r from %s", agent_name, - session_dir, + logs_dir, exc_info=True, ) return None diff --git a/pier/tests/test_harbor_bridge.py b/pier/tests/test_harbor_bridge.py index c9ff74b..e184547 100644 --- a/pier/tests/test_harbor_bridge.py +++ b/pier/tests/test_harbor_bridge.py @@ -12,8 +12,6 @@ _get_dockerfile_workdir, _write_mounts_compose, build_trial_result_json, - detect_agent_from_session_dir, - find_container_agent_session_dir, get_compose_project, get_agent_exec_env, get_container_name, @@ -188,14 +186,15 @@ def test_produces_valid_harbor_trial_result(self, tmp_path: Path): assert result.started_at == start assert result.finished_at == end - def test_defaults_agent_to_human(self, tmp_path: Path): + def test_defaults_agent_to_unknown(self, tmp_path: Path): + """When no agent is specified, agent_info.name defaults to 'unknown'.""" task_dir = _make_task_dir(tmp_path) result_json = build_trial_result_json(task_dir, "my-task", "s", {"reward": 1.0}) from harbor.models.trial.result import TrialResult result = TrialResult.model_validate_json(result_json) - assert result.agent_info.name == "human" + assert result.agent_info.name == "unknown" def test_scanner_discovers_pier_layout(self, tmp_path: Path): """Verify that Harbor's JobScanner finds trials in pier's .pier/ layout.""" @@ -250,120 +249,6 @@ def test_container_name_with_uppercase(self): assert get_container_name("Pier-WS") == "pier-ws-main-1" -class TestDetectAgentFromSessionDir: - def test_detects_claude_code(self, tmp_path: Path): - session = tmp_path / "session" - session.mkdir() - (session / "log.jsonl").write_text("{}\n") - assert detect_agent_from_session_dir(session) == "claude-code" - - def test_returns_none_for_empty_dir(self, tmp_path: Path): - session = tmp_path / "session" - session.mkdir() - assert detect_agent_from_session_dir(session) is None - - def test_returns_none_for_nonexistent_dir(self, tmp_path: Path): - session = tmp_path / "nonexistent" - assert detect_agent_from_session_dir(session) is None - - -class TestFindContainerAgentSessionDir: - def test_finds_single_claude_session(self, tmp_path: Path): - agent_dir = tmp_path / "agent" - project = agent_dir / "sessions" / "projects" / "abc123" - project.mkdir(parents=True) - (project / "session.jsonl").write_text("{}\n") - - result = find_container_agent_session_dir("claude-code", agent_dir) - assert result == project - - def test_returns_none_for_no_sessions(self, tmp_path: Path): - agent_dir = tmp_path / "agent" - agent_dir.mkdir() - assert find_container_agent_session_dir("claude-code", agent_dir) is None - - def test_returns_none_for_empty_projects(self, tmp_path: Path): - agent_dir = tmp_path / "agent" - projects = agent_dir / "sessions" / "projects" - projects.mkdir(parents=True) - assert find_container_agent_session_dir("claude-code", agent_dir) is None - - def test_returns_none_for_multiple_sessions(self, tmp_path: Path): - agent_dir = tmp_path / "agent" - projects = agent_dir / "sessions" / "projects" - for name in ("proj1", "proj2"): - d = projects / name - d.mkdir(parents=True) - (d / "session.jsonl").write_text("{}\n") - - result = find_container_agent_session_dir("claude-code", agent_dir) - assert result is None - - def test_returns_none_for_unknown_agent(self, tmp_path: Path): - agent_dir = tmp_path / "agent" - agent_dir.mkdir() - assert find_container_agent_session_dir("unknown-agent", agent_dir) is None - - def test_ignores_dirs_without_jsonl(self, tmp_path: Path): - agent_dir = tmp_path / "agent" - projects = agent_dir / "sessions" / "projects" - # One dir with jsonl, one without - real = projects / "real" - real.mkdir(parents=True) - (real / "session.jsonl").write_text("{}\n") - empty = projects / "empty" - empty.mkdir() - - result = find_container_agent_session_dir("claude-code", agent_dir) - assert result == real - - def test_finds_single_codex_session(self, tmp_path: Path): - agent_dir = tmp_path / "agent" - session = agent_dir / "sessions" / "2026" / "04" / "codex-session" - session.mkdir(parents=True) - - result = find_container_agent_session_dir("codex", agent_dir) - assert result == session - - def test_codex_returns_none_for_multiple_sessions(self, tmp_path: Path): - agent_dir = tmp_path / "agent" - (agent_dir / "sessions" / "a" / "one").mkdir(parents=True) - (agent_dir / "sessions" / "b" / "two").mkdir(parents=True) - - result = find_container_agent_session_dir("codex", agent_dir) - assert result is None - - @pytest.mark.parametrize( - ("agent_name", "filename"), - [ - ("cursor-cli", "cursor-cli.txt"), - ("gemini-cli", "gemini-cli.trajectory.json"), - ("kimi-cli", "kimi-cli.txt"), - ("opencode", "opencode.txt"), - ], - ) - def test_finds_direct_log_agents( - self, tmp_path: Path, agent_name: str, filename: str - ): - agent_dir = tmp_path / "agent" - agent_dir.mkdir() - (agent_dir / filename).write_text("log\n") - - result = find_container_agent_session_dir(agent_name, agent_dir) - - assert result == agent_dir - - def test_finds_qwen_coder_session_root(self, tmp_path: Path): - agent_dir = tmp_path / "agent" - session = agent_dir / "qwen-sessions" / "project-a" - session.mkdir(parents=True) - (session / "events.jsonl").write_text("{}\n") - - result = find_container_agent_session_dir("qwen-coder", agent_dir) - - assert result == agent_dir - - class TestBridgeClaudeCode: def test_creates_symlink_structure(self, tmp_path: Path): session_dir = tmp_path / "my-session" diff --git a/pier/tests/test_pier_cli.py b/pier/tests/test_pier_cli.py index c96df13..b97fe3c 100644 --- a/pier/tests/test_pier_cli.py +++ b/pier/tests/test_pier_cli.py @@ -1553,6 +1553,7 @@ def test_verify_defaults_agent_from_session( catch_exceptions=False, ) assert result.exit_code == 0 + assert "using agent from session: claude-code" in result.output.lower() args = mock_assemble.call_args[0] assert args[6] == "claude-code" # agent defaulted from session @@ -1584,6 +1585,117 @@ def test_verify_agent_flag_overrides_session( assert args[6] == "codex" # explicit flag wins +@patch("pier.harbor_bridge.verify_environment", return_value={"reward": 1.0}) +@patch("pier.harbor_bridge.is_environment_running", return_value=True) +def test_verify_multi_agent_session_errors( + mock_running, + mock_verify, + runner, + index_path, + task_dir, + tmp_path, +): + """Multiple agents in session errors before running verifier.""" + ws = tmp_path / "ws" + ws.mkdir() + sess = _container_session(task_dir=str(task_dir), agents=["claude-code", "codex"]) + _write_session(ws, sess, index_path) + result = runner.invoke(cli, ["verify"]) + assert result.exit_code != 0 + assert "multiple agents" in result.output.lower() + mock_verify.assert_not_called() + + +def test_verify_host_no_agent_proceeds(runner, index_path, task_dir, tmp_path): + """Host verify without --agent saves reward with no trajectory.""" + ws = tmp_path / "ws" + ws.mkdir() + _write_session( + ws, + { + "mode": "host", + "task_dir": str(task_dir), + "task_ref": "my-task", + "started_at": "2026-02-24T12:00:00+00:00", + }, + index_path, + ) + + def fake_verify(task_dir, workspace, trial_dir): + from datetime import datetime, timezone + + vdir = trial_dir / "verifier" + vdir.mkdir(parents=True, exist_ok=True) + (vdir / "reward.json").write_text('{"reward": 1.0}') + now = datetime.now(timezone.utc) + return {"reward": 1.0}, now, now + + with patch("pier.cli._verify_host_in_container", side_effect=fake_verify): + result = runner.invoke(cli, ["verify"], catch_exceptions=False) + assert result.exit_code == 0 + assert "no agent specified" in result.output.lower() + + +@patch("pier.harbor_bridge.verify_environment", return_value={"reward": 1.0}) +@patch("pier.harbor_bridge.is_environment_running", return_value=True) +def test_verify_container_no_agent_saves_reward( + mock_running, + mock_verify, + runner, + index_path, + task_dir, + tmp_path, +): + """Container verify with no agent saves reward but skips trajectory.""" + ws = tmp_path / "ws" + ws.mkdir() + _write_session( + ws, _container_session(task_dir=str(task_dir), agents=[]), index_path + ) + result = runner.invoke(cli, ["verify"], catch_exceptions=False) + assert result.exit_code == 0 + assert "no agent specified" in result.output.lower() + trials = list((ws / ".pier" / "trials").iterdir()) + assert len(trials) == 1 + + +@patch("pier.harbor_bridge.extract_agent_logs", return_value={"cost_usd": 0.05}) +@patch("pier.cli._copy_session_from_container") +@patch("pier.harbor_bridge.verify_environment", return_value={"reward": 1.0}) +@patch("pier.harbor_bridge.is_environment_running", return_value=True) +def test_verify_container_session_dir_copies_from_container( + mock_running, + mock_verify, + mock_copy, + mock_extract, + runner, + index_path, + task_dir, + tmp_path, +): + """--session-dir in container mode copies from container via docker cp.""" + ws = tmp_path / "ws" + ws.mkdir() + sess = _container_session(task_dir=str(task_dir), agents=[]) + _write_session(ws, sess, index_path) + + # Mock _copy_session_from_container to create the destination dir + def fake_copy(hsid, container_path, dest): + dest.mkdir(parents=True, exist_ok=True) + + mock_copy.side_effect = fake_copy + + result = runner.invoke( + cli, + ["verify", "--agent", "claude-code", "--session-dir", "/root/.claude"], + catch_exceptions=False, + ) + assert result.exit_code == 0 + mock_copy.assert_called_once() + # Verify the container path was passed through + assert mock_copy.call_args[0][1] == "/root/.claude" + + def test_verify_host_custom_trial_dir(runner, index_path, task_dir, tmp_path): """--trial-dir overrides the auto-generated trial directory for host mode.""" ws = tmp_path / "ws" @@ -1906,11 +2018,10 @@ def test_summarize_all_flag(runner, index_path, tmp_path, monkeypatch): @patch("pier.harbor_bridge.extract_agent_logs", return_value={"cost_usd": 0.05}) -@patch("pier.harbor_bridge.detect_agent_from_session_dir", return_value="claude-code") def test_capture_explicit_session_dir( - mock_detect, mock_extract, runner, index_path, tmp_path, monkeypatch + mock_extract, runner, index_path, tmp_path, monkeypatch ): - """pier capture --session-dir skips auto-discovery.""" + """pier capture --session-dir with -a extracts trajectory.""" session_dir = tmp_path / "session" session_dir.mkdir() (session_dir / "main.jsonl").write_text("{}\n") @@ -1922,12 +2033,32 @@ def test_capture_explicit_session_dir( monkeypatch.setenv("PWD", str(ws)) result = runner.invoke( - cli, ["capture", "--session-dir", str(session_dir)], catch_exceptions=False + cli, + ["capture", "--session-dir", str(session_dir), "-a", "claude-code"], + catch_exceptions=False, ) assert result.exit_code == 0 assert "trajectory extracted" in result.output.lower() +def test_capture_session_dir_without_agent_errors( + runner, index_path, tmp_path, monkeypatch +): + """pier capture --session-dir without -a errors.""" + session_dir = tmp_path / "session" + session_dir.mkdir() + + ws = tmp_path / "ws" + ws.mkdir() + _write_session(ws, _host_session(), index_path) + monkeypatch.chdir(ws) + monkeypatch.setenv("PWD", str(ws)) + + result = runner.invoke(cli, ["capture", "--session-dir", str(session_dir)]) + assert result.exit_code != 0 + assert "--agent" in result.output + + def test_capture_host_no_session_dir_fails(runner, index_path, tmp_path, monkeypatch): """pier capture without --session-dir in host mode gives a clear error.""" ws = tmp_path / "ws" @@ -1942,9 +2073,8 @@ def test_capture_host_no_session_dir_fails(runner, index_path, tmp_path, monkeyp @patch("pier.harbor_bridge.extract_agent_logs", return_value={"cost_usd": 0.05}) -@patch("pier.harbor_bridge.detect_agent_from_session_dir", return_value="claude-code") def test_capture_creates_trial_dir( - mock_detect, mock_extract, runner, index_path, tmp_path, monkeypatch + mock_extract, runner, index_path, tmp_path, monkeypatch ): """pier capture creates a timestamped trial directory under .pier/trials/.""" session_dir = tmp_path / "session" @@ -1958,7 +2088,9 @@ def test_capture_creates_trial_dir( monkeypatch.setenv("PWD", str(ws)) runner.invoke( - cli, ["capture", "--session-dir", str(session_dir)], catch_exceptions=False + cli, + ["capture", "--session-dir", str(session_dir), "-a", "claude-code"], + catch_exceptions=False, ) trials_dir = ws / ".pier" / "trials" @@ -1969,9 +2101,8 @@ def test_capture_creates_trial_dir( @patch("pier.harbor_bridge.extract_agent_logs", return_value={"cost_usd": 0.05}) -@patch("pier.harbor_bridge.detect_agent_from_session_dir", return_value="claude-code") def test_capture_outside_workspace( - mock_detect, mock_extract, runner, index_path, tmp_path, monkeypatch + mock_extract, runner, index_path, tmp_path, monkeypatch ): """pier capture works outside a pier workspace (no session.json).""" session_dir = tmp_path / "session" @@ -1984,16 +2115,17 @@ def test_capture_outside_workspace( monkeypatch.setenv("PWD", str(project)) result = runner.invoke( - cli, ["capture", "--session-dir", str(session_dir)], catch_exceptions=False + cli, + ["capture", "--session-dir", str(session_dir), "-a", "claude-code"], + catch_exceptions=False, ) assert result.exit_code == 0 assert (project / ".pier" / "trials").is_dir() @patch("pier.harbor_bridge.extract_agent_logs", return_value={"cost_usd": 0.05}) -@patch("pier.harbor_bridge.detect_agent_from_session_dir", return_value="claude-code") def test_capture_outside_workspace_does_not_attach_to_unrelated_active_workspace( - mock_detect, mock_extract, runner, index_path, tmp_path, monkeypatch + mock_extract, runner, index_path, tmp_path, monkeypatch ): """capture from a non-workspace directory should write into cwd, not another active workspace.""" session_dir = tmp_path / "session" @@ -2010,7 +2142,9 @@ def test_capture_outside_workspace_does_not_attach_to_unrelated_active_workspace monkeypatch.setenv("PWD", str(project)) result = runner.invoke( - cli, ["capture", "--session-dir", str(session_dir)], catch_exceptions=False + cli, + ["capture", "--session-dir", str(session_dir), "-a", "claude-code"], + catch_exceptions=False, ) assert result.exit_code == 0 @@ -2018,22 +2152,15 @@ def test_capture_outside_workspace_does_not_attach_to_unrelated_active_workspace assert not (active_ws / ".pier" / "trials").exists() -@patch("pier.harbor_bridge.extract_agent_logs", return_value={"cost_usd": 0.05}) -@patch( - "pier.harbor_bridge.find_container_agent_session_dir", - return_value=Path("/tmp/fake-session"), -) -@patch("pier.harbor_bridge.detect_agent_from_session_dir", return_value="claude-code") +@patch("pier.harbor_bridge.extract_agent_context", return_value={"cost_usd": 0.05}) def test_capture_container_auto_discover( - mock_detect, - mock_find, mock_extract, runner, index_path, tmp_path, monkeypatch, ): - """pier capture in container mode auto-discovers session from agent logs.""" + """pier capture in container mode extracts via Harbor directly.""" ws = tmp_path / "ws" ws.mkdir() sess = _container_session(agents=["claude-code"]) @@ -2044,17 +2171,14 @@ def test_capture_container_auto_discover( result = runner.invoke(cli, ["capture"], catch_exceptions=False) assert result.exit_code == 0 assert "trajectory extracted" in result.output.lower() - mock_find.assert_called_once() + mock_extract.assert_called_once() -@patch( - "pier.harbor_bridge.find_container_agent_session_dir", - return_value=None, -) +@patch("pier.harbor_bridge.extract_agent_context", return_value=None) def test_capture_container_no_session_found( - mock_find, runner, index_path, tmp_path, monkeypatch + mock_extract, runner, index_path, tmp_path, monkeypatch ): - """pier capture in container mode fails clearly when no session found.""" + """pier capture in container mode fails clearly when extraction returns nothing.""" ws = tmp_path / "ws" ws.mkdir() sess = _container_session(agents=["claude-code"]) @@ -2064,7 +2188,7 @@ def test_capture_container_no_session_found( result = runner.invoke(cli, ["capture"]) assert result.exit_code != 0 - assert "could not find" in result.output.lower() + assert "could not extract" in result.output.lower() def test_capture_container_multiple_agents_requires_flag( From ca4a03258f6ea19a57882e17ec2a7da02092039c Mon Sep 17 00:00:00 2001 From: Jonathan Bragg Date: Thu, 9 Apr 2026 21:06:27 -0700 Subject: [PATCH 2/2] Surface stderr in _tar_copy_from_container on failure --- pier/cli.py | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/pier/cli.py b/pier/cli.py index 42681aa..534dae4 100644 --- a/pier/cli.py +++ b/pier/cli.py @@ -221,17 +221,30 @@ def _tar_copy_from_container(container: str, workdir: str, workspace: Path) -> N ".", ], stdout=subprocess.PIPE, + stderr=subprocess.PIPE, ) as tar_create: extract = subprocess.run( ["tar", "-xf", "-", "--no-same-owner", "-C", str(workspace)], stdin=tar_create.stdout, env={**os.environ, "COPYFILE_DISABLE": "1"}, + capture_output=True, ) + if tar_create.stdout is not None: + tar_create.stdout.close() tar_create.wait() - if tar_create.returncode != 0 or extract.returncode != 0: + tar_create_stderr = b"" + if tar_create.stderr is not None: + tar_create_stderr = tar_create.stderr.read() + if extract.returncode != 0: + stderr = extract.stderr.decode(errors="replace").strip() + raise click.ClickException( + f"Failed to extract container files to workspace: " + f"{stderr or 'tar extraction failed'}" + ) + if tar_create.returncode != 0: + stderr = tar_create_stderr.decode(errors="replace").strip() raise click.ClickException( - f"tar copy from container failed (create={tar_create.returncode}, " - f"extract={extract.returncode})" + f"Failed to archive container files: {stderr or 'tar create failed'}" )