#458

fix: require real delegation before the primary agent reports a successful subtask

Openlalith0407wants to merge intofrom

Description of the Change

Problem

The primary agent's done barrier handler only validates that success is set before storing the subtask result. It never checks whether any work was actually delegated or executed, so the model can call done{success:true, result:"<made-up command output>"} with zero delegations and the fabricated result is stored and reported to the user. This was seen on a simple "print the hostname" flow that reported a hostname with no termlogs entry (nothing ran). Reproduced at ~2/5 on a clean OpenAI stack.

Solution

Add a lightweight evidence check to the primary agent's done path:

  • After Done.Validate(), checkDoneEvidence looks up the subtask's tool calls via the existing GetSubtaskToolcalls query. If success:true but no delegation (AgentToolType: coder/maintenance/pentester/search/memorist/advice) happened in this subtask, the handler returns a new *tools.BarrierDeclinedError.
  • A declined barrier is soft: performer.go feeds the reason back to the model as the tool response and keeps the chain running (it is not routed through argument-correction retries), so the agent can delegate and try again — no flow failure.
  • Escape hatches (backward compatible, no schema change): a success:false (failure/blocked, or "cannot verify / no execution needed") result is always accepted; and after one rejection a repeated success is accepted with a warning (kept well under the existing repeating-tool-call abort threshold so it can never deadlock a subtask).
  • Fail-open: if the tool-call lookup errors, it logs a warning and accepts the done; a cancelled context is propagated as an error, not a rejection.
  • One reinforcing line added to primary_agent.tmpl.

Scoped to the primary agent only; specialist barriers are untouched.

Closes #457

Type of Change

  • 🐛 Bug fix (non-breaking change which fixes an issue)

Areas Affected

  • AI Agents (Researcher/Developer/Executor)

Testing and Verification

Test Configuration

PentAGI Version: main (h843dfeb)
Docker Version: 29.9.1
Host OS: Debian 13
LLM Provider: OpenAI (default models)
Enabled Features: none extra

Test Steps

  1. cd backend && go test ./pkg/providers/ -run TestDone_ -v
  2. Live: bind-mount the patched binary onto the plain stack and run several "print the hostname of this machine" flows (OpenAI).

Test Results

  • New unit tests (backend/pkg/providers/done_evidence_test.go) cover: success with nothing delegated → rejected; success after a delegation → accepted; any specialist counts; failure result → accepted; rejection cap → accepted with a warning; a delegation in another subtask does not count; lookup error → fail-open; cancelled context → error not rejection; and a declined barrier keeps the chain running without an argument-fix retry. All pass; they fail on the unpatched handler.
  • go vet clean; gofmt clean.
  • Live (5 flows): the guard fired on the 2 flows that were about to report a fabricated hostname with no delegation. One then delegated and returned the real termlog-verified hostname; the other honestly reported failure. 0/5 phantoms shipped vs 2/5 unpatched; 4/5 produced real hostnames.

Security Considerations

Reduces silent fabrication of results, improving the trustworthiness of flow reports. No new dependencies or permissions.

Performance Impact

One extra indexed DB read (GetSubtaskToolcalls) per done on the primary agent. Negligible.

Documentation Updates

  • None required.

Checklist

Code Quality

  • My code follows the project's coding standards
  • I have added tests to cover my changes
  • All new and existing tests pass
  • I have run go fmt and go vet (for Go code)

Security

  • I have considered security implications
  • Changes maintain or improve the security model

Compatibility

  • Changes are backward compatible

Additional Notes

The rejection cap is intentionally tight (accept on the 2nd attempt) to stay within the existing repeating-tool-call abort threshold. Happy to make the cap or the message configurable if preferred.

HUMAN:

While testing PentAGI locally with simple "print the hostname" flows, I noticed that some flows reported a hostname even though nothing had run: the command log for that subtask was empty. The primary agent can't run commands itself, and its done step never checks whether it delegated anything, so a made-up result went straight into the report. I added a check that sends that kind of success back to the agent once, with the reason, so it can delegate and try again. Reporting failure is always accepted, so a subtask can't get stuck. In my runs this cut fabricated results from 2 in 5 flows to 0 in 5. I kept the check strict on purpose, but I'm happy to make the cap configurable, or to add an explicit "nothing needed to run" flag instead of relying on success:false.

Activity

2
AlaeddineMessadireviewed15d ago
AlaeddineMessadi15d ago

The decline doesn't keep the fabricated summary away from the operator. customExecutor.Execute writes the done call's message as a MsglogTypeDone row and publishes it to the UI before the barrier handler runs (executor.go#L321-L328, registry.go#L476-L477, msglog.go#L198-L210). When checkDoneEvidence returns BarrierDeclinedError, Execute returns at executor.go#L388-L392 and never updates that row. subtask.result stays clean, but in the hostname case the message log still shows a done entry with the made-up recap and no result.

The smallest fix I see is in that error branch. If errors.As(err, &declined) and msgID != 0, write declined.Reason with ce.mlp.UpdateMsgResult, so the entry reads as a rejected claim. Holding back the PutMsg for barrier tools until the handler succeeds would hide the entry completely, but that is a bigger change.

None of the new tests go through the real Execute: primaryBarrier calls cfg.Barrier directly, and decliningBarrierExecutor stubs Execute. A case through GetPrimaryExecutor with a recording MsgLogProvider would cover this.