Reduce LLM code generation noise

[TonyCTHsu]

What does this PR do?

Prevent agent from writing code comment

Motivation:

Too much noise.

Change log entry
None.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e9d21bc9d1

ℹ️ 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".

[datadog-datadog-prod-us1]

 

⚠️ Warnings

🚦 2 Pipeline jobs failed

Unit Tests | Ruby 3.4 / build & test (standard) [0] — 🔧 Needs a code fix, caused by this PR

View more details · View in GitHub Actions

1 failed test for Datadog integration graceful shutdown for file descriptors. Open file descriptors not closed properly, resulting in assertion failure.

Check Pull Request CI Status | all-jobs-are-green

View more details · View in GitHub Actions

📋 Copy fix prompt

CI on my pull request is failing. Help me find and fix the root cause of each failing job below — they were flagged as caused by changes in this PR, so focus on the diff. For each job, explain the failure and propose a fix.

Before you start, set up the Datadog software-delivery tooling so you can
query the CI data yourself:

1. Check whether you already have the Datadog software-delivery MCP tools
   (e.g. a `search_datadog_ci_pipeline_events` tool) and the `unblock-pr` skill.
2. If either is missing, STOP and ask me for permission before installing
   anything. Do not install or run anything until I have said yes.
3. Only with my explicit approval, set up the Datadog software-delivery MCP
   server and skills by following:
     https://docs.datadoghq.com/getting_started/software_delivery_mcp_tools/
   then restart so the skill is picked up.
4. If I decline, skip all of the above and work from the context below alone.

Then run /unblock-pr — it will pull the CI data itself. The job context below is what we already know.

If /unblock-pr is not available — because I declined the setup above, or it did not install — work from the context below instead.

Datadog has already classified this failure as caused by changes in this PR.
Take that as given and work the fix:

1. Locate the change. Diff this branch against its base and find the change
   that produces this error. Explain the mechanism, don't just name a file:
     git fetch origin && git diff $(git merge-base origin/master HEAD)...HEAD
2. Reproduce it locally. Run the failing job's command or test before
   proposing anything.
3. Propose the smallest fix that addresses the root cause — not a workaround,
   not a broadened assertion, not a disabled or skipped test.
4. Re-run the same command to confirm, and say exactly what you ran.
5. If the failure turns out to be intermittent rather than deterministic, say
   so plainly instead of "fixing" it — that is a flaky test, and patching it
   hides the problem.

If the right move is to re-run the job rather than change code, use the job
link in the context below. For GitHub Actions: `gh run rerun <run-id> --failed`,
where the run ID is the number after `/runs/` in that URL (not the trailing
number, which is the job ID).

Branch: tonycthsu/no-comment

Unit Tests | Ruby 3.4 / build & test (standard) [0]
Commit: 4e505011b5dbe928aa509e87f141fe15cd9f5f8f
Error (code / test):
1 failed test for Datadog integration graceful shutdown for file descriptors. Open file descriptors not closed properly, resulting in assertion failure.
CI job: https://github.com/DataDog/dd-trace-rb/actions/runs/32823971257/job/97728115455

ℹ️ Info

No other issues found (see more)

🧪 All tests passed
❄️ No new flaky tests detected

Useful? React with 👍 / 👎

_{This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 4e50501 | Docs | View more details | Give us feedback!}

[eregon]

If this prevents all comments added by the LLM, that's too extreme.
For example most comments in these PRs were written by LLM and many of them are useful:

• https://github.com/DataDog/dd-trace-rb/pull/5777/changes#diff-887d03c7414c8248b1a4c6942077a4da1cf5b707ccde30f91f47d0a7239f430aR217 the comment on the gvl_state_change_count field and following fields in ext/datadog_profiling_native_extension/collectors_thread_context.c.
• https://github.com/DataDog/dd-trace-rb/pull/5955/changes#diff-887d03c7414c8248b1a4c6942077a4da1cf5b707ccde30f91f47d0a7239f430a many comments in ext/datadog_profiling_native_extension/collectors_thread_context.c, I'd say most are useful and many explain non-trivial things which one wouldn't be able to infer from the code.

So I think it's unfortunately not that simple.
What we want to avoid is comments that are just describing literally what the code does and so are redundant, or comments which explain something which has already been explained somewhere else.
In general I think comments on fields/constants are quite valuable, comments on functions/methods it depends a lot more, and comments inside functions/methods I would say most of the time useful.

It's up to the creator of the PR to self-review and remove/trim redundant comments, and also clarify comments as the LLM can be quite unclear at times.

Maybe we can instruct it to avoid adding comments for things that are obvious from the code or are already explained somewhere else (in that latter case, possibly "link" to that explanation)?
Maybe this can be summarized as:

• Avoid writing code comments that are obvious from reading the code.
• Avoid writing code comments which duplicate another code comment, link instead if important for understanding, otherwise omit.

[eregon]

Marking it as request changes because I don't think we can merge this as-is.

[TonyCTHsu]

That's fair pushback, but I think the rule already accounts for this. "Unless explicitly requested" covers exactly the cases you're pointing to: a PR author who identifies a struct field, constant, or non-obvious invariant worth documenting can ask for that comment directly, and the agent will write it. "Or instructed" covers cases like the ::Time.now load-time exception already documented elsewhere in this file, or other explicit conventions.

What the rule removes is the LLM's own unprompted judgment about what's "worth explaining." In practice that judgment has been a source of noise, not signal: reviewers end up spending review time triaging and asking for cleanup of comments nobody asked for, many of which just restate the code. Shifting to "ask for the comment you actually want" puts a human in the loop on the one part of this that's genuinely subjective, instead of relying on the LLM to guess well every time.

If it turns out this is still trimming too much in practice (e.g. agents interpreting "instructed" too narrowly and not writing the field-level comments you got good value from before), that's a signal to loosen the wording — but I'd rather tighten first and see if review load actually drops, since the status quo before this rule was the noisy one.

[eregon]

"Unless explicitly requested" covers exactly the cases you're pointing to: a PR author who identifies a struct field, constant, or non-obvious invariant worth documenting can ask for that comment directly, and the agent will write it.

But I did not request the LLM to comment this, it's just part of writing code to document/comment the tricky parts.
So with this PR it would stop doing that (IIUC), and the code would be much harder to understand; Or I would need to explicitly request it to add comments which is very easy to forget and not workable.

[bengl]

What the rule removes is the LLM's own unprompted judgment about what's "worth explaining." In practice that judgment has been a source of noise, not signal: reviewers end up spending review time triaging and asking for cleanup of comments nobody asked for, many of which just restate the code.

This is indeed a real problem, but the solution here is way too heavy-handed. It's worth instead instructing the LLMs to be terse and to-the-point, only adding comments where it deems the code to be non-obvious, or where callouts to URLs/docs are necessary.

Otherwise, you're adding unnecessary friction to creating well-commented code.

[andrewlock]

OMG yes please

[ddyurchenko]

Hey folks 👋

On one hand, my take is pretty much the same as Bryan's and Benoit's. Disabling code comments completely is likely to be harmful for more complex code.

On the other hand, I do agree that sometimes Claude is too verbose and duplicate the code in the comment. Sometimes it also generates tests that consist of 99% of mocks, testing nothing.

The rule says
"- Write code comment, unless explicitly requested or instructed"

I think this is too wide and prevents AI from providing helpful comments (the unknown unknowns will be missed by a reviewer). The downside of having AI comments as of today is that they require extra maintenance and can cause confusion when they are badly written.

What I would love us generally is to come up with a common core of best practices on code comments. And then we can supply these to AI.

Some references for best practices that could be used as a starter:

• https://mitcommlab.mit.edu/broad/commkit/coding-and-comment-style/
• https://stackoverflow.blog/2021/12/23/best-practices-for-writing-code-comments/

[ddyurchenko]

.

[TonyCTHsu]

I want to push back on the framing that this is a close call between two reasonable tradeoffs.

The comment noise is a real, ongoing cost — it's happening on PRs right now, to the reviewers who have to read them. The "good comments" being cited against it are a handful of retrospective examples pulled from specific PRs. That's not evidence of a reliable rate of value; it's survivorship bias. It's easy to point at two or three fields in collectors_thread_context.c that turned out well and hold them up as proof the current approach works, but that ignores every PR where the same unprompted-commenting judgment produced noise instead. I think the value of those good examples is being overstated relative to the cost that's actually being paid at volume.

There's also an assumption underneath "it's up to the creator of the PR to self-review and remove/trim redundant comments" — that this self-review reliably happens. In practice it doesn't. People don't go back and prune or tighten LLM-generated comments before asking for review; that's exactly how the noise ends up in front of reviewers in the first place. So the safety net this objection is leaning on isn't actually there today, which is part of why I don't think "just tell the LLM to be terse and use good judgment" fixes anything — that's roughly the standard already in place, and it's what produced the status quo this PR is responding to.

I'd rather ship the strict version and revisit if we see clear, recurring evidence of real documentation being lost, than keep relying on a self-review step that isn't happening and a "use good judgment" standard that already isn't working.