Skip to content

agent: the finish check asks for the result as requested, not as the checks left it - #7120

Merged
senamakel merged 1 commit into
tinyhumansai:mainfrom
sanil-23:pr/finish-check-leave-as-requested
Oct 8, 2026
Merged

senamakel merged 1 commit into
tinyhumansai:mainfrom
sanil-23:pr/finish-check-leave-as-requested

Conversation

@sanil-23

@sanil-23 sanil-23 commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Two Terminal-Bench 2.0 trials on 2026-10-08 had every requirement ticked and failed on
what the agent's own work left behind.

polyglot-c-py: the request was one file, runnable by python3 and compilable by gcc. The
agent verified the C path with the request's own command, gcc main.py.c -o cmain, and
finished with cmain beside the deliverable; the check "only main.py.c" was then false.

sanitize-git-repo: asked to replace the secrets in a repository's files with placeholders
and told not to modify anything uncontaminated, the agent also rewrote the repository's
history to purge the old values. The check that diffs against the original commit found
that commit gone.

Step 5 of the finish check: remove what your checks created that the request did not ask
for (build outputs, compiled binaries, scratch and test files), confirm that what remains
is exactly what was asked for, and treat an irreversible change the request did not ask
for (rewriting history, deleting or resetting data, changing settings) as something to
say in the answer rather than do. A user who asked for one file does not want a stray
binary next to it, and nobody wants their git history rewritten unasked.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Verification now checks that equivalent workflows produce consistent outcomes, notifications, and state changes.
    • Unrequested artifacts created during verification are removed, and unrequested irreversible changes are avoided. When a change is warranted, it is reported in the final response rather than being made without request.
    • These checks help ensure verification results are consistent and that its side effects are limited and clearly communicated.

…s the checks left it

Two Terminal-Bench 2.0 trials on 2026-10-08 had every requirement ticked and failed on
what the agent's own work left behind.

polyglot-c-py: the request was one file, runnable by python3 and compilable by gcc. The
agent verified the C path with the request's own command, `gcc main.py.c -o cmain`, and
finished with `cmain` beside the deliverable; the check "only main.py.c" was then false.

sanitize-git-repo: asked to replace the secrets in a repository's files with placeholders
and told not to modify anything uncontaminated, the agent also rewrote the repository's
history to purge the old values. The check that diffs against the original commit found
that commit gone.

Step 5 of the finish check: remove what your checks created that the request did not ask
for (build outputs, compiled binaries, scratch and test files), confirm that what remains
is exactly what was asked for, and treat an irreversible change the request did not ask
for (rewriting history, deleting or resetting data, changing settings) as something to
say in the answer rather than do. A user who asked for one file does not want a stray
binary next to it, and nobody wants their git history rewritten unasked.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: af142f21-4fda-4d3e-a387-1ea8fd3dac6e
📥 Commits

Reviewing files that changed from the base of the PR and between 9754a92 and a27a2cd.

📒 Files selected for processing (2)
  • crates/openhuman-core/src/agent/tinyagents/verify_before_finish.rs
  • crates/openhuman-core/src/agent/tinyagents/verify_before_finish_tests.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The verification instruction now covers consistency across equivalent workflows, cleanup of unrequested artifacts, and avoiding unrequested irreversible changes. Its content test checks for this guidance.

Changes

Verification Guidance

Layer / File(s) Summary
Instruction and content test
crates/openhuman-core/src/agent/tinyagents/verify_before_finish.rs, crates/openhuman-core/src/agent/tinyagents/verify_before_finish_tests.rs
The check instruction adds guidance on consistent outcomes across equivalent workflows, removing unrequested artifacts, avoiding unrequested irreversible changes, and reporting warranted changes. The test checks for this guidance, including avoiding history rewrites.

Priority: ➖ Normal

Estimated code review effort: 1 (Trivial) | ~4 minutes

Change: Bug fix

Suggested reviewers: senamakel

Merge Risk: ⚪ Minimal · up to a27a2

The finish check directs cleanup of verification-created artifacts and reporting instead of making unrequested irreversible changes. No concrete remaining risk to user files is established, so merge risk is minimal pending normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to a27a2

The guidance discourages irreversible actions and limits cleanup to temporary outputs. Residual uncertainty concerns identifying those outputs correctly and preserving existing data during interrupted or concurrent cleanup.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — Any cleanup effects occur through tools already registered for the eligible root turn. The PR adds no tool, credential, or execution privilege. The precise filesystem, service, and data-store exposure still depends on the caller-supplied tool set and existing policies, whose complete mutation contracts were not inspected.

Trust Boundaries and Controls

  • observed — The changed instruction is fixed harness text, framed separately from operator content. Its cleanup command is expressly limited to unrequested artifacts created by checks, while unrequested irreversible history, data, and settings changes must be disclosed rather than performed. The surrounding instruction requires interpreting ambiguity against the original request.
  • observed — Existing tool-boundary controls remain unchanged: declared sandbox requirements and CLI/RPC-only restrictions are installed, while approval and configured tool-policy enforcement are conditional. Approval depends on external-effect declarations and configuration; these controls do not establish universal ownership-checked local cleanup.

Resilience and Maintainability Implications

  • inferred — Continuation recovery is distinct from cleanup recovery. FinishActivity records tool rounds and names, not artifact creator identities. The added assertions verify wording, not preservation of pre-existing files, cleanup idempotency, atomicity, or safety after interruption and concurrent mutation. Those properties remain coverage gaps, not demonstrated failures.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: the finish check must ensure the final result matches the request instead of preserving artifacts or other unintended changes from verification.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

A rabbit checks the paths with care
It leaves no extra bits to spare
It keeps each outcome in its place
And tells what changed with honest grace
Then hops away, its work complete

Comment @coderabbitai help to get the list of available commands.

@tinysweeper

tinysweeper Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Tiny Sweeper review

Tiny Sweeper reviewed this change across 6 lane(s) and found 0 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below.

State: Reviewing pending checks
Priority: none
Reviewed head: a27a2cdcd2ce
Updated: 1791440729 (Unix time)

Review snapshot

Change surface Files Review signal Count
Production 1 Active findings 0
Tests 1 Noted findings 0
Documentation 0 Resolved findings 0
Configuration 0 Pending checks/questions 4

Completeness: Complete
Test assessment: No supported feature-to-test mapping was available; this does not mean tests are absent or passed.

What changed

The review could not produce a supported behavioral summary; inspect the cited changed surface and lane details below.

Features

None identified with supported citations.

Tests

No supported feature-to-test mapping was produced. Test execution is not inferred.

Findings

No active actionable findings.

Pending checks: Rust E2E (mock backend), Build Playwright E2E Artifact, E2E (Playwright / web lane), Desktop E2E (full suite, 3 OS)

Before merge

  • Wait for Rust E2E (mock backend), Build Playwright E2E Artifact, E2E (Playwright / web lane), Desktop E2E (full suite, 3 OS).
Agent review details

critique

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The change adds cleanup and side-effect verification guidance to the finish check and updates its unit test accordingly. It is safe to merge. _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._

security

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The change extends the pre-finish verification guidance to cover cleanup and unintended irreversible actions, with matching test coverage. It introduces no security issue and is safe to merge. _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._

tests

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The change extends the verify-before-finish prompt with a cleanup/no-irreversible-actions rule, and the existing needle-based test in verify_before_finish_tests.rs is updated to pin the new text, so a regression to the prompt would fail the suite. This is prompt-text content with matching coverage; it looks sound and safe to merge. _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._

commits

  • Conclusion: Neutral
  • Scope reviewed: all assigned evidence
  • Lane summary: Nothing sensitive found in what this pull request commits.

description

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: This change adds a fifth step to the finish-check prompt text and extends the needle-based test to cover it, matching the PR description exactly. The change looks sound. _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._

e2e

  • Conclusion: Neutral
  • Scope reviewed: all assigned evidence
  • Lane summary: This change only extends the verify-before-finish prompt text in the core and its sibling unit-test needle list; it adds no route, flag, command, or persisted format that an end-to-end harness could drive, and prompt wording is not verifiable by any end-to-end test. The change looks sound. Waiting on end-to-end jobs: `Rust E2E (mock backend)`, `Build Playwright E2E Artifact`, `E2E (Playwright / web lane)`, `Desktop E2E (full suite, 3 OS)`.
  • Unresolved questions/checks: Rust E2E (mock backend), Build Playwright E2E Artifact, E2E (Playwright / web lane), Desktop E2E (full suite, 3 OS)
Evidence and run details
  • Models: gpt-5.6-luna, glm-5.3-flash
  • Spend: $0.001356
  • Tokens: 40746 input · 2514 output · 8420 cached · 0 embedding
Head State Pass summary
a27a2cdcd2ce pending 0 active finding(s), 0 resolved finding(s) (at 1791440729)

tinysweeper 0.1.0

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tinysweeper found nothing blocking. Approving.

             $0.0014 · 40,746 in / 2,514 out · 8,420 cached (21%) · gpt-5.6-luna, glm-5.3-flash
critique:    $0.0006 · 12,211 in / 471 out   · 2,124 cached (17%) · gpt-5.6-luna
security:    $0.0006 · 11,204 in / 709 out   · 1,880 cached (17%) · gpt-5.6-luna
tests:       $0.0000 · 4,689 in  / 150 out   · 1,856 cached (40%) · glm-5.3-flash
description: $0.0000 · 4,357 in  / 42 out    · 2,368 cached (54%) · glm-5.3-flash
e2e:         $0.0000 · 5,556 in  / 129 out   · 64 cached (1%)     · glm-5.3-flash

@tinysweeper tinysweeper Bot added the priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect. label Oct 8, 2026
@senamakel
senamakel merged commit d93adb0 into tinyhumansai:main Oct 8, 2026
24 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants