review-fix(comments): restore lost rationale in the 7 files the sweep had to skip (docstring/comment-only)

agent/coding_context.py

@@ -412,7 +412,10 @@ def coding_compact_skill_categories(*, platform: Optional[str] = None, cwd: Opti
 def _git(cwd: Path, *args: str) -> str:
     """``git -C <cwd> <args>`` → stripped stdout, or ``""`` on any failure. bounded_git_probe
     bounds post-kill cleanup on Windows — plain ``subprocess.run(timeout=...)`` deadlocked
-    when a killed git left a suspended descendant holding the pipe handles."""
+    when a killed git left a suspended descendant holding the pipe handles.
+
+    See #66037.
+    """
     return bounded_git_probe(["git", "-C", str(cwd), *args], timeout=_GIT_TIMEOUT)
 
 

agent/status_output.py

@@ -44,7 +44,13 @@ class StatusOutputMixin:
 
     def _should_emit_quiet_tool_messages(self) -> bool:
         """True when quiet-mode tool summaries should print directly (CLI, no callback owns rendering);
-        ``suppress_status_output`` always wins so ``[tool]``/``[done]`` never land in captured stdout."""
+        ``suppress_status_output`` always wins so ``[tool]``/``[done]`` never land in captured stdout.
+
+        ``suppress_status_output`` (the strict machine-readable mode used by ``hermes chat -Q``) always
+        wins: those flows neutralize the rendering callbacks, and without this gate the "no callback owns
+        rendering" fallback would print ``[tool]``/``[done]`` spinner lines into the captured stdout it
+        exists to keep clean (#93220).
+        """
         if getattr(self, "suppress_status_output", False):
             return False
         return self.quiet_mode and not self.tool_progress_callback and getattr(self, "platform", "") == "cli"
@@ -92,7 +98,12 @@ class StatusOutputMixin:
         ))
 
     def _warn_uncompressed_context_overflow(self, preflight_tokens: int, context_length: int) -> None:
-        """Deduped warning when uncompressed context exceeds the model limit; points the user at /compact."""
+        """Deduped warning when uncompressed context exceeds the model limit; points the user at /compact.
+
+        When compression is explicitly disabled (compression.enabled: false), long sessions can grow past
+        the model context window with no compression to shrink them (#89297). Surface an actionable warning
+        so the user knows to run /compact or enable compression.
+        """
         _warn_key = ("uncompressed_ctx_overflow", context_length)
         if getattr(self, "_last_ctx_overflow_warn", None) != _warn_key:
             self._last_ctx_overflow_warn = _warn_key

agent/system_prompt.py

@@ -185,7 +185,17 @@ def _session_start_like(agent: Any, now: Any) -> Any:
     lineage-root session id's embedded stamp (compaction rotates ids, each with
     its own mint time), the current session id's stamp, ``agent.session_start``,
     then ``now``.  Stamps are box-local wall-clock: attach that zone first, then
-    convert to ``now``'s zone so the date matches the per-turn clock."""
+    convert to ``now``'s zone so the date matches the per-turn clock.
+
+    0. the LINEAGE-ROOT session id's embedded timestamp — compaction can rotate the session id, and each
+    rotated id embeds its OWN mint time, so after months of compactions rung 1 alone would quietly re-birth
+    the conversation at its latest rotation. Walking to the lineage root (same walk as
+    ``_conversation_root_id``) recovers the ORIGINAL birth stamp — a Bot Mode forever-chat keeps knowing
+    when it was first born, across every compaction (maintainer-directed, #98426); 1. the timestamp embedded
+    in ``session_id`` (``YYYYMMDD_HHMMSS_...``) — immutable for the life of the session, so the line is
+    byte-stable across every rebuild boundary (preserving prefix-cache KV); 2. 3. ``now`` (initial/legacy
+    build without either).
+    """
     from datetime import datetime
     def _to_display_tz(dt: Any) -> Any:
         if dt.tzinfo is None:
@@ -221,7 +231,17 @@ def _agent_home(agent: Any) -> Optional[Path]:
     A bound HERMES_HOME ContextVar override wins (the gateway multiplexes
     profiles over one shared session DB and binds the home per turn); else the
     parent of ``_session_db.db_path`` — ground truth on threads that lost the
-    ContextVar, where ambient resolution would leak the launch profile."""
+    ContextVar, where ambient resolution would leak the launch profile.
+
+    1. Surfaces that multiplex several profiles over ONE shared session DB (the messaging gateway:
+    ``gateway/run.py`` hands every agent the launch-home ``state.db`` and binds the profile home per turn
+    via ``_profile_runtime_scope`` + ``copy_context``) would otherwise have the db-derived launch home STOMP
+    the correctly-bound profile — inverting the leak this helper exists to fix (found by @kshitijk4poor's
+    post-merge probe on #86313). 2. Fallback: the home containing the agent's ``_session_db.db_path``
+    (``<home>/state.db``) — ground truth on threads that lost the ContextVar (ContextVars don't propagate
+    into ``threading.Thread``), where the unbound build previously fell back to the launch home and leaked
+    the default profile's skills/identity into a bot prompt.
+    """
     try:
         from hermes_constants import get_hermes_home_override
         override = get_hermes_home_override()
@@ -348,6 +368,11 @@ def _active_profile_line(agent: Any) -> str:
     # A non-default name is only returned when the resolved home is ALREADY
     # <root>/profiles/<name>, so the profile home is the session home itself.
     profile_home = str(_agent_home_path) if _agent_home_path is not None else str(get_hermes_home())
+    # A non-default name is only ever returned when the resolved home is ALREADY <root>/profiles/<name> —
+    # that is exactly how both _profile_name_for_home() and _resolve_active_profile_name() derive it. So the
+    # profile home is the session home itself; appending /profiles/<name> again doubled it (#72894). The
+    # default profile's data sits at the ROOT (get_default_hermes_root()), which in ambient profile mode is
+    # NOT get_hermes_home().
     default_root = get_default_hermes_root()
     return (
         f"Active Hermes profile: {active_profile}. This session reads "
@@ -419,6 +444,13 @@ def _timestamp_line(agent: Any) -> str:
     _zone_suffix = f" ({', '.join(_bits)})" if _bits else ""
     _start = _session_start_like(agent, now)
     timestamp_line = f"Conversation started: {_start.strftime('%A, %B %d, %Y')}{_zone_suffix}"
+    # Second line (maintainer design, salvaging #96224's anchor): long-lived sessions — Bot Mode
+    # forever-chats, messenger channels people never close — span many days and many compactions. A lone
+    # birth date leads the model to believe it is still living in that old day. The prompt is rebuilt at
+    # every compaction boundary, so stamp the rebuild day too: 'started' stays anchored and byte-stable, 'as
+    # of' refreshes exactly when the cache prefix is already being invalidated (compaction), so the added
+    # line costs no extra cache churn. Same-day sessions skip the second line entirely — nothing to correct,
+    # and the single-line shape stays byte-identical for the day (prefix-cache safe).
     if now.strftime("%Y%m%d") != _start.strftime("%Y%m%d"):
         timestamp_line += (f"\nToday's date (as of the last context rebuild): {now.strftime('%A, %B %d, %Y')} "
                            "— trust this over the start date for what day it is now; query tools for exact time.")
@@ -439,6 +471,9 @@ def _memory_parts(agent: Any) -> List[str]:
             block = agent._memory_store.format_for_system_prompt(kind) if enabled else None
             if block:
                 parts.append(block)
+    # External memory provider system prompt block (additive to built-in). Gated on the same check
+    # ``inject_memory_provider_tools`` uses so we never advertise provider tools that the agent's toolset
+    # configuration has already gated off (#81014).
     if agent._memory_manager:
         try:
             from agent.memory_manager import memory_provider_tools_exposed as _mem_exposed
@@ -621,7 +656,15 @@ def build_system_prompt(agent: Any, system_message: Optional[str] = None) -> str
 def invalidate_system_prompt(agent: Any) -> None:
     """Force a rebuild on the next turn (after compression): reload memory from
     disk and clear the frozen plugin snapshot (previous bytes stashed as the
-    fail-open fallback) so plugins re-render at the same boundary."""
+    fail-open fallback) so plugins re-render at the same boundary.
+
+    Called after context compression events. Also reloads memory from disk so the rebuilt prompt captures
+    any writes from this session, and clears the frozen plugin-section snapshot so plugins re-render at the
+    same boundary (maintainer-directed, #95681 arc): a plugin section is just another prompt block carrying
+    state — freezing it while memory, skills, and guidance refresh would recreate the stale-block disease
+    inside plugin-land. The previous bytes are stashed so a plugin whose render RAISES falls back to its
+    last good section instead of vanishing (fail-open guard, not a freeze).
+    """
     agent._cached_system_prompt = None
     agent._cached_system_prompt_static = None
     if hasattr(agent, "_plugin_system_prompt_sections_snapshot"):

agent/title_generator.py

@@ -25,6 +25,7 @@ FailureCallback = Callable[[str, BaseException], None]
 TitleCallback = Callable[[str, str], None]
 # () -> bool, called right before the LLM request; False skips (e.g. the user switched models and
 # the request would reload one the runtime already evicted).
+# Validation callback: () -> bool. See #19027.
 RuntimeValidator = Callable[[], bool]
 
 # Text budget handed to the model (Claude Code / OpenClaw converged on 1000).
@@ -32,6 +33,10 @@ MAX_TITLE_INPUT_CHARS = 1000
 # Cap on the instant derived title; a raw fragment reads worse the longer it runs.
 MAX_DERIVED_TITLE_CHARS = 48
 # Answer-shaped guard: a tiny model sometimes answers instead of titling; longer is rejected, not truncated.
+# Upper bound on accepted title word count. Titling is a 3-7 word task; a small tiny-model sometimes ignores
+# the task and answers the user's message instead — that answer must never become the session title (see the
+# answer-shaped output guard in generate_title; port of can1357/oh-my-pi#7306). 12 leaves headroom for
+# legitimate wordy titles while excluding full-sentence answers.
 _MAX_TITLE_WORDS = 12
 
 _TITLE_PROMPT_TEMPLATE = (
@@ -78,6 +83,11 @@ _MACHINE_PREFIXES = (
     "[CONTEXT COMPACTION", LEGACY_SUMMARY_PREFIX, "[Runtime note:", "[System note:", "[SYSTEM]",
     # tui_gateway.server._MODEL_SWITCH_MARKER_PREFIX (keep in sync); persisted as role="user" because
     # strict providers reject a non-first system message.
+    # Model-switch marker from tui_gateway.server._append_model_switch_marker. It is persisted with
+    # role="user" (strict OpenAI-compatible providers reject a system message that is not first — #48338),
+    # so without this entry it looks like a real opening turn: switching models before the first real
+    # message titled the session "[System: The active model for this chat has…" instead of the user's actual
+    # question.
     "[System: The active model for this chat has changed to ",
 )
 
@@ -227,7 +237,12 @@ def generate_title(
     runtime_validator: Optional[RuntimeValidator] = None,
 ) -> Optional[str]:
     """Title from the opening message alone (waiting for the assistant made this slow and bought
-    nothing). ``runtime_validator`` runs right before the request; False skips silently."""
+    nothing). ``runtime_validator`` runs right before the request; False skips silently.
+
+    If it returns False (e.g. the user's model was switched since the background thread captured its runtime
+    snapshot), the call is skipped silently — no request is sent, so a stale title request can't reload a
+    model the runtime already unloaded (#19027).
+    """
     if not _auto_title_enabled():
         logger.debug("Auto-title skipped: auxiliary.title_generation.enabled=false")
         return None
@@ -254,6 +269,11 @@ def generate_title(
             extra_body={"response_format": _TITLE_RESPONSE_FORMAT},
         )
         title = _clean_title(_extract_title_text(response.choices[0].message.content or ""))
+        # Answer-shaped output guard: titling is a 3-7 word task, so a title with many words is a model that
+        # ignored the task and answered the user's message instead ("I don't have context on X — that's not
+        # something I recognize..."). Truncating would store half an assistant blob as the session title,
+        # which is still an assistant blob — reject instead so the caller retries on the next exchange
+        # (maybe_auto_title fires for the first two exchanges). Port of can1357/oh-my-pi#7306.
         if title is not None and len(title.split()) > _MAX_TITLE_WORDS:
             # Answer-shaped output: reject (not truncate) so the caller retries next exchange.
             logger.debug("Rejecting answer-shaped title output (%d words > %d)", len(title.split()), _MAX_TITLE_WORDS)
@@ -283,7 +303,11 @@ def _persist_session_title(session_db, session_id, title, *, source, dedupe=True
     transaction, so a manual ``/title`` is never overwritten); None when a higher authority held the row.
     ``ValueError`` = unique-title index collision → append ``#N`` via ``get_next_title_in_lineage``;
     ``dedupe=False`` re-raises instead (the derived title is on the critical path, collides constantly
-    on "hi", and the model replaces it a second later anyway)."""
+    on "hi", and the model replaces it a second later anyway).
+
+    ``ValueError`` means the name is taken by an unrelated session (the unique-title index); rather than
+    leave the session untitled (#50537), append a ``#N`` suffix via ``get_next_title_in_lineage``.
+    """
     auto_fn = getattr(session_db, "set_auto_title", None)
 
     def _set(candidate):
@@ -348,6 +372,8 @@ def auto_title_session(
         with suppress(Exception):
             conversation_id = session_db.get_conversation_root(session_id) or session_id
         set_conversation_context(conversation_id)
+        # Same for the accounting context, so the title call's token usage is recorded against this session
+        # (task='title_generation', #23270).
         set_accounting_context(session_db, session_id)
         title, source = generate_title(
             user_message, failure_callback=failure_callback, main_runtime=main_runtime, runtime_validator=runtime_validator,

agent/tool_executor.py

@@ -93,7 +93,13 @@ def _ensure_file_checkpoint(agent, function_name: str, function_args: dict, effe
 def _budget_for_agent(agent) -> BudgetConfig:
     """Tool-result BudgetConfig scaled to the agent's context window. Unknown length goes
     through ``budget_for_context_window(None)`` (not DEFAULT_BUDGET) so the MCP threshold
-    override still applies."""
+    override still applies.
+
+    Large-context models keep the historical 100K/200K char defaults; small models (e.g. a 65K-token local
+    model switched into mid-session) get a budget proportional to their window so a single large tool result
+    can't push the request past the model's limit (#23767). Falls back to the default budget when the
+    context length isn't resolvable.
+    """
     try:
         ctx = getattr(getattr(agent, "context_compressor", None), "context_length", None)
         return budget_for_context_window(int(ctx) if ctx else None)
@@ -114,10 +120,22 @@ def _authorization_gate_lock_timeout() -> float:
     """Authorization-lock bound = ``tools.approval.human_wait_ceiling`` (approval timeout +
     margin, capped so it can't overflow Lock.acquire): never break serialization while a
     prompt is answerable, never let a wedged holder park workers forever. Deliberately NOT
-    min()'d with the fallback so the gate never gives up early."""
+    min()'d with the fallback so the gate never gives up early.
+
+    Delegates to ``tools.approval.human_wait_ceiling`` — the same bound that clamps a human-wait window's
+    deadline contribution — so the two can't drift. Long enough that serialization is never broken while a
+    legitimate approval prompt is still answerable; short enough that a wedged holder (hanging
+    ``pre_tool_call`` plugin, dead approval client) cannot park other workers forever (#79719). Resolved
+    once per gate (per batch), so a mid-process ``approvals.timeout`` change applies from the next batch.
+    """
     try:
         from tools.approval import human_wait_ceiling
 
+        # human_wait_ceiling is platform-safety-capped (agent/deadline.py MAX_SAFE_TIMEOUT_S): a huge
+        # approvals.timeout can no longer overflow Lock.acquire's time_t on macOS (#83220). Deliberately NOT
+        # min()'d with _AUTHORIZATION_GATE_LOCK_TIMEOUT_S — the gate must never give up while a legitimate
+        # approval prompt is still answerable (#79719), so a configured approvals.timeout above 360s must
+        # extend the gate.
         return human_wait_ceiling()
     except Exception:
         return _AUTHORIZATION_GATE_LOCK_TIMEOUT_S
@@ -208,7 +226,12 @@ def _ra():
 
 
 def _is_interpreter_shutdown_submit_error(exc: RuntimeError) -> bool:
-    """Shutdown-race predicate; ``tools.interpreter_shutdown`` knows both CPython message variants."""
+    """Shutdown-race predicate; ``tools.interpreter_shutdown`` knows both CPython message variants.
+
+    Delegates so all sites (cron delivery, conversation-loop retry, tool submission) recognize both CPython
+    shutdown-message variants instead of each matching its own substring (the bug class behind
+    #55924/#58720).
+    """
     from tools.interpreter_shutdown import interpreter_shutting_down
 
     return interpreter_shutting_down(exc)
@@ -431,6 +454,17 @@ class _ConcurrentToolAuthorizationGate:
     the batch behind a wedged plugin/approval client. Exclusion is measured at the SOURCE
     of the human wait (``tools.approval.human_wait_seconds``), NOT as gate residency —
     residency-based exclusion let a wedged plugin keep the deadline from ever firing.
+
+    Serialization keeps concurrent approval prompts from interleaving on the user's screen. The acquire is
+    BOUNDED: a worker wedged inside the gate (a hanging ``pre_tool_call`` plugin, or an approval round-trip
+    to a client that went away) must not park every other worker forever. On expiry the worker runs its
+    prompt unserialized — worst case is interleaved prompts, strictly better than permanent starvation (same
+    tradeoff as the start-order gate, #79705).
+    Gate residency is arbitrary code — using it as the exclusion signal let a wedged plugin grow the
+    exclusion 1:1 with wall clock, keeping the batch deadline's ``remaining`` constant so it never fired and
+    the turn hung forever (#79719). A wedged plugin now contributes nothing to the exclusion and the batch
+    times out normally, while a genuine approval wait (which can legitimately exceed any fixed bound) is
+    still excluded in full.
     """
 
     def __init__(self, *, lock_timeout: float | None = None, session_key: str | None = None) -> None:
@@ -462,6 +496,13 @@ class _ConcurrentToolAuthorizationGate:
 
     def run(self, callback):
         if not self._serialization_lock.acquire(timeout=self._lock_timeout):
+            # Deterministic failure (bad command, non-MCP URL, 401/403): every retry hits the same wall.
+            # Park immediately instead of burning the retry ladder and spamming N identical warnings
+            # (#65673). Auth failures park here too rather than returning. Returning ends the run task, and
+            # with it the only listener on ``_reconnect_event`` — so a 401 on the very first connect left
+            # the server unrevivable for the life of the process, even after the user re-authenticated with
+            # ``hermes mcp login``. Parking keeps the task alive so the 300s self-probe (and an explicit
+            # /mcp refresh) can pick up fresh tokens.
             logger.warning(
                 "authorization gate lock not acquired after %.1fs "
                 "(holder wedged in a pre_tool_call plugin or approval "
@@ -538,6 +579,10 @@ def _run_with_activity_heartbeat(agent, function_name: str, fn):
     """Run ``fn()`` under the activity heartbeat; covers both executor paths."""
     stop = threading.Event()
     thread = threading.Thread(
+        # Keep the gateway turn-inactivity watchdog from abandoning a turn whose tool call runs silently for
+        # longer than the inactivity timeout (#84491): stamp activity periodically while the tool is in
+        # flight, not just at start/completion. Both the sequential and the concurrent paths funnel through
+        # here, so a single heartbeat covers every tool.
         target=_run_tool_activity_heartbeat,
         args=(agent, stop, f"tool running: {function_name}"),
         kwargs={"interval": _TOOL_ACTIVITY_HEARTBEAT_INTERVAL_S},
@@ -1107,6 +1152,8 @@ class _ConcurrentBatch:
         abandoned at the gate (the main thread already wrote this slot; emitting would
         double-report the tool_call_id)."""
         agent = self.agent
+        # Approval/sudo callbacks (thread-local) and the agent turn's ContextVars are propagated by
+        # propagate_context_to_thread() at the submit site below (GHSA-qg5c-hvr5-hjgr, #13617).
         start = time.time()
         blocked = dispatched = False
         try:

agent/tool_guardrails.py

@@ -289,6 +289,11 @@ class ToolCallGuardrailController:
         self._halt_decision: ToolGuardrailDecision | None = None
         # Identical-call streak: CONSECUTIVE identical (tool, args, result) calls; any different call or
         # result resets it, so re-reads after edits and varied polling are never flagged.
+        # Identical-call loop-breaker state (agent.stall_guards): tracks the CONSECUTIVE streak of identical
+        # (tool, canonical args) calls whose results were also identical. Per-turn, like everything else
+        # here. NOTE: open PR #85352 (patrykkopycinski) tracks no-progress loops ACROSS turns via a
+        # detection window — a different mechanism from this per-turn consecutive streak. Coordinate future
+        # work there.
         self._identical_streak_sig: ToolCallSignature | None = None
         self._identical_streak_result_hash: str = ""
         self._identical_streak_count: int = 0
@@ -353,6 +358,12 @@ class ToolCallGuardrailController:
             # same_tool_failure counts DIFFERENT args on one tool; for failure-tolerant
             # tools a run of distinct red commands is diagnosis, not a loop — warn, never halt.
             if (
+                # Hard-stop widening (#89069 / #100849 bundle): the per-turn no-progress BLOCK above only
+                # covers tools in idempotent_tools, so a model replaying the same successful
+                # `terminal`/`skill_view` call with a byte-identical result ran until the iteration budget.
+                # The consecutive-identical streak is tool-agnostic; when hard stops are enabled, halt at
+                # the same idempotent_no_progress threshold. Pollers stay exempt (an unchanged poll is
+                # progress).
                 self.config.hard_stop_enabled
                 and tool_name not in FAILURE_TOLERANT_TOOL_NAMES
                 and same_count >= self.config.same_tool_failure_halt_after

tools/image_source.py

@@ -87,6 +87,11 @@ def _guard_credential_read(host_target: Path, src: str) -> None:
     """Shared credential-read guard: refuse secret-bearing files (.env, auth.json) with a specific
     error. Guard import is best-effort; a real block always propagates."""
     try:
+        # Shared credential-read guard (agent.file_safety, #57698): refuse secret-bearing files (.env,
+        # auth.json, ...) with an intentional, specific error instead of relying on the magic-byte sniff to
+        # reject them incidentally. Same chokepoint the image-gen/video-gen provider plugins enforce on
+        # model-supplied local paths. Import is best-effort (guard unavailability must not break image
+        # loading); a real block always propagates.
         from agent.file_safety import raise_if_read_blocked
     except Exception:  # noqa: BLE001 — guard unavailable: proceed
         return
@@ -190,7 +195,12 @@ def _get_active_env(task_id: Optional[str]):
 
 def _ensure_container_env(task_id: Optional[str]) -> None:
     """Lazily bring up the sandbox before an in-sandbox read (vision may be a session's first
-    action). Best-effort: failure leaves the env absent and the caller hits the fail-closed error."""
+    action). Best-effort: failure leaves the env absent and the caller hits the fail-closed error.
+
+    Unlike the terminal tool, vision never triggered environment creation, so a session whose first action
+    is ``vision_analyze`` on a container-only path under a non-local backend found no active env and failed
+    — until a terminal command happened to create one (issue #62825).
+    """
     if not task_id:
         return
     try:
@@ -208,8 +218,13 @@ async def _resolve_container_fallback(
     Cold-start retry: under Docker the first exec against a fresh container can fail (empty
     pipe) while a second succeeds. On final failure the container's output is folded into the
     error so "no such file" / "permission denied" / "never came up" are distinguishable.
+
+    We retry once with a short delay before giving up, so callers don't see "could not read inside the
+    sandbox" on a file that is verifiably readable on the immediate retry. See #76566.
     """
     import shlex
+    # Bring the sandbox up on demand: without this, the first vision_analyze of a session (before any
+    # terminal command) has no active env to read from under a non-local backend (issue #62825).
     _ensure_container_env(ctx.task_id)
     env = _get_active_env(ctx.task_id)
     if env is None: