PMM-7 Make AGENTS.md guidance reach Claude Code - #5885
Conversation
Nested inside logrus's own quoted field value, %q double-escapes and produces output like: error="environment variable name \"MONGODB_URI\" is reserved" '%s' delimits the value just as clearly without the escaping, and %s alone is enough when the value cannot contain spaces. This documents the convention for new and changed code; no sweep of the ~190 existing %q call sites is intended. Signed-off-by: Alex Demidoff <a@demidoff.me>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5885 +/- ##
==========================================
+ Coverage 43.59% 44.05% +0.45%
==========================================
Files 415 434 +19
Lines 43134 35474 -7660
Branches 0 581 +581
==========================================
- Hits 18804 15627 -3177
+ Misses 22454 18264 -4190
+ Partials 1876 1583 -293 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. WalkthroughUpdated global and component Merge Risk: ⚪ Minimal · up to This change updates repository guidance and Claude Code pointer files without modifying application behavior. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description includes the ticket number, feature-build status, API documentation applicability, detailed change scope, rationale, and verification notes. It matches the repository template and explains that no API change occurred. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Claude Code reads CLAUDE.md, not AGENTS.md. Only the repo root had a pointer, and it imports the root AGENTS.md alone -- the component guides are merely linked from a table, so they reached Claude only if it chose to open them. Nothing merged them into context automatically. Claude Code does load a nested CLAUDE.md on demand, the first time it reads a file in that directory, and an @import there resolves relative to the importing file. So a one-line CLAUDE.md beside each component AGENTS.md makes that guide load automatically for whichever component is being worked on, without the root file pulling in all eleven. Each pointer contains the import and nothing else; the maintainer note is an HTML comment, which is stripped before the file enters context. Signed-off-by: Alex Demidoff <a@demidoff.me>
ademidoff
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: ce3bab62-8a66-4243-b559-2ea92d0f1466
📒 Files selected for processing (12)
AGENTS.mdadmin/CLAUDE.mdagent/CLAUDE.mdapi-tests/CLAUDE.mdapi/CLAUDE.mdbuild/CLAUDE.mddashboards/dashboards/CLAUDE.mddashboards/pmm-app/CLAUDE.mdmanaged/CLAUDE.mdqan-api2/CLAUDE.mdui/CLAUDE.mdvmproxy/CLAUDE.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
Now that a component AGENTS.md loads alongside the root one, a rule restated in both costs context twice and gives it two places to drift. Applies one rule: a component guide keeps only what is specific to its area, and anything that merely restates the root's Global Development Conventions is removed. The root's "PMM-specific choices" list is left alone -- it repeats a handful of rules deliberately, as a curated set of pitfalls, and says so. managed/AGENTS.md carried the bulk of it: twelve of its thirteen "Do" bullets and seven of its ten "Don't" bullets restated the root, as did most of Testing and all of Code Generation. api/AGENTS.md and api-tests/AGENTS.md had four between them. The remaining seven guides were already free of it and are untouched. Three rules existed only in managed/AGENTS.md despite being general Go style, so they move up to the root instead of being deleted: - //nolint as the sole exception to the no-inline-comments rule - don't inline err != nil checks - existing github.com/pkg/errors uses may remain until refactored Each removal was checked against the root to confirm the rule still has a home there. Signed-off-by: Alex Demidoff <a@demidoff.me>
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 17e19303-38ba-4e1e-b4db-b765b9ca7e2a
📒 Files selected for processing (4)
AGENTS.mdapi-tests/AGENTS.mdapi/AGENTS.mdmanaged/AGENTS.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
💤 Files with no reviewable changes (1)
- api-tests/AGENTS.md
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.
The maintainer comment said the same thing eleven times, and the root AGENTS.md already explains the mechanism in one place. Drop it; each pointer is now the single import line it describes. Signed-off-by: Alex Demidoff <a@demidoff.me>
It sat in the root's Error Handling list scoped with "in pmm-managed", which is the tell that it belongs a level down. managed/AGENTS.md already stated it, in the reform Key conventions list beside the other reform rules, so this is a deletion from the root rather than a move. Nothing is lost: the component guide now loads whenever Claude reads a file under managed/, which is the only place the rule applies. Every remaining bullet in the root's Error Handling list is genuinely global. Signed-off-by: Alex Demidoff <a@demidoff.me>
Same tell as reform.ErrNoRows: the root scoped it with "(pmm-managed only)". managed/AGENTS.md already showed the directive in its reform example, so the bullet there is promoted from "models carry //go:generate directives" to naming the command and what regenerates the *_reform.go files. reform is still introduced at the root, in the Tech Stack table, so the "reform: not used" notes in agent/ and qan-api2/ keep their referent. The generated-file list keeps *_reform.go, which is a repo-wide rule about what not to edit rather than a managed convention. Signed-off-by: Alex Demidoff <a@demidoff.me>
The previous commit moved it into managed/AGENTS.md on the strength of the root guide's own claim that reform is "pmm-managed only". That claim is false: pmm-agent generates reform models too, in agent/agents/mysql/perfschema agent/agents/postgres/pgstatstatements agent/agents/postgres/pgstatmonitor each with a //go:generate go tool reform directive and a checked-in models_reform.go, and it uses them at runtime -- reform.NewDB in perfschema.go, SelectRows and SelectAllFrom in the collectors. Two components generate reform models, so the rule is global and belongs in the root. Restored there without the false scope, and describing both users: managed maps PMM's own PostgreSQL, agent maps the monitored database's system views. managed/AGENTS.md keeps its bullet, trimmed so it names where the models live rather than repeating the directive. Signed-off-by: Alex Demidoff <a@demidoff.me>
The claim appears in three places and is false in all of them. pmm-agent's QAN collectors use reform to map monitored-database system views: reform.ErrNoRows in perfschema/history.go, perfschema/summaries.go, pgstatstatements.go and stat_monitor_cache.go, and reform.NewDB in perfschema.go. 24 files under agent/ import reform.v1. - the Tech Stack row said "used in pmm-managed only" - agent/AGENTS.md said reform was "not used (agent has no DB)" - agent/AGENTS.md said "Don't use raw SQL -- the agent has no database; all data comes via gRPC", which is two claims: the agent having no datastore of its own (true) and its data all arriving by gRPC (false -- QAN data is read out of the monitored database) An earlier commit in this branch trusted the first of these and pushed the reform.ErrNoRows rule down to managed/AGENTS.md. It belongs in the root, scoped to both users, and is restored there; managed's copy in the reform key-conventions list goes, since the root now states it. qan-api2's "no reform" note is correct and left alone -- its ErrNoRows uses are sql.ErrNoRows. Signed-off-by: Alex Demidoff <a@demidoff.me>
reform is used by two components out of eleven, so a root-level rule makes every other component's reader carry it. Stated instead in managed/AGENTS.md, in the reform key-conventions list, and in agent/AGENTS.md, where it names the QAN collectors that read monitored-database system views. The layering note now says a rule belongs in the most specific guide or guides that cover it, since a rule can apply to two components without being global. Signed-off-by: Alex Demidoff <a@demidoff.me>
Same placement as reform.ErrNoRows: managed/AGENTS.md and agent/AGENTS.md both already described the directive, so the root bullet was the redundant third copy. managed/AGENTS.md names the full command again. It had been trimmed to "a reform //go:generate directive" only because the root spelled it out; with the root bullet gone that reason is gone too. The root keeps what is genuinely repo-wide: *_reform.go in the never-edit list, and the Tech Stack row, which introduces reform for the agent/ and qan-api2/ guides that refer to it. Signed-off-by: Alex Demidoff <a@demidoff.me>
Nobody runs //go:generate by hand -- make gen does it, via go generate ./... in managed/Makefile. That is exactly why the directive is worth documenting: go generate acts only on files that contain one, and gen depends on clean, which deletes every *_reform.go first. So a new model file missing the line gets no *_reform.go while the other sixteen regenerate normally, with no error. The bullet now says that instead of describing the directives already present, and drops the make gen half, which the root guide's workflow and Definition of Done already cover. agent/AGENTS.md stays descriptive; reform models are not added there. Signed-off-by: Alex Demidoff <a@demidoff.me>
api/ uses get 46 times, post 44, delete 10, put 8, and patch never. The rule records that rather than introducing it. Worth stating because the tempting cases already exist: ChangeAgent, ChangeService and ChangeSettings all send just the fields being changed, which is the shape a contributor would reach for PATCH to express. They are PUT, so the Do bullet now says PUT covers partial updates too. Nothing enforces it -- buf lint's STANDARD rules check proto identifiers, not the method keys inside google.api.http -- so the bullet says that, matching how the REST Path Naming section flags its own lack of enforcement. Signed-off-by: Alex Demidoff <a@demidoff.me>
|
@coderabbitai Your preamble takes more than once full page. That's super distracting and it clutters the PR, making it difficult to find what really matters. Please configure yourself to prevent this from happening - it's quite annoyoing. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain⚙️ CodeRabbit configuration inspected Length of output: 5812
The repository configuration already disables generated high-level summaries and changed-file summaries. It has no setting to limit one individual comment. Future review comments will omit long preambles and state only the issue and recommended fix. ✏️ Learnings added
You are interacting with an AI system. |
1 participant
Ticket number: PMM-7
Feature build: n/a — documentation and agent-tooling only, no code or user-facing change.
Three related fixes to the
AGENTS.mdsetup, aimed at guidance actually reaching the agent that needs it, once.1. Prefer
'%s'over%qin logs and errorsAdds one bullet under Error Handling and one under Logging in the root
AGENTS.md: don't interpolate strings with%qin log or error messages; use%s, or'%s'when the value can contain spaces.%qnests inside logrus's own quoted field value and double-escapes. A message written asreaches the log as
'%s'delimits the value just as clearly without the escaping, and plain%sis enough when the value can't contain spaces. This came up on #5781, where the pattern is visible in new test output.This documents the convention for new and changed code. There are ~190 existing
%qcall sites in non-test Go; no sweep is proposed here, and nothing is enforced by a linter.2. Per-component
CLAUDE.mdpointersClaude Code reads
CLAUDE.md, notAGENTS.md. Until now only the repo root had a pointer, and it imports the rootAGENTS.mdalone. The component guides are linked from a table in the root document, so they reached Claude only if it chose to open them — nothing merged them into context.That is the reason a rule like the one above could be added to the root file and still be missed:
managed/AGENTS.mdcarried its own self-contained Do/Don't list restating the error-handling and logging conventions, so an agent reading it saw a complete-looking set of rules with no%qentry.Claude Code does load a nested
CLAUDE.mdon demand, the first time it reads a file in that directory, and a relative@importresolves against the importing file rather than the working directory (docs). So this adds aCLAUDE.mdbeside each of the 11 componentAGENTS.mdfiles, containing the import and nothing else:@AGENTS.mdEach pointer is that single line and nothing else. The mechanism is explained once, in the root
AGENTS.md, rather than repeated in all eleven files.Verified locally: with
managed/CLAUDE.mdin place, readingmanaged/models/agent_model.goloadsmanaged/AGENTS.mdinto context automatically; without it, the guide never loads.3. Remove conventions duplicated between root and component guides
Once a component guide loads alongside the root one, a rule restated in both costs context twice and gains a second place to drift out of sync. One rule applied throughout:
managed/AGENTS.mdcarried the bulk of it — twelve of thirteen "Do" bullets and seven of ten "Don't" bullets restated the root, as did most of Testing and all of Code Generation.api/AGENTS.mdandapi-tests/AGENTS.mdhad four between them. The other seven guides were already clean and are untouched.Three rules lived only in
managed/AGENTS.mddespite being general Go style, so they move up to the root rather than being deleted://nolintas the sole exception to the no-inline-comments ruleerr != nilchecksgithub.com/pkg/errorsuses may remain until refactoredEvery removal was checked against the root to confirm the rule still has a home there. Net: −42/+11 lines across the guides, with
managed/AGENTS.mddown from 197 to 165.The root's PMM-specific choices list is deliberately left alone — it repeats a handful of rules on purpose as a curated pitfall list, and now says so. The root also gains a short note stating where a new rule belongs, so this doesn't grow back.
4. Correct the "reform is pmm-managed only" claim
While applying (3) it turned out the repo states this in three places and it is false in all of them. pmm-agent's QAN collectors use reform to map monitored-database system views — 24 files under
agent/importreform.v1, three carry//go:generate go tool reformwith checked-inmodels_reform.go, andreform.ErrNoRowsappears inagent/agents/mysql/perfschema/history.go:73,agent/agents/mysql/perfschema/summaries.go:78,agent/agents/postgres/pgstatstatements/pgstatstatements.go:274andagent/agents/postgres/pgstatmonitor/stat_monitor_cache.go:129.Corrected:
agent/AGENTS.mdno longer says reform is "not used (agent has no DB)"agent/AGENTS.mdno longer says "Don't use raw SQL — the agent has no database; all data comes via gRPC". pmm-agent having no datastore of its own is true; its data all arriving over gRPC is not, since QAN data is read out of the monitored database.The real distinction, now stated: pmm-managed uses reform for PMM's own PostgreSQL; pmm-agent uses it as a row mapper against the monitored database's system views.
qan-api2/AGENTS.md's "no reform" note is correct and untouched — itsErrNoRowsuses aresql.ErrNoRows.