Continue Fiori reliability follow-up fixes - #594
Merged
Merged
Conversation
…tion reasons
Two small, independent fixes found while chasing the Phase 0c baseline's
remaining failures.
1. force_kill_pid (web.rs, from the Chrome-leak fix) used `.status()`,
which inherits this process's own stdout/stderr. The common case is a
PID that's already gone (the graceful CDP close usually worked), so
`kill`/`taskkill` printing "No such process" on every routine success
was leaking into flowproof's own error output - observed firsthand in
two of this baseline run's failure reports. Both call sites (the real
fix and its own e2e test's liveness check) now redirect the child's
stdout/stderr to null.
2. scripts/fiori-eval.py's score_spec only checked a record response for
an "error" key. A response shaped as `{"needs_clarification": {...}}`
(flowproof's own well-formed way of saying "I don't know how to
proceed, here's why and here's the scene") fell through neither
branch and got reported as an opaque "exit 2" - exactly what happened
to medium-05-search-retry-after-no-match.flow.yaml in the last two
baseline runs, hiding its real, already-diagnosed reason (a rule_step
target that lost its iframe scope) behind a code that told a future
reader nothing. Now surfaces `needs_clarification`'s own reason and
hint directly.
Root-cause: both are the same shape of bug - an internal detail (a
subprocess's own stdio, a structured non-error response shape) that
wasn't being suppressed/handled, so it leaked through as noise instead of
either staying invisible or being reported usefully.
Evidence: crates/flowproof-adapters/tests/shared_browser_shutdown_e2e.rs
re-run clean, no "No such process" in its output (previously present).
fiori-eval.py change verified by ast.parse; cargo fmt/clippy clean.
… clean 33.33%
evals/fiori/20260914T133044Z.json: FAA 0.00% - the corporate Fiori host
(fiorits3.cnt-online.at) was completely unreachable for roughly two hours
("Network is unreachable" at the OS routing level, confirmed with curl/
traceroute - a dropped VPN or corporate network path, not an application
issue). Every browser-dependent spec failed at navigation; even the
pure-API short-04 failed. Kept as evidence rather than discarded, per this
branch's own convention of archiving every real run.
evals/fiori/20260914T185311Z.json: FAA 33.33% (4/12) - the first genuinely
clean run: network confirmed working, and past both the Chrome-leak fix
and the PTP-tile corpus correction from two commits ago. All four short
specs pass, including short-02/short-03, directly confirming the corpus
fix. Negative controls held. The remaining 8 failures are now real,
uncontaminated signal - see docs/fiori-reliability/FINDINGS.md for the
per-spec breakdown feeding the next round of fixes.
Trace files re-recorded fresh by these two scoring runs (score_spec always
re-records, by design, to measure FAA honestly) update three already-
committed cassettes (short-01, medium-02, medium-04) and add two new ones
(short-02, short-03, matching this corpus's PTP-tile correction). Per the
precedent set explicitly with the user for this same situation earlier
tonight (keep the new content, commit deliberately rather than silently):
committed here as a deliberate part of documenting these two runs, not a
side effect.
evals/fiori/dev/medium-05-search-retry-after-no-match.flow.yaml failed
with `rule_step target 'Text("Material")' is not one of the listed scene
targets`. The model's authored rule_step lost the field's iframe scope,
resolving to a bare, unscoped Text("Material") the flat top-level scene
never lists (the real field only exists inside the "Application" iframe).
Root cause: the system prompt's generic rule_step instruction ("copy
listed target tokens into quoted targets") is impossible to follow
correctly for a framed or scoped element. Their token IS a compound
string containing its own embedded quote marks
(`framed:"<frame>" > <inner>`, `scoped:<container> containing "<anchor>"
> <inner>`), and rule_step's own quoting is naive: `quoted_label` (rules.rs)
takes the substring up to the FIRST `"`, not a balanced pair. Pasting
`framed:"container" > css:#foo` into `Clear the "..." field` therefore
parses "framed:" as the label and leaves the rest as unparseable garbage
- this is not one specific model's mistake, it is a structural
incompatibility between the prompt's own advice and the grammar's
quoting for every framed/scoped token, on every target of that shape.
Fix: the prompt now carves out an explicit exception for scope-bearing
entries - use the plain scope.inner token in quotes plus the natural
clause the deterministic grammar already parses correctly (`in the
iframe "<frame>"` for scope.frame, `in the item containing "<anchor>"`
for scope.container), with a concrete worked example. This is a prompt-
side fix; a live model's actual phrasing choice can't be asserted by a
scripted-backend test, so the tests instead prove the mechanism the new
guidance points to: that once a rule_step correctly uses the scope
clause, the whole pipeline (parse -> validate against the scene) resolves
to the right, properly framed target - and that pasting the raw compound
token, the thing this fix tells the model not to do, is rejected loudly
by a grammar parse error rather than silently mis-resolving.
Evidence: two new tests in crates/flowproof-agent/src/author.rs.
rule_step_grounds_a_framed_target_via_the_scope_clause_not_the_raw_token
proves the escape hatch end to end against the existing
HUMAN_PRIMITIVE_SCENE fixture's iframe-scoped input.
rule_step_rejects_a_raw_framed_token_pasted_into_quotes proves the
failure mode this fix targets fails loudly, not silently. Full
flowproof-agent suite: 387 passed, 0 failed (up from 377 before tonight's
merged main). fmt/clippy clean.
Not yet re-verified live: the real Fiori system started returning 503
Service Unavailable partway through tonight, independent of this bug.
Will re-record medium-05 once it recovers to confirm the model actually
follows the new instruction in practice, not just that the mechanism it
points to works.
…elements, close a real leak in e2e tests, fix a race in the fix Three related findings from tonight's Phase 0c corpus work, landed together because the second and third were only found while properly testing the first. 1. medium-02-change-info-record-readonly.flow.yaml failed live with "driver transport fault: probing an iframe: Unable to make method calls because underlying connection is closed". `probe_frame`'s CDP call talks to the tab directly and had NO retry at all, unlike every `with_element`-routed call (which already gets one automatic retry on exactly this class of error via `is_transport_fault`). A transient CDP hiccup during an iframe probe killed the whole flow with zero chance to recover, while the same hiccup during an element-scoped call already recovered fine. Root-cause: extracted the retry decision itself into `retry_once_on_transport_fault`, a small pure function `probe_frame` now uses - deliberately NOT touching `with_element`'s own working, more heavily-relied-upon inline version, to keep this change's blast radius to the one call site with actual evidence behind it. 2. While adding this fix's own tests, cargo test's default parallelism surfaced a genuinely separate, pre-existing leak: every e2e test that constructs a WebAppDriver uses the shared-browser path (the default), and unlike flowproof-cli's own invocations (fixed earlier tonight via SharedBrowserGuard), no test ever called shutdown_shared_browser() - so every e2e test run leaked its own Chrome process tree and profile dir, independent of the CLI-invocation leak already fixed. Exposed by ps showing 18 orphaned Chrome trees after one ordinary e2e run. Fix: exported SharedBrowserGuard from flowproof-adapters (previously a private struct local to flowproof-cli) and added it to every e2e test that launches the shared browser (a11y_capture, drag_spike, network_idle_e2e, static_area_e2e). 3. Adding a guard per test immediately reproduced a NEW race: network_idle_e2e's two tests, run concurrently by cargo test's default threading, share the one process-global browser - the first test's own guard unconditionally killed it while the second was still mid-launch, reproducing "Unable to make method calls because underlying connection is closed" a third, unrelated way. Caught by actually running the new fix repeatedly, not assumed correct from a single green run. Fix: SharedBrowserGuard now reference-counts (GUARD_COUNT); only the last guard to drop actually shuts anything down. flowproof-cli's own single-guard-per-process usage degrades to the same behavior it had before (1 -> 0 still triggers shutdown), so this is a pure widening, not a behavior change for the CLI path. Evidence: two new unit tests for retry_once_on_transport_fault (recovers on one retry; a fault that never clears still fails after exactly one, not an infinite loop; a non-transport error is never retried at all) and one for the reference count (a held sibling guard keeps the count above zero, proving the exact race scenario above cannot recur). network_idle_e2e's two tests, previously failing under contention, now pass together on three consecutive runs. flowproof-adapters (29) and flowproof-cli (122) lib suites green; fmt/clippy clean; zero leaked Chrome processes or profile dirs confirmed after every e2e run in this session.
Same gap as probe_frame, found by checking the one other direct-to-tab call this session flagged as "not yet checked" while writing up tonight's findings: frame_act's CDP evaluate call - the mechanism type_text's framed path uses for every field inside an iframe (also what frame_value, and therefore the value-help "accepted a different value after commit" readback, reuses) - had no transport-fault retry either. A transient CDP hiccup here killed the flow the same way it did in probe_frame before that fix, on what is probably the single most exercised iframe call in the whole corpus (every Type/Clear/value-help step against the classic SAP GUI iframe goes through it). Root-cause: identical to the probe_frame fix two commits ago - a direct tab().evaluate() call with no element to re-resolve never went through with_element's retry, so it got none at all. Reuses the same retry_once_on_transport_fault helper and its existing tests; no new test needed for the decision logic itself; net effect is verified by the e2e suite still using this exact function to drive real iframe content (static_area_e2e) and by cargo fmt/clippy on the change. Also updates docs/fiori-reliability/FINDINGS.md with the full account of tonight's autonomous continuation: the branch collision with a colleague's merged PR-review fixes and the move to fiori-reliability/2026-09-14-v2, and all three findings fixed so far (the rule_step scope-clause prompt fix, probe_frame's retry, and the leak+race found while testing it) - plus an honest list of what remains open (the value-help mechanism itself still unconfirmed, medium-03's timing needs a live re-record to distinguish real variance from a wrong element, long-02 unexplained) and cannot be closed without the corporate system, still down with a 503, coming back.
…continued work The report was still describing the 2-spec smoke-corpus state from earlier tonight - no FAA baseline, no corpus scored. Updates the summary, gate status, numbers, and control-results sections to the current, true state: the 14-spec Phase 0c dev corpus has now been scored three times (16.67% contaminated, 0.00% network outage, 33.33% clean), five real fixes beyond H1/H2/H5 have landed (the timeout bug, the Chrome-process leak, the rule_step scope-clause gap, and two transport-fault retry gaps), and negative controls have held in every real run. Still an honest report: 33.33% on a hand-written corpus is not the gate, the generator does not exist, and three findings remain genuinely open pending a live re-verification the corporate system's ongoing 503 has not allowed yet.
…ium-02's status While waiting on the corporate system's continued 503, checked whether an inter-action settle-wait exists during live recording (the same class of gap H2 fixed for scene-reading, but between individual actions within or across steps) - it does not. settled_scene only runs once per grounding round (inside scene()); wait_actionable, the closest equivalent, exists only in flowproof-replay, not the recorder's live path. This is a structurally plausible explanation for the value-help commit mismatch (a value-help selection's own backend round-trip not being waited for before the next action fires) but is explicitly recorded as an unconfirmed hypothesis, not acted on - a genuine fix here would be a new mechanism, not a small extension of proven code, and should not be built without live confirmation. Also updates medium-02's status: very likely (not yet confirmed) closed by the frame_act retry fix, since that is the exact call type_text's framed path uses and medium-02 types into several framed fields.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification