investigate-ci: address two more PR review comments

- Gate the flaky verdict on true master rows (pull_request_number = 0) only.
  base_ref IN ('master','') matches every PR targeting master, so an unrelated
  PR's failure could mask the investigated PR's own regression as FLAKY. Move
  the cross-PR check to an explicit secondary corroboration query.
- Stop auto-allowing the repo-local node/python3 helper scripts in the default
  profile; a PR could edit .claude/tools/* and have it run unprompted. Keep them
  prompt-gated. Document the same risk in the investigate profile and require
  running it against a trusted checkout in a no-credential throwaway env.

https://github.com/ClickHouse/ClickHouse/pull/107085

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This commit is contained in:
George Larionov
2026-06-10 23:02:17 +00:00
co-authored by Claude Opus 4.8
parent ddeb7ae44e
commit 9f88c021c0
3 changed files with 22 additions and 21 deletions
+1 -1
View File
@@ -1,5 +1,5 @@
{
"//": "Locked-down profile for CI-failure investigation. Launch with: claude --settings .claude/settings.investigate.json. 'deny' wins over 'allow', so destructive actions are blocked even if some other settings file allows them. Read-only investigation commands run without prompts. For defense in depth, run this inside a throwaway git worktree and/or an environment with no push credentials.",
"//": "Locked-down profile for CI-failure investigation. Launch with: claude --settings .claude/settings.investigate.json. 'deny' wins over 'allow', so destructive actions are blocked even if some other settings file allows them. Read-only investigation commands run without prompts. WARNING: the node/python3 helper allows below execute repo-local scripts (.claude/tools/*) from the current checkout, and the Bash deny/allow list does NOT constrain child processes those scripts spawn — a PR under investigation could have modified them. Because of this, running this profile inside a throwaway git worktree on a trusted base-branch checkout of .claude/tools AND an environment with no credentials is REQUIRED when investigating untrusted PRs, not merely defense in depth.",
"permissions": {
"deny": [
"Edit",
-3
View File
@@ -23,9 +23,6 @@
"Bash(grep:*)",
"Bash(wc:*)",
"Bash(jq:*)",
"Bash(node .claude/tools/fetch_ci_report.js:*)",
"Bash(python3 .claude/tools/fetch_perf_report.py:*)",
"Bash(python3 .claude/tools/analyze-assembly.py:*)",
"Bash(curl -sS https://play.clickhouse.com/?user=play:*)",
"Bash(which:*)",
"Bash(strings:*)",
+21 -17
View File
@@ -146,24 +146,28 @@ SELECT test_name,
FROM checks
WHERE check_start_time >= now() - INTERVAL 90 DAY
AND test_name IN ( '<test1>', '<test2>', ... )
AND (pull_request_number = 0 OR base_ref IN ('master',''))
AND pull_request_number != <investigated PR>
AND pull_request_number = 0
GROUP BY test_name
ORDER BY fail_7d DESC
FORMAT TabSeparatedWithNames
"
```
The `pull_request_number = 0 OR base_ref IN ('master','')` filter restricts to master commits
and PRs targeting master, dropping fork-PR noise.
**Gate on true master rows only — `pull_request_number = 0`.** Do *not* widen to
`base_ref IN ('master','')`: that matches every PR targeting master (hundreds of nonzero PRs),
so a failure in any *unrelated* PR — including a real regression that PR introduced — would make
`fail_<window>d >= 1` here and send the investigated PR's own regression down the `FLAKY` path,
skipping root-cause analysis. Direct `master` commits are the only rows that show the test is
flaky independent of any PR.
**Exclude the PR you are investigating** (`AND pull_request_number != <PR>`, the number from
step 1). Its own failing checks carry `base_ref='master'`, so without this clause they satisfy
the filter above and inflate `fail_<window>d` — a real regression that fails *only* in this PR
would then read as `fail_<window>d >= 1` and be misclassified `FLAKY`, skipping root-cause
analysis. The remaining rows (true master + *other* PRs) are exactly the independent signal the
flaky verdict needs. When investigating master directly or an issue with no associated PR, there
is no PR to exclude — drop the clause.
**Cross-PR failures are a separate, secondary signal.** If master rows are sparse, you may widen
to other PRs as *corroboration* — never as the gate — and only after comparing failure modes (a
flaky repeats the *same* error across unrelated PRs; distinct errors mean distinct bugs). Exclude
the investigated PR so its own failures never feed back in:
```sql
AND base_ref IN ('master','') AND pull_request_number NOT IN (0, <investigated PR>)
```
**Never `GROUP BY` over or filter with `LIKE` on `test_context_raw`** — that column holds the
full test output and a 90-day scan over it times out (60 s limit). The aggregate query above is
@@ -179,17 +183,17 @@ FROM checks
WHERE check_start_time >= now() - INTERVAL 90 DAY
AND test_name = '<test>'
AND test_status IN ('FAIL','ERROR')
AND (pull_request_number = 0 OR base_ref IN ('master',''))
AND pull_request_number != <investigated PR>
AND pull_request_number = 0
ORDER BY day DESC
FORMAT TabSeparatedWithNames
"
```
Use this to confirm the master failures share the **same failure mode** as the report (a real
regression repeats the same error; a flaky test often fails identically across *unrelated* PRs)
and to see which `check_name` configs are affected (e.g. only the heavily-loaded
`arm_binary, parallel` shard points to a load/timing race).
Use this to confirm the master failures share the **same failure mode** as the report, and to
see which `check_name` configs are affected (e.g. only the heavily-loaded `arm_binary, parallel`
shard points to a load/timing race). In the optional cross-PR corroboration query, the same-error
test is the giveaway: a flaky fails identically across *unrelated* PRs, whereas distinct errors
mean distinct bugs.
**Classify each failed test:**