codex/prs/bolinfest/study/PR-2736-study.md

PR #2736 Review Takeaways (bolinfest)

DOs

• Pass futures directly: create the future without wrapping in an unnecessary async block.

  // Good
  let rollout_fut = RolloutRecorder::new(&config, session_id, user_instructions.clone());
  
  let mcp_fut = McpConnectionManager::new(config.mcp_servers.clone());
  let default_shell_fut = shell::default_user_shell();
  let history_meta_fut = crate::message_history::history_metadata(&config);
  
  let (rollout_recorder_res, mcp_res, default_shell, (history_log_id, history_entry_count)) =
      tokio::join!(rollout_fut, mcp_fut, default_shell_fut, history_meta_fut);
  

• Propagate errors concisely: convert-and-bubble with map_err and ? while logging.

  let rollout_recorder = rollout_recorder_res.map_err(|e| {
      error!("failed to initialize rollout recorder: {e:#}");
      anyhow::anyhow!("failed to initialize rollout recorder: {e:#}")
  })?;
  

• Keep whitespace readable: insert a blank line between method definitions.

  impl Session {
      fn something(&self) {
          // ...
      }
  
      async fn record_initial_history(&self, turn_context: &TurnContext, history: InitialHistory) {
          // ...
      }
  }
  

• Attach doc comments to the correct item: ensure comments describe the intended type/value.

  /// Represents a newly created Codex conversation, including the first event.
  pub struct NewConversation {
      pub conversation_id: Uuid,
      pub conversation: Arc<CodexConversation>,
      pub first_event: Event,
  }
  
  /// Initial conversation history when spawning a session.
  #[derive(Debug, Clone)]
  pub enum InitialHistory {
      New,
      Resumed(Vec<ResponseItem>),
  }
  

• Keep comments accurate to the logic after refactors.

  fn truncate_after_dropping_last_messages(items: Vec<ResponseItem>, n: usize) -> InitialHistory {
      if n == 0 {
          return InitialHistory::Resumed(items);
      }
  
      // Find the cut index by walking backward over user messages.
      let mut cut_index = 0usize;
      let mut to_drop = n;
      for (idx, item) in items.iter().enumerate().rev() {
          if item.is_user_message() {
              if to_drop == 1 {
                  cut_index = idx;
                  break;
              }
              to_drop -= 1;
          }
      }
  
      // If no user messages remain before the cut, start fresh.
      if cut_index == 0 {
          InitialHistory::New
      } else {
          InitialHistory::Resumed(items.into_iter().take(cut_index).collect())
      }
  }
  

• Prefer assert_eq! when comparing exact values in tests; use matches! only when necessary.

  // After refactor returning InitialHistory, keep assertions simple:
  let truncated = truncate_after_dropping_last_messages(items.clone(), 1);
  if let InitialHistory::Resumed(v) = truncated {
      assert_eq!(v, vec![items[0].clone(), items[1].clone(), items[2].clone()]);
  } else {
      panic!("expected InitialHistory::Resumed");
  }
  
  let truncated2 = truncate_after_dropping_last_messages(items, 2);
  assert!(matches!(truncated2, InitialHistory::New));
  

DON’Ts

• Don’t wrap a future in an async block just to await it immediately.

  // Avoid
  let rollout_fut = async { RolloutRecorder::new(&config, session_id, user_instructions.clone()).await };
  

• Don’t hand-roll Ok/Err matches when a map_err + ? expresses it clearly.

  // Avoid
  let rollout_recorder = match rollout_recorder_res {
      Ok(rec) => rec,
      Err(e) => {
          error!("failed to initialize rollout recorder: {e:#}");
          return Err(anyhow::anyhow!("failed to initialize rollout recorder: {e:#}"));
      }
  };
  

• Don’t leave stale comments that no longer describe the code paths or return values.

  // Avoid: comment refers to previous Vec<ResponseItem> return; code now returns InitialHistory.
  // If fewer than n messages exist, drop everything and return an empty Vec.
  

• Don’t misplace doc comments when moving code; verify they’re attached to the intended item.

  // Avoid
  /// Represents a newly created Codex conversation...
  #[derive(Debug, Clone)]
  pub enum InitialHistory { /* ... */ }
  

• Don’t overcomplicate tests with pattern matches when a direct equality check is clearer.

  // Avoid
  assert!(matches!(foo(), Bar(baz) if baz == expected));
  // Prefer destructuring + assert_eq! as shown in DOs.