test(query-parity): attribute known failures to their measured causes - #1468
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06dccf3918
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptileai review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8093aecc1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@greptileai review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcd010cda7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47e547cb59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
known_issues.py labelled each case with the first matching id pattern, so a case was often blamed on its scenario (period clipping, equal timestamps) when a transform divergence was the real cause, or the other way round. For example period-clip-p1-flood was labelled #162 and does flip with the #162 fix alone, but identical-p0-flood needs both the #163 fix and the Rust flood() port. known_failures.txt now records the issues per case ("A,B" all needed, "A | B" either suffices), and the xfail reason lists them. The keys were measured with the new attribute.py from 23 runs: no fixes, each of the 10 open fix PRs alone (aw-core#161, #165-#169, aw-server-rust#744, #748-#750), all of them, and all but one. Cases that still fail with every fix keep only the causes that have no fix yet (#1466 shapes, ms timestamp precision). The id patterns remain as suggestions for new failures.
…ults A mistyped results directory read as zero failures, and --write then emptied known_failures.txt. Require base.txt (with failures), all.txt and both result files per fix.
…fied combinations - When pruning fixed keys from a case that still fails, prune within each alternative instead of flattening "A | B,C" into a conjunction. - A combination that reduces to a single key its solo run already disproves (or that an optional <KEY>+<KEY>.txt run shows is not enough) is reported as underdetermined, and --write refuses. - Reject all-<KEY>.txt without a matching <KEY>.txt. - Optional <KEY>+<KEY>.txt runs verify combinations; the script reports how many are only inferred from the leave-one-out runs. - Unit tests for attribute.py. All 153 combination specs in known_failures.txt are now confirmed by direct runs of the 11 distinct combinations; the list is unchanged.
… runs The underdetermined diagnostic asked for <KEY>+<KEY>.txt runs but never used them. Fall back to the supplied runs that fix the case and contain every needed key, keeping the minimal ones as alternatives (A,B | A,C).
47e547c to
823f426
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 823f4265ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 823f4265ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Zero-duration filter at the end of another filter ((0,6),(6,0)), at the end of the event ((0,10),(10,0)), and adjacent filters ((0,6),(6,4)), from the review of ActivityWatch/aw-core#166. With ActivityWatch/aw-server-rust#749 both servers agree on all three (half-open filters: [(0,6),(6,0)], [(0,10)], [(0,6),(6,4)]); the pinned Rust drops the zero-duration piece, so those cases are RUST_747. The 36 new known failures are attributed with the same measured runs (each fix alone, all, all but one, and the combinations).
|
@greptileai review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61ab8809d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Rework how a case that some fixes flip gets its spec: collect every set of fixes known or inferred to be enough (single fixes, passing combination runs, the leave-one-out set), keep only the minimal ones as alternatives, and report a measured set that may carry an unneeded key. - A case fixed by A alone and by B+C (neither alone) is now "A | B,C"; the combination run used to be ignored when a single fix sufficed. - A passing superset whose extra key the leave-one-out runs show is not needed, and whose smaller run is missing, is reported instead of written. - Redundant alternatives go: "CORE_163 | CORE_161,CORE_163,RUST_744" is just "CORE_163" (4 entries in known_failures.txt).
Any failing parametrized test with a [ in its node id was recorded as a parity case, so a failing helper test (e.g. test_attribute.py's test_refuses_incomplete_results[missing] from #1468) would have written 'missing' into known_failures.txt. Only count test_query_parity.py cases.
|
@greptileai review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54d0d73180
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…sult files CORE_161+CORE_161.txt collapsed to the single fix, and A+B.txt next to B+A.txt silently kept one of them.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f89a864b58
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if len(keys) == 1: | ||
| return c in single[next(iter(keys))] |
There was a problem hiding this comment.
With exactly two fixes, <A>.txt and all-B.txt represent the same configuration, but this branch always trusts the former while needed is computed from the latter. If the copied runs disagree because one is stale or flaky—for example, A.txt passes while all-B.txt fails—the script can emit A with no problem and allow --write, despite direct evidence that A alone fails. Equivalent single-fix and leave-one-out runs should be checked for agreement and contradictory inputs refused.
Useful? React with 👍 / 👎.
🤖 AI code reviewThis PR reworks the query-parity known-failures tracking: known_failures.txt now stores per-case issue keys (comma = all needed, pipe = alternatives) instead of bare case ids, and a new attribute.py script derives those keys from runs of the suite with each fix alone, all fixes, and all-but-one. known_issues.py gains ISSUES, suggest_spec, spec_keys, spec_reason, read_known_failures, and write_known_failures, and conftest.py uses these to mark xfails and to rewrite the list under --update-known-failures. It also adds three filter_period_intersect boundary scenarios and a unit test file for attribute.py. Not safe to merge — 1 P1 openConfidence 3/5 1 finding · ❌ 1 P1❌ P1 high — In attribute(), the How this was verified: Traced the branch: for a combo run of size n-1 that is not the leave-one-out set, 2 advisory findings (summary-only, not scored)These P2 guard, heuristic, trade-off, or documentation claims are retained for judgment without opening review threads.
In attribute(), the How this was verified: Traced the loop for a single-fix candidate: keys - {k} is empty, fails_with returns True, so no problem is reported. For a combination candidate, the check requires the single-fix runs to exist, which they do because attribute() calls failing() on every fix. No input produces wrong output.
In known_issues.py, How this was verified: Checked the pattern list; period-clip-p1-flood matches both. The README documents this. Files changed (7) — the diff as I read it
Reviewed Maintainer commands
|
| return c in single[next(iter(keys))] | ||
| if keys == frozenset(fixes): | ||
| return c in all_ | ||
| if len(keys) == len(fixes) - 1: |
There was a problem hiding this comment.
❌ P1 — In attribute(), the fails_with helper for a key set of size len(fixes)-1 uses all_but[left_out], but all_but is keyed by the left-out key. If keys is not exactly frozenset(fixes) - {left_out} for some left_out, this branch is not taken. The condition len(keys) == len(fixes) - 1 is checked before the keys == frozenset(fixes) check, so for a set of size n-1 that is not a leave-one-out set (e.g. a combination run of n-1 keys that is not all-but-one), it will incorrectly use all_but[left_out] where left_out is the single key not in keys. But all_but only contains entries for each fix, and left_out is computed as set(fixes) - keys, which is a singleton. So all_but[left_out] is the run with all fixes except that one. If the user supplied a combination run of n-1 keys that is not the leave-one-out set (e.g. they ran CORE_161+CORE_162 when there are three fixes, but that is not all-CORE_163), the code will treat it as the leave-one-out run for the missing key, which is wrong. However, the unknown validation and the combos dict would also contain that run, and fails_with would be called with that key set from the combos iteration, but the branch order means it will use all_but instead of the actual combo run. This is a real bug: a combination run of size n-1 that is not the leave-one-out set is misinterpreted. The README says <KEY>+<KEY>.txt runs are optional and can be any combination, including size n-1. For example, with fixes A, B, C, a run A+B.txt (exactly A and B) would be misinterpreted as all-C.txt (all except C). If the user provides A+B.txt and it passes, the code would think all-C passes, which may be false. This can lead to wrong attribution. The fix is to check keys == frozenset(fixes) - {left_out} before using all_but, or to only use all_but when the key set is exactly a leave-one-out set.
There was a problem hiding this comment.
False positive — this branch cannot misattribute.
fails_with is only ever called with keys ⊆ fixes:
needed = frozenset(k for k in fixes if c in all_but[k])keys - {k}comes fromminimal, whose members are singletons offixesorcomboskeys
For a subset of fixes with size len(fixes) - 1, set(fixes) - keys is necessarily the singleton {left_out} and keys == frozenset(fixes) - {left_out} — so the premise ("a size n-1 run that is not a leave-one-out set") is unsatisfiable: over n distinct keys, any n-1 distinct keys are exactly the complement of one key.
Concretely with fixes = A,B,C: A+B.txt and all-C.txt are the same run. all_but[C] is defined as failing(results / "all-C.txt"), and failing() calls sys.exit when that file is missing, so it is mandatory. Reading all_but[C] for keys = {A,B} therefore returns exactly the A+B result — there is no case where the two can disagree.
Reproduced against the merged head (43e7ce32f0) with a synthetic results dir: base=[c1], all=[], all-A=[c1], all-B=[c1], all-C=[], A+B=[] → python3 attribute.py results prints 1 CORE_161,CORE_162, the intended attribution. Adding the suggested guard (keys == frozenset(fixes) - {left_out}) is a no-op on every reachable input.
Separate, non-blocking observation — not this finding, and only reachable with a malformed results dir: _combo_keys accepts a combo whose parts are ISSUES keys but not necessarily keys in fixes, so <K1>+<K3>.txt where K3 has no <K3>.txt run is not rejected (an orphan all-<K> is rejected). When such a combo has size n-1 it survives into minimal and, filtered through the fixes-only mapping, can add an alternative that contradicts the measured single-fix run. Harmless for the committed list (all 11 combos were run with their singles present); a one-line all(k in fixes for k in parts) guard in _combo_keys would close it.
… cases' known failures (#1469) * fix(query-parity): retry startup polls on read timeouts, keep skipped cases' known failures - Server.start only caught requests.ConnectionError while polling /api/0/info. A server that accepts the connection but takes more than the 1 s poll timeout to answer raises ReadTimeout (not a ConnectionError subclass), which crashed the fixture instead of retrying until the deadline. Catch requests.RequestException. - pytest.skip() inside test_invariant produces a call-phase report with skipped=True, which was counted as run, so --update-known-failures dropped the case's existing entry although it never ran. Skipped reports no longer count as run, as the sessionfinish comment already promised. test_harness.py covers both: a fake server that answers slowly the first time (fails with ReadTimeout before the fix), and record_report with a skipped call report. * fix(query-parity): only record parity cases for --update-known-failures Any failing parametrized test with a [ in its node id was recorded as a parity case, so a failing helper test (e.g. test_attribute.py's test_refuses_incomplete_results[missing] from #1468) would have written 'missing' into known_failures.txt. Only count test_query_parity.py cases.
2 participants
Follow-up to #1467 (merged).
known_issues.pylabelled each known failure with the first id pattern that matched, so the xfail reason often named the wrong issue. With only the aw-core#164 fix applied, 41 cases XPASS, but 6 of them were labelled #162 or #163. Conversely,identical-p0-floodwas labelled #163, but it needs both the #163 fix and the Rustflood()port. Pattern order can't fix this: all-match labelling over-attributes instead (period-clip-p1-floodmatches*-flood, but the #162 fix alone flips it).What changes
known_failures.txtnow records the causes per case:<case id> KEY,KEY(all needed) orKEY | KEY,KEY(alternatives), with keys fromISSUESinknown_issues.py. The xfail reason lists those issues.attribute.pytakes the failing-case lists from runs with no fixes, each fix alone, all fixes, and all but one, and works out which fixes flip which cases. The README explains how to rerun it.attribute.pyrefuses to write on missing/incomplete result files or cases the runs can't attribute, keeps|alternatives when pruning, and has unit tests (test_attribute.py).--update-known-failureskeeps the keys of cases that still fail. New failures get keys suggested by the id patterns (every match counts) and are printed for confirmation. Unmatched ones are printed as new divergences, as before.How the list was measured
23 runs, plus 11 combination runs, against aw-core 33c8528 and aw-server-rust c68d32c (which fails on exactly the same 929 cases as the pinned submodules), covering the 10 open fix PRs: aw-core#161, #165, #166, #167, #168, #169 and aw-server-rust#744, #748, #749, #750. No run introduced a failure outside the 929.
CORE_163 | CORE_161,RUST_744)Verified: the suite is still green (2087 passed, 929 xfailed).
--update-known-failuresrewrites the file byte-for-byte. With aw-core#167 alone, exactly the 41CORE_164cases XPASS, each with the #164 reason.Side note from the same runs: the two Python-side divergences that were filed under #163 (Python
floodcollapsing identical events,filter_period_intersectzero-duration pieces onsame-start) are covered by existing PRs. The first is covered by aw-core#169 plus aw-server-rust#748, and the second by aw-core#166 plus aw-server-rust#749.Also:
filter_period_intersectboundary scenariosFrom the review of ActivityWatch/aw-core#166, three scenarios: a zero-duration filter at the end of another filter
(0,6),(6,0), one at the end of the event(0,10),(10,0), and adjacent filters(0,6),(6,4). With aw-server-rust#749 (now on Rust master), both servers give the same results:[(0,6),(6,0)],[(0,10)]and[(0,6),(6,4)]. That holds with and without aw-core#166. The pinned Rust (pre-#749) drops the zero-duration piece, so those cases are listed asRUST_747. The 36 new known failures are attributed from the same measured runs. Suite: 2256 passed, 965 xfailed.