tests/bench/run_tests.py (2)

173-183: Improve error handling with more specific timeout information.

The error message lacks details about which session timed out during polling.

Enhance the error logging to include session context:

 try:
     await honcho_client.poll_deriver_status(
         session_id=session_id,
         timeout=float(self.timeout_seconds)
         if self.timeout_seconds
         else 10000.0,
     )
     return True
 except Exception as e:
-    self.logger.warning(f"Error polling deriver status: {e}")
+    self.logger.warning(
+        f"Error polling deriver status for session '{session_id}': {e}"
+    )
     return False

---

730-731: Pass timeout parameter to TestRunner constructor.

The --timeout argument is parsed but never passed to the TestRunner constructor.

 # Create test runner
 runner = TestRunner(
-    honcho_url=args.honcho_url, anthropic_api_key=args.anthropic_api_key
+    honcho_url=args.honcho_url,
+    anthropic_api_key=args.anthropic_api_key,
+    timeout_seconds=args.timeout
 )

src/config.py (1)

197-203: Peer-card model and token bounds: align with earlier review and provider limits

Current bounds (gt=1000, le=10_000, default=4000) are at odds with earlier review guidance and typical provider limits for GPT‑5 Nano; also peer cards are intentionally short.

Update defaults and bounds to be flexible for short outputs and match model capacity:

-    PEER_CARD_MODEL: str = "gpt-5-nano-2025-08-07"
-    # Note: peer cards should be very short, but GPT-5 models need output tokens for thinking which cannot be turned off...
-    PEER_CARD_MAX_OUTPUT_TOKENS: Annotated[
-        int, Field(default=4000, gt=1000, le=10_000)
-    ] = 4000
+    PEER_CARD_MODEL: str = "gpt-5-nano-2025-08-07"  # verify availability in envs
+    # Peer cards are concise. Allow small budgets; upper bound reflects model capacity.
+    PEER_CARD_MAX_OUTPUT_TOKENS: Annotated[
+        int, Field(default=2000, ge=128, le=128_000)
+    ] = 2000  # includes any provider reasoning tokens

Optionally add a validator to ensure the correct API key is set for the provider (based on prior feedback). I can draft it if you want it in this PR.

src/dialectic/chat.py (1)

306-318: Add explicit error handling for peer card retrieval.

The current implementation doesn't handle potential errors from crud.get_peer_card. While tracked_db may handle some errors, consider what happens if the database operation fails unexpectedly.

Apply this diff to add proper error handling:

    # 4. Peer card(s) ----------------------------------------------------------
    async with tracked_db("chat.get_peer_card") as db:
-       peer_card = await crud.get_peer_card(db, workspace_name, peer_name)
-       if target_name:
-           target_peer_card = await crud.get_peer_card(db, workspace_name, target_name)
-       else:
-           target_peer_card = None
+       try:
+           peer_card = await crud.get_peer_card(db, workspace_name, peer_name)
+       except Exception as e:
+           logger.error("Failed to retrieve peer_card for %s in workspace %s: %s",
+                        peer_name, workspace_name, e)
+           peer_card = None  # Use None as fallback
+       
+       if target_name:
+           try:
+               target_peer_card = await crud.get_peer_card(db, workspace_name, target_name)
+           except Exception as e:
+               logger.warning("Could not fetch target_peer_card for %s: %s", target_name, e)
+               target_peer_card = None
+       else:
+           target_peer_card = None

src/crud/representation.py (1)

48-73: Add 25-line validation and improve error handling for peer cards.

According to DEV-1032 requirements, peer cards should be limited to 25 lines. Also, consider removing the "peer_card" key when setting to None instead of storing null values.

Apply this diff to address both issues:

 async def set_peer_card(
     db: AsyncSession, workspace_name: str, peer_name: str, peer_card: list[str] | None
 ) -> None:
     """
     Set peer card for a peer.
 
     Raises:
         ResourceNotFoundException: If the peer does not exist
+        ValidationException: If the peer card exceeds 25 lines
     """
+    # Validate 25-line limit
+    if peer_card is not None and len(peer_card) > 25:
+        raise exceptions.ValidationException(
+            f"Peer card exceeds 25 line limit: {len(peer_card)} lines"
+        )
+
+    # Build update statement based on whether we're setting or clearing
+    if peer_card is None:
+        # Remove the key entirely when clearing
+        stmt = (
+            update(models.Peer)
+            .where(models.Peer.workspace_name == workspace_name)
+            .where(models.Peer.name == peer_name)
+            .values(
+                internal_metadata=models.Peer.internal_metadata.op("-")("peer_card")
+            )
+        )
+    else:
         stmt = (
             update(models.Peer)
             .where(models.Peer.workspace_name == workspace_name)
             .where(models.Peer.name == peer_name)
             .values(
                 internal_metadata=models.Peer.internal_metadata.op("||")(
                     {"peer_card": peer_card}
                 )
             )
         )
     result = await db.execute(stmt)
     if result.rowcount == 0:
-        raise exceptions.ResourceNotFoundException(
-            f"Peer {peer_name} not found in workspace {workspace_name}"
-        )
+        # Log context, then raise
+        logger.warning(
+            "Peer not found when setting peer card (workspace=%s, peer=%s)",
+            workspace_name,
+            peer_name,
+        )
+        raise exceptions.ResourceNotFoundException()
     await db.commit()

src/deriver/deriver.py (6)

41-44: Use absolute imports for consistency.

Replace relative imports with absolute ones to comply with project conventions.

-from .prompts import critical_analysis_prompt, peer_card_prompt
-from .queue_payload import (
-    RepresentationPayload,
-)
+from src.deriver.prompts import critical_analysis_prompt, peer_card_prompt
+from src.deriver.queue_payload import RepresentationPayload

---

91-98: Add return type annotation for public function.

The public function peer_card_call is missing a return type annotation, which is inconsistent with other public functions in the codebase.

 async def peer_card_call(
     old_peer_card: list[str] | None,
     new_observations: list[str],
-):
+) -> PeerCardQuery:

---

101-109: Expand docstring to follow Google style conventions.

The docstring should include Args and Returns sections for better documentation.

     """
     Process a representation task by extracting insights and updating working representations.
+    
+    Args:
+        db: Database session
+        payload: Representation task payload containing workspace, session, and message details
+    
+    Returns:
+        None
     """

---

555-558: Improve error handling for peer card updates.

Use logger.exception to preserve stack trace and consider whether this should be a fatal error.

         except Exception as e:
             if settings.SENTRY.ENABLED:
                 sentry_sdk.capture_exception(e)
-            logger.error("Error updating peer card! Skipping... %s", e)
+            logger.exception("Error updating peer card for %s! Skipping...", self.ctx.sender_name)

---

612-612: Use timezone-aware datetime for consistency.

Using datetime.now() without timezone can cause issues in distributed systems or when comparing timestamps across different timezones.

     working_rep_data = {
         "final_observations": final_obs_dict,
         "message_id": payload.message_id,
-        "created_at": datetime.datetime.now().isoformat(),
+        "created_at": datetime.datetime.now(datetime.timezone.utc).isoformat(),
     }

---

221-231: Type inconsistency: peer card is list[str] but used as if it's a string.

The sender_peer_card variable is a list[str] | None from crud.get_peer_card, but the downstream reasoner.reason method and prompts expect a string representation.

Convert the list to a string before passing downstream:

     if payload.sender_name == payload.target_name:
         sender_peer_card: list[str] | None = await crud.get_peer_card(
             db, payload.workspace_name, payload.sender_name
         )
         if sender_peer_card is None:
             logger.warning("No peer card found for %s", payload.sender_name)
+            sender_peer_card_str = None
         else:
-            logger.info("Using peer card: %s", sender_peer_card)
+            sender_peer_card_str = "\n".join(sender_peer_card)
+            logger.info("Using peer card:\n%s", sender_peer_card_str)
     else:
         logger.info("No peer card used for directional representation derivation")
-        sender_peer_card = None
+        sender_peer_card_str = None

     # Run single-pass reasoning
     final_observations = await reasoner.reason(
         db,
         working_representation,
         formatted_history,
-        sender_peer_card,
+        sender_peer_card_str,
     )

Then update the reason method signature and related methods to accept str | None instead of list[str] | None.

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T17:17:14.646Z
Learning: Applies to src/crud.py : Maintain database CRUD operations in src/crud.py

Learnt from: dr-frmr
PR: plastic-labs/honcho#131
File: src/routers/sessions.py:206-213
Timestamp: 2025-06-18T20:42:06.458Z
Learning: The `get_or_create_session` function in this codebase is designed to handle both session creation and adding peers to existing sessions. When called with peers, it will add those peers to an existing session rather than creating a duplicate session.

Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T17:17:14.646Z
Learning: Applies to {src,tests}/**/*.py : Follow isort conventions with absolute imports preferred in Python code

src/dialectic/prompts.py (3)

39-41: Typos fixed (“user’s”): LGTM

Thanks for correcting “users's” to “user's” in both branches.

Also applies to: 48-49

---

77-78: Wrap {query_target} in tags for consistency with other blocks

Matches the style of <recent_conversation_history>, , etc., and eases parsing/debugging.

Apply this diff:

-{query_target}
+<query_target>
+{query_target}
+</query_target>

---

13-16: Default peer_card to None in dialectic_prompt signature

Mirror the existing optional target_peer_card default so adding this parameter remains non-breaking. All current call sites in src/dialectic/chat.py already pass peer_card positionally, so this change won’t affect them.

• File: src/dialectic/prompts.py, adjust lines 13–16

     peer_name: str,
-    peer_card: list[str] | None,
+    peer_card: list[str] | None = None,
     target_name: str | None = None,
     target_peer_card: list[str] | None = None,

src/deriver/prompts.py (1)

111-120: Render old card only when present and use '\n' join

Avoid showing an empty “Current user biographical card” section; prefer "\n" over chr(10).

Apply this diff:

-    old_peer_card_section = (
-        f"""
+    old_peer_card_section = (
+        f"""
 Current user biographical card:
-{chr(10).join(old_peer_card)}
+{\n.join(old_peer_card)}
     """
-        if old_peer_card is not None
+        if old_peer_card
         else """
 User does not have a card. Create one with any key observations.
     """
     )

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

src/deriver/queue_manager.py (1)

16-21: Prefer absolute imports; unify models import for consistency

You switched to an absolute import for exceptions but still use a relative import for models. Per guidelines, prefer absolute imports throughout.

Apply this diff:

-from .. import models
+from src import models

src/deriver/prompts.py (5)

35-45: Skip empty peer-card blocks; use "\n" and filter blank lines

Avoid emitting empty <peer_card> tags and prefer "\n" over chr(10). Also filter out empty lines for cleaner formatting.

-    # Format the peer card as a string with newlines
-    peer_card_section = (
-        f"""
-        The user's known biographical information:
-        <peer_card>
-        {chr(10).join(peer_card)}
-        </peer_card>
-        """
-        if peer_card is not None
-        else ""
-    )
+    # Format the peer card as a string with newlines, skipping empties
+    peer_card_lines = [line for line in (peer_card or []) if line and line.strip()]
+    peer_card_section = (
+        f"""
+The user's known biographical information:
+<peer_card>
+{'\n'.join(peer_card_lines)}
+</peer_card>
+"""
+        if peer_card_lines
+        else ""
+    )

---

47-56: Skip empty current-context blocks to avoid blank tags

Test truthiness so empty strings don’t produce empty-tag sections.

-    working_representation_section = (
+    working_representation_section = (
         f"""
 The current user understanding:
 <current_context>
 {working_representation}
 </current_context>
 """
-        if working_representation is not None
+        if working_representation
         else ""
     )

---

111-120: Render old_peer_card as readable text and fall back if empty

Join lines with "\n" and treat empty lists as “no card”.

-    old_peer_card_section = (
-        f"""
-Current user biographical card:
-{chr(10).join(old_peer_card)}
-    """
-        if old_peer_card is not None
-        else """
-User does not have a card. Create one with any key observations.
-    """
-    )
+    old_peer_card_lines = [line for line in (old_peer_card or []) if line and line.strip()]
+    old_peer_card_section = (
+        f"""
+Current user biographical card:
+{'\n'.join(old_peer_card_lines)}
+    """
+        if old_peer_card_lines
+        else """
+User does not have a card. Create one with any key observations.
+    """
+    )

---

154-156: Format new observations as readable lines and wrap in tags

Avoid Python list repr; join lines and add a clear tag.

-New observations:
-{chr(10).join(new_observations)}
+New observations:
+<new_observations>
+{'\n'.join(obs for obs in new_observations if obs and obs.strip())}
+</new_observations>

---

157-158: Remove reference to undefined “notes” field

The prompt mentions a “notes field,” but no output schema includes it. This can confuse the model.

-If there's no new key info, you should return an empty card. **NEVER** include notes or temporary information in the card itself, instead use the notes field.
+If there's no new key info, return an empty card. **NEVER** include notes or temporary information in the card.

src/deriver/queue_manager.py (2)

276-281: Good: parameterized logging on shutdown path

The switch to parameterized logging resolves prior lint (G004) and avoids f-string expansion.

---

250-256: Fix head-of-line blocking and avoid PII leakage in LLMError path

• continue will re-select the same unprocessed message and can loop indefinitely, stalling the work unit.
• f-string logs the full QueueItem repr, which likely includes payload (PII risk).
• Use logger.exception (stack trace) with parameterized logging, capture to Sentry, and fall through so processed=True is applied.

Apply this diff:

-                    except exceptions.LLMError as e:
-                        logger.error(
-                            f"LLM returned bad JSON for message {message}, re-queueing",
-                        )
-                        if settings.SENTRY.ENABLED:
-                            sentry_sdk.capture_exception(e)
-                        continue
+                    except exceptions.LLMError as e:
+                        logger.exception(
+                            "LLM returned malformed JSON for queue item id=%s; "
+                            "marking processed to prevent stalling",
+                            message.id,
+                        )
+                        if settings.SENTRY.ENABLED:
+                            sentry_sdk.capture_exception(e)
+                        # fall through to mark processed and commit

src/dialectic/chat.py (1)

306-317: Handle DB errors and avoid PII leakage when logging peer cards; consider truncation

• get_peer_card may return None and can still raise (e.g., DB issues). Wrap calls in try/except and default to a safe empty list to keep types consistent.
• Logging full peer cards at INFO risks leaking PII. Log counts at INFO and full content only at DEBUG.
• Optional: bound the size of cards (by tokens/lines) before passing to the prompt to prevent context overflow.

Apply this diff:

-    async with tracked_db("chat.get_peer_card") as db:
-        peer_card = await crud.get_peer_card(db, workspace_name, peer_name)
-        if target_name:
-            target_peer_card = await crud.get_peer_card(db, workspace_name, target_name)
-        else:
-            target_peer_card = None
-
-    if target_peer_card:
-        logger.info("Retrieved peer cards:\n%s\n%s", peer_card, target_peer_card)
-    else:
-        logger.info("Retrieved peer card:\n%s", peer_card)
+    async with tracked_db("chat.get_peer_card") as db:
+        try:
+            peer_card = await crud.get_peer_card(db, workspace_name, peer_name)
+        except Exception as e:
+            logger.error(
+                "Failed to retrieve peer_card for peer=%s in workspace=%s: %s",
+                peer_name,
+                workspace_name,
+                e,
+            )
+            peer_card = []
+        if target_name:
+            try:
+                target_peer_card = await crud.get_peer_card(
+                    db, workspace_name, target_name
+                )
+            except Exception as e:
+                logger.warning(
+                    "Failed to retrieve target_peer_card for target=%s in workspace=%s: %s",
+                    target_name,
+                    workspace_name,
+                    e,
+                )
+                target_peer_card = []
+        else:
+            target_peer_card = None
+
+    # Avoid PII leakage: log only counts at INFO; full content gated to DEBUG
+    if logger.isEnabledFor(logging.DEBUG):
+        logger.debug(
+            "Retrieved peer cards content (DEBUG). peer=%s, target=%s\npeer_card=%s\ntarget_peer_card=%s",
+            peer_name,
+            target_name,
+            peer_card,
+            target_peer_card,
+        )
+    if target_peer_card:
+        logger.info(
+            "Retrieved peer cards: peer=%s lines=%d, target=%s lines=%d",
+            peer_name,
+            len(peer_card or []),
+            target_name,
+            len(target_peer_card or []),
+        )
+    else:
+        logger.info(
+            "Retrieved peer card: peer=%s lines=%d",
+            peer_name,
+            len(peer_card or []),
+        )

Optional truncation helper to keep cards within a safe token budget (place near the top of this file, alongside other utils), then call it here before logging:

def truncate_peer_card(
    card: list[str] | None,
    tokenizer: tiktoken.Encoding,
    max_tokens: int = 1000,
) -> list[str]:
    if not card:
        return []
    tokens = tokenizer.encode("\n".join(card))
    tokens = tokens[:max_tokens]
    text = tokenizer.decode(tokens)
    return text.splitlines()

Then invoke:

peer_card = truncate_peer_card(peer_card, tokenizer)
if target_peer_card is not None:
    target_peer_card = truncate_peer_card(target_peer_card, tokenizer)

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T17:17:14.646Z
Learning: Applies to src/crud.py : Maintain database CRUD operations in src/crud.py