This partially reverts the changes from commit 020166080cf57e6eb869abc56d603eb1d6e5e45b to testnet4_miner_tests.cpp
Also, remove some confusing IWYU pragmas. Those were inconsistently added in 8c58f63578a44c8b4ebdda4637d1b198d8a67d08, but without any rationale why adding them is the correct approach. The correct approach should be done in a proper follow-up, with a clear rationale.
This partially reverts the changes from commit
020166080cf57e6eb869abc56d603eb1d6e5e45b to testnet4_miner_tests.cpp
Also, remove some confusing IWYU pragmas. Those were inconsistently
added in 8c58f63578a44c8b4ebdda4637d1b198d8a67d08, but without any
rationale why adding them is the correct approach. The correct approach
should be done in a proper follow-up, with a clear rationale.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35391.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Concept ACK
ACK fad4f417d1590aadbff15ba4557b78fb716771c7
#33966 was not supposed to "fix" boost related IWYU issues, so reverting those makes sense.
Dropping the pragmas in unenforced files is fine too. The original "rationale" is that they were required in my particular IWYU environment at some point in the history of that PR. And conversely IWYU did not complain about them later (I ran it again on every rebase). But CI is different anyway.
Am curious @maflcko if you noticed these issues just reviewing the merged PR or in some other way?
Both of these cases (uses of pragma to silence IWYU, and use of static_cast work around an include bug) error seem like cases where probably a coding agent was given some task, and it added suboptimal code to get the task done that a person would have followed up on.
Workarounds like these seem seem like something authors and reviewers should be specially looking out for in llm generated code. Other ways to prevent this problem might be to prompt differently to add more code comments and make workarounds more obvious. Or to have review tools that specifically look for suspicious things like new static casts or pragmas and check if they are necessary.
Am curious @maflcko if you noticed these issues just reviewing the merged PR or in some other way?
Just manual review.
Both of these cases (uses of pragma to silence IWYU, and use of static_cast work around an include bug) error seem like cases where probably a coding agent was given some task, and it added suboptimal code to get the task done that a person would have followed up on.
Yeah, I think that is the point that blindly using llm tools without understanding every single thing they did is problematic. As soon as an llm bot sees a (build) failure, they will immediately try to fix it in a plausible way.
The issues here are only in tests, and pretty harmless, so I don't think a linter or review tool make sense.
My recommendation would be to only use llm agents when the author first reviews their output before asking others to review it.
Some reconstruction:
There was a review nit #33966 (review):
Nit (iwyu): Can you correct the includes for this new module?
In retrospect, it makes sense to require new files to be enforced by IWYU from the get go. But I overlooked that the comment was just about this new file, and instead interpreted it as applying all unenforced IWYU suggestions. That opened a can of worms in a PR that was already quite large. See #33966 (comment)
I did three pushes after that:
The Boost exception was in place from the get go. And this didn't mess with the test.
That snuck in during a later regular rebase:
This was a rebase after #34858, which churned a lot of the same tests.
I often let the LLM take a stab at a rebase and then check the result with git range-diff to ensure it didn't hallucinate changes. If the diff had been smaller, e.g. if I had dropped the IWYU changes, I likely would have noticed it (and deleted the change, since I don't understand it nor asked for it).
I don't think the problem here was the LLM, it's the size of the diff and churn across rebases and feedback. If I had done the rebase fully manually I probably would have made mistakes too.
uses of pragma to silence IWYU
These pragmas probably snuck in because one run (or environment) demanded the include and another run (or environment) wanted it gone. Had I noticed it, it would have been yet another sign on the wall that expanding IWYU coverage in this PR was a bad idea.
I think we should have dedicated pull requests to expand IWYU coverage. It's a deceptively simple tool, but investigating even a single issue can take hours, see e.g. the multiple attempts to please IWYU for #35101.
I think we should have dedicated pull requests to expand IWYU coverage.
Agree