Skip to content

Add fail-closed goal verifier to completion resolution - #281

Open
raymondginger2018-sudo wants to merge 1 commit into
HKUDS:mainfrom
raymondginger2018-sudo:pr/goal-verifier
Open

raymondginger2018-sudo wants to merge 1 commit into
HKUDS:mainfrom
raymondginger2018-sudo:pr/goal-verifier

Conversation

@raymondginger2018-sudo

Copy link
Copy Markdown
Contributor

Summary

Closes the goal-completion verify loop: a registered async verifier decides whether a goal request may complete, and rejections keep the agent working instead of falsely completing. Borrowed from harness-style goal verification. Dormant unless a verifier is configured — with none registered, behavior is exactly as before.

Changes

  • New core/agent_runtime/goal_verifier.py: GoalRuntimeRouter.configure_completion_verifier registration surface plus verdict normalization helpers.
  • core/agent_runtime/goal_runtime.py:
    • resolve_completion(...) awaits the registered verifier (handles both coroutine and sync verdicts), then feeds the outcome into request(verified=...).
    • A rejection raises GoalRuntimeError("...keep working.") so the turn continues.
    • Fails closed: a verifier exception or empty evidence means "not completed" (the goal is not falsely closed).
  • core/harness/tools/goal.py: pass the verifier outcome through the goal tool response.
  • New tests/test_goal_completion_verifier.py (approval, rejection, exception, empty-evidence, no-verifier paths).

Tests

  • tests/test_goal_completion_verifier.py + tests/test_goal_runtime.py — 19 passed
  • CI-exact lint: pre-commit run --all-files (ruff 0.15.21) — passed
  • Local preflight verdict recorded against this revision (baseline reds unchanged)

Notes

  • Independent PR; no follow-up required for callers that don't register a verifier.

@Zongwei9888

Copy link
Copy Markdown
Collaborator

Thanks for putting this together, @raymondginger2018-sudo. Checking a goal's completion against evidence is something we care about, but we can't merge this version.

When an async verifier rejects (including model_backed_verifier answering "no") or raises, UpdateGoalTool calls request(verified=False), _confirm_completion (goal_runtime.py:195-198) calls the verifier again synchronously, and bool(coroutine) is True, so the goal is recorded as complete. Every rejection also runs the verifier twice, a sync verifier that raises escapes as a raw RuntimeError, and the documented .verify protocol (RuleBasedVerifier) raises TypeError because the router calls verifier(request).

Nothing in the app registers a verifier, so the feature would ship switched off. Users can already veto completion today with a PreToolUse hook on update_goal, and the model sees the hook's reason.

A version we could accept would:

  • call the verifier exactly once, asynchronously, with no public verified bypass;
  • pass the real goal objective and the Turn's existing evidence references (today objective is always "");
  • return the verifier's justification to the model, and move the goal to blocked after N rejections instead of burning Turns;
  • include a real caller (a setting or the hook path), plus an end-to-end test through UpdateGoalTool with an async verifier that rejects;
  • drop the unused classes (GoalVerifierRejected, GoalCompletionVerifierPolicy) and the references to other products.

If that's more than you want to take on, we'll close this for now. Thanks again!

…idence, blocked-after-N, drop unused classes

Address @Zongwei9888's five review points on the goal completion verifier:

1. Verifier runs exactly once, asynchronously, before any durable update.
   GoalRuntimeRouter.request() is now async and awaits the verifier once;
   there is no 'verified' bypass parameter. A rejection returns data and
   never calls handler.update_goal, so no completion is recorded. A sync
   verifier that raises is caught by the same try and fails closed (rejects)
   instead of escaping as a raw RuntimeError.

2. The claim handed to the verifier carries the real goal objective (no
   longer hardcoded empty) plus this Turn's existing evidence references,
   via GoalExtension.completion_request(), reusing the outcome's evidence
   filter and caps.

3. A rejection returns the verifier's justification to the model as a
   structured dict; after max_rejections (default 3) the router itself
   moves the Goal to blocked and freezes the Turn instead of letting the
   model burn Turns retrying.

4. Real caller: build_agent_session arms RuleBasedVerifier when
   config.goal.completion_verifier == 'rule' (new GoalConfig, default
   'off' preserves existing behavior). E2e test drives UpdateGoalTool
   through the ToolRegistry with an async verifier that rejects.

5. Drop the unused GoalVerifierRejected and GoalCompletionVerifierPolicy
   classes and the .verify protocol; goal_verifier.py is stdlib-only,
   import-inert, and has no references to other products.
@raymondginger2018-sudo

Copy link
Copy Markdown
Contributor Author

Hi @Zongwei9888,

Thanks for the detailed review — the double-call and bool(coroutine) bugs you found were exactly right. I've reworked the PR against each point you listed.

1. Verifier called exactly once, asynchronously, no verified bypass

GoalRuntimeRouter.request() is now async def and awaits the verifier exactly once, before any durable update:

if status == "complete" and verifier is not None:
    try:
        verdict = await verifier(handler.completion_request(context, reason=reason))
    except Exception as exc:
        verdict = GoalVerdict(confirmed=False, justification=f"Completion verification failed: {exc}")
    if not verdict.confirmed:
        return self._reject_completion(handler, context, verdict)

There is no verified parameter on request() (or anywhere), so there's no public bypass. A rejection returns data and does not call handler.update_goal — no completion is recorded. There is no synchronous re-check, so the verifier runs once per claim. A sync verifier that raises is caught by the same try and fails closed (rejects) rather than escaping as a raw RuntimeError.

2. Real objective + the Turn's existing evidence

GoalExtension.completion_request() builds the claim handed to the verifier: the actual goal.objective (no longer "") plus this Turn's evidence references, using the same _EVIDENCE_KINDS filter and the existing 12-item / 240-char caps as the outcome. The verifier judges the real objective against the evidence the Turn already recorded.

3. Justification returned to the model; blocked after N rejections

A rejection returns a structured dict to the model — {"updated": False, "verdict": {"confirmed": False, "justification": ...}, "rejections": n, "maxRejections": N} — so the model sees why it was rejected and can either produce the missing evidence or declare the Goal blocked. After max_rejections (default 3) the router itself moves the Goal to blocked and freezes the Turn, instead of letting the model burn Turns retrying the same claim. The rejection counter spans Turns (it's cleared on success, auto-block, or reconfigure).

4. Real caller + e2e test through UpdateGoalTool

  • Caller: build_agent_session in core/agent_setup.py arms RuleBasedVerifier when config.goal.completion_verifier == "rule" (new GoalConfig, default "off" preserves existing behavior).
  • Test: tests/test_goal_completion_verifier.py drives UpdateGoalTool through the ToolRegistry with an async verifier that rejects — asserting the verifier ran exactly once, the justification reached the model, no durable complete was recorded, the Turn did not freeze on the first rejection, and that after N rejections the Goal auto-blocks and freezes.

5. Dropped unused classes and other-product references

GoalVerifierRejected and GoalCompletionVerifierPolicy are gone. core/agent_runtime/goal_verifier.py is stdlib-only (dataclasses + typing) and import-inert — no references to other products.

The documented .verify protocol is also gone: the contract is now a single async def __call__(request) -> GoalVerdict, so RuleBasedVerifier and model_backed_verifier both satisfy it without adaptation.

Local checks: tests/test_goal_runtime.py (16) and tests/test_goal_completion_verifier.py (12) pass, as does the automation suite I touched; pre-commit (ruff format + check) and the secret scan are green. I've force-pushed the rework to this branch (d3470652). Happy to adjust the default or the auto-block threshold if you'd prefer different wiring.

Thanks again for the careful review.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants