VibecoderMcSwaggins commited on
Commit
c6474c2
Β·
1 Parent(s): 21e2351

docs: Add senior review findings to P2 round counter bug

Browse files

External review confirmed our analysis and added nuances:
- Manager agent also fires ExecutorCompletedEvent (explains 11 events)
- Time estimation is doubly flawed (wrong unit + wrong calibration)
- API discovery: ORCH_MSG_KIND_USER_TASK could track actual rounds

Review status: CONFIRMED - Ready for implementation

docs/bugs/P2_ROUND_COUNTER_SEMANTIC_MISMATCH.md CHANGED
@@ -223,6 +223,44 @@ The time estimate becomes useless after the first few agent completions.
223
 
224
  ---
225
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
226
  ## References
227
 
228
  - SPEC-18: Agent Framework Core Upgrade (where ExecutorCompletedEvent was introduced)
 
223
 
224
  ---
225
 
226
+ ## Senior Review Findings (2025-12-05)
227
+
228
+ **Reviewer**: External Gemini CLI Agent
229
+ **Status**: CONFIRMED - Analysis accurate and sufficient
230
+
231
+ ### Additional Nuances Identified
232
+
233
+ 1. **Manager Agent Also Fires Events**: The Manager itself is an agent. If `ExecutorCompletedEvent` fires for Manager's turn completion PLUS sub-agents' completions, the count accelerates 2-3x faster per logical round. This explains why we saw 11 events for ~2-3 workflow rounds.
234
+
235
+ 2. **Time Estimation Doubly Flawed**:
236
+ - Not just bottoming out at 0
237
+ - `_EST_SECONDS_PER_ROUND` (45s) is calibrated for a FULL workflow round, not a single agent step
238
+ - If we counted agent steps correctly: 10 steps Γ— 45s = 450s (way overestimated)
239
+ - A full round of 4 agents might only take 60s total
240
+
241
+ 3. **API Discovery - Can Track Actual Rounds**:
242
+ ```python
243
+ # These constants exist in agent_framework:
244
+ ORCH_MSG_KIND_INSTRUCTION = 'instruction'
245
+ ORCH_MSG_KIND_USER_TASK = 'user_task'
246
+ ORCH_MSG_KIND_TASK_LEDGER = 'task_ledger'
247
+ ORCH_MSG_KIND_NOTICE = 'notice'
248
+ ```
249
+
250
+ Counting `user_task` events from `MagenticOrchestratorMessageEvent` would align iteration with `max_rounds` 1:1, since this signals "Manager is beginning a new evaluation cycle."
251
+
252
+ ### Reviewer Recommendations
253
+
254
+ 1. **Option A (Rename)**: APPROVED - Safest, most honest fix
255
+ 2. **Option B (Track Workflow Rounds)**: DEFER - Requires verifying framework behavior across versions, risks brittleness
256
+ 3. **Remove Denominator**: Display `Agent Step {iteration}` without `/5` to avoid confusion
257
+ 4. **Delete Dead Code**: Confirmed `_get_progress_message` is never called
258
+ 5. **Fix Constants**: Use `self._EST_SECONDS_PER_ROUND` consistently
259
+
260
+ ### Review Status: βœ… PASSED - Ready for Implementation
261
+
262
+ ---
263
+
264
  ## References
265
 
266
  - SPEC-18: Agent Framework Core Upgrade (where ExecutorCompletedEvent was introduced)