fix(flows): add result.json as second success-confirmation channel - #234
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![preloop-staging[bot]](https://avatars.githubusercontent.com/in/2720497?s=60&v=4){kind=small}
🔍 Preloop Code ReviewLast Updated: 2026-08-17 📝 SummaryThe incremental delta (commit ✅ What Looks Good
✅ Resolved Issues
Progress: 4 of 4 issues addressed This summary updates automatically on each review. Inline comments provide detailed feedback on specific lines. |
The sentinel is a deliberate fail-closed positive-confirmation contract (3902606): agent CLIs exit 0 even when the agent died mid-task, so exit code 0 is never sufficient for success. But the single channel produced false negatives: execution ff1294e1 completed a full PR review (review verifiably submitted via MCP, exit 0) yet was FAILED because the model forgot to print FLOW_EXECUTION_SUCCESS after a 3.7M-token run. Reduce false negatives without weakening the contract, via redundant explicit confirmation channels: - A /workspace/result.json artifact with a success status (including the preloop.eval.result/v1 "pass" vocabulary) now counts as positive confirmation, equal in standing to the printed sentinel. Either explicit act suffices; neither present -> FAILED (unchanged). - An explicit failure status in result.json wins over everything, including a printed sentinel. - The injected instruction stays at the very END of the resolved prompt (recency) and now states the result.json alternative. - When a run fails ONLY for missing confirmation (exit 0, no error heuristics), the error_message says so explicitly and names both channels, so operators can tell this class apart from real failures. - Preset 002 (PR reviewer) gains a mandatory final step writing result.json ({status, risk_level, findings_count, review_action}) as its completion act. (No EE preset variant lives in this repo.) Tests cover the contract matrix: exit 0 + neither channel -> FAILED; exit 0 + sentinel -> SUCCEEDED; exit 0 + result.json success -> SUCCEEDED; result.json failure + sentinel -> FAILED; nonzero exit -> FAILED despite both channels claiming success.
status=fail means the subject's checks failed but the eval finished. Only error/failed/failure override the flow to FAILED. Grace-period failure override now carries exit_code for retry decisions. Co-authored-by: Cursor <cursoragent@cursor.com>
d402d1b to
408f34b
Compare
There was a problem hiding this comment.
💬 Preloop has suggestions for this PR. All three previously raised code findings are now resolved — nice work. Only a documentation gap remains (ARCHITECTURE.md still describes the result.json artifact contract as passive). See the summary comment for details.
Document the active status contract so operators can distinguish flow failures from customer-owned eval verdicts. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Review cleanup is complete on current HEAD
|
| IMPORTANT: When you have successfully completed your task, you MUST print the following marker on a line by itself: {FLOW_SUCCESS_SENTINEL} | ||
| Do not include any other text on the same line as the marker. This signals successful completion. | ||
| IMPORTANT: When you have successfully completed your task, you MUST confirm success in one of two ways: print the following marker on a line by itself (no other text on that line): {FLOW_SUCCESS_SENTINEL} | ||
| or write /workspace/result.json containing {{"status": "success"}}. Without one of these confirmations the run is marked FAILED. |
There was a problem hiding this comment.
[MEDIUM] [Quality]
Affected files:
backend/preloop/services/flow_orchestrator.py:81-86backend/preloop/services/flow_orchestrator.py:866backend/presets/003-observe-eval.yaml:40-44
FLOW_SUCCESS_INSTRUCTION is written in review vocabulary — write /workspace/result.json containing {"status": "success"} — and offers the sentinel as the alternative channel, but it is appended unconditionally to every resolved prompt (line 866), including Observe/Eval flows. The 003 preset tells its agent the opposite: result.json must use status: pass | fail | error (003-observe-eval.yaml:50), and it must "Do NOT print sentinel markers or paste the JSON into your chat output" (003-observe-eval.yaml:43).
Because the injected instruction is the last thing the agent sees (recency, per your own comment at line 861), an eval agent risks (a) overwriting its structured report (schema/summary/metrics/checks) with a bare {"status": "success"}, silently destroying the deliverable the downstream verifier depends on while the flow still reports SUCCEEDED, or (b) printing a sentinel the preset explicitly forbids. The status logic is correct (pass/fail map to success confirmation), but the instruction never tells the eval agent that its own pass/fail verdict already satisfies the contract, so it has no reason not to "helpfully" rewrite the file.
Recommendation:
Make the injected instruction vocabulary-aware. Either exempt Observe/Eval flows from the {"status": "success"} example (their preset already mandates a result.json), or reword the result.json channel so it accepts the flow's own status vocabulary — e.g. "write /workspace/result.json with a completion status (the eval preset's pass/fail verdicts qualify)" — so eval agents don't print a forbidden sentinel or clobber their report.
| or write /workspace/result.json containing {{"status": "success"}}. Without one of these confirmations the run is marked FAILED. | |
| or write /workspace/result.json with a completion status (the eval preset's | |
| "pass"/"fail" verdicts qualify). Without a confirmation the run is marked FAILED. |
There was a problem hiding this comment.
Fixed in 1ca8f90. Prompt resolution now selects an eval-specific final instruction when it sees preloop.eval.result/v1 or the existing no-sentinel directive. That instruction treats the structured report as the confirmation channel, preserves richer report fields, and explains pass/fail/error without offering the sentinel or a bare success object. Normal flows retain both confirmation channels.
Keep the final completion instruction from overriding Observe/Eval vocabulary or encouraging agents to clobber structured reports. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Fixed the new MEDIUM completion-instruction review in
Tested with: Result: 3 passed, 55 deselected. |
The success-confirmation tests from #234 still patched flow_orchestrator.create_agent_executor, which this branch replaced with create_executor_for_execution. Document runner_pool on the flow schema and CI trigger guide, and drop the leftover CHANGELOG line from the merge. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Problem: sentinel false negatives
The success sentinel is a deliberate fail-closed positive-confirmation contract (3902606): agent CLIs exit 0 even when the agent died mid-task, so exit code 0 is never sufficient for success. That ruling stands — this PR does not trust exit codes (unlike closed PR #232, which went the wrong direction).
The problem is false negatives: execution
ff1294e1(staging, internal id) completed a full PR review — review verifiably submitted via MCP, exit 0 — but forgot to printFLOW_EXECUTION_SUCCESSafter a 3.7M-token run, and was marked FAILED.Fix: redundant explicit confirmation channels
Success still requires an explicit act by the agent, now through either of two channels:
FLOW_EXECUTION_SUCCESSsentinel in logs (armed only afterPRELOOP_AGENT_EXEC_START)./workspace/result.jsonwith a success status (success/succeeded/pass/passed— includes thepreloop.eval.result/v1vocabulary). Captured first-class via the artifact path merged in feat(flows): Observe / Eval preset with first-class result.json artifact #231.Rules:
Hardening & diagnostics
result.jsonalternative.error_messagesays so explicitly and names both channels, so operators can tell this class apart from real failures at a glance. New milestones:success_confirmation_missing,result_artifact_failure_override,result_artifact_confirmed_success.result.jsonwith{status, risk_level, findings_count, review_action}as its completion act. No EE preset variant lives in this repo, so nothing to mirror here.Tests
New
TestSuccessConfirmationChannelscovers the contract matrix:backend/tests/test_flow_orchestrator.py(38 passed), preset/container suites (97 passed), and the full non-integration backend suite pass against a local dockerized Postgres (pgvector) afteralembic upgrade head; the only failures are 13 pre-existing on cleanmain(auth/roles/issue-duplicates, unrelated). ruff check/format clean.Note
Medium
Accepts
result.jsonas a second success-confirmation channel; the priorfail-conflation concern is resolved, but the injected confirmation instruction now conflicts with the Observe/Eval preset's result.json contract.Overview
Adds
/workspace/result.jsonas a redundant success-confirmation channel alongside the printedFLOW_EXECUTION_SUCCESSsentinel, keeps nonzero-exit failure authoritative, treats evalpass/failas completed-run verdicts (success confirmation), and adds a mandatoryresult.jsonstep to the PR-reviewer preset, with a contract-matrix test suite.Written by Preloop PR Reviewer for commit 4e15172. Updates automatically on new commits.