I've gone back and forth on this, but I think we should raise an exception here. It's indicative of something going very wrong. Not raising here caused me to miss a pretty severe bug in another PR

Oh I see the issue--you've got multiple AgentControllers registering themselves...

Let me think on this one for a minute.

It’s by design and unavoidable when you have (multi-layer) delegates - so I maintain a stack here.

What if the parent AgentController just calls on_event to its child? Then we only subscribe once from the top-level AgentController.

We should probably pass None as the event_stream to the Delegate in order to prevent issues

That's a viable approach I think, but it would be hard to implement/manage multi-layer agent delegate. If grandpa delegates to parent, which in turn delegates to a child, e.g. main agent -> database agent -> postgres migration agent, it would be hard/inelegant to let the root AgentController handle everything.

Can you explain more the issue here? Each AgentController would get to see every event, so it should be flexible enough to handle every possible case.

To be clear, the logic I'm envisioning is something like

def on_event(evt):
  if isinstance(evt, Foo):
    # ...all the normal logic...
  if self.delegate:
    self.delegate.on_event(evt)

Oh that’s a great idea! Will try

Done. I am not happy with the current implementation either (need to check whether itself is a delegate before it subscribe/unsubscribe) but looks like there's no perfect approach!

We cannot just pass None as event stream because AgentController needs to call event_stream.add_event.

I changed it back to the original implementation - let each AgentController worry about their own subscriptions and listeners. I added an append parameter to event_stream.subscribe method, and only delegates could set this parameter to true. This should largely prevent bugs like duplicate subscription...

FWIW, verifier Agent has severely hallucinated that there's a "good.txt" - a name coming out of nowhere.

In the long term, we do need to think of some intermediate representation of code changes - e.g. ".patch" or ".diff" so that the verifier agent could really see the "updates". Now it only sees the snapshot after the "fix" is already applied.

Alternatively, maybe we should work on the prompts and don't assign tasks to verifier agent if it's not in a git repository.

Please help me understand this a little, now that the delegate is done, does the parent need to send the child's historical messages to the LLM going forward?

Or is there another purpose for updating itself with its history?

Originally suggested by @rbren here: #1910 (comment)

I think the rationale is, the parent might be interested in learning what the child has done, to drive the task forward. It is debatable, though, that the parent may only need to know the result/summary of its child. But should parent trust child's conclusion? What if the child executes with a weaker LLM (not possible at the moment but maybe in the future to lower down the cost - parent agent using more powerful LLM while delegates use small models). Would a memory condenser be interesting? These open questions are hard to answer without benchmarking...

Yeah I've gone back and forth on this.

I think keeping the parent's memory out of the child makes a lot of sense. I could also see a case for keeping the child's memory out of the parent.

I forget what the problem was that led me to that original comment, but it definitely broke something I was working on...

I say we get this in either way, and we can take another look at history management soon

Yeah let's keep it this way at the moment.

I changed my mind when I was working on #2103. The parent agent would get confused when it sees the child agent's history because the parent doesn't know those commands used by child agent.

So now, the parent agent only learns about the result provided by the child agent.

It might be pure luck, but after removing child history, I ran regenerate and ManagerAgent was able to finish the test tasks in fewer turns.

Interesting! Thanks for the details. Hmm, food for thought... What LLM did you use in the tests?

I was using gpt-4o (and gpt-4-turbo, sometimes).

FWIW, I had to do this one-shot learning otherwise StudyRepoForTaskAgent, backed by GPT-4o or GPT-4-turbo, consistently writes code to finish the task by itself, no matter how much I emphasize it should not make changes/write data/implement the task by itself.

What's more interesting is, if I copy the full prompt (without this example) to ChatGPT-4, it can give reasonable results - calling finish action once it sees the repository is empty.