Repository navigation
CLI-422 Drop doc claims the code already states - #815
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
✅ Deploy Preview for sonarqube-cli canceled.
|
…l site The cut narrowed "Only for credential-free requests that need to follow a CDN redirect" to a rule that no longer covers all three of fetchAnonymous's actual callers. Verified: downloadBinary (sonarsource-releases.ts) and check.ts's stable.version check both target binaries.sonarsource.com (genuinely CDN), but fetchServerVersion (server-info.ts:63) calls fetchAnonymous against <serverURL>/api/system/status — a public unauthenticated SonarQube endpoint, not a CDN. The retained sentence was therefore actively wrong, not just less detailed. Broadened the criterion to name both shapes without re-listing all three files. Gitar's second finding on this PR (that dropping the "(version check and the post-command update notice)" gloss on notification.ts removed the only record of that timing fact) is not applied: notification.ts's own class docblock already says "the post-command 'new version available' stderr notice" almost verbatim, so the cut correctly followed this stack's own established criterion (don't duplicate what the source's header comment already states) — the same criterion #815 used throughout. Gitar's premise that this fact existed nowhere else in the repo's docs is factually incorrect; replied on the review thread with the source excerpt.
e90d8b8 to
a08248a
Compare
8 background agents cross-checked the 8 nested CLAUDE.md/skill files for the "file X does A, file Y does B" pattern the official Claude Code best-practices guide calls out separately from pure code duplication: a list of files whose purpose is fully obvious from their own names, adding nothing beyond `ls` + reading names. Only 2 of 8 files had a genuine hit (most of this content turned out to already carry a non-obvious wiring fact, invariant, or ordering requirement attached, which the agents were told to preserve): - src/core/update/CLAUDE.md: dropped the "(version check and the post-command update notice)" gloss on check.ts/notification.ts. - sonarqube-api-layer skill: dropped the three-file enumeration (downloadBinary/sonarsource-releases.ts, the stable.version check, fetchServerVersion) illustrating fetchAnonymous's CDN-redirect use case — each is exactly what its name says. One flagged candidate (state-and-auth) is NOT applied here: an agent proposed re-cutting "distinct from installed-agent-detector.ts" from the agent-detector.ts bullet, on the grounds that installed-agent-detector.ts's own header already explains the distinction. That's true once you're in that file, but the clause being cut is the *only* pointer to that file's existence at all in this skill — cutting it would recreate the exact gap #815's Gitar review caught and fixed for this same line. Disagreeing with that finding.
…l site The cut narrowed "Only for credential-free requests that need to follow a CDN redirect" to a rule that no longer covers all three of fetchAnonymous's actual callers. Verified: downloadBinary (sonarsource-releases.ts) and check.ts's stable.version check both target binaries.sonarsource.com (genuinely CDN), but fetchServerVersion (server-info.ts:63) calls fetchAnonymous against <serverURL>/api/system/status — a public unauthenticated SonarQube endpoint, not a CDN. The retained sentence was therefore actively wrong, not just less detailed. Broadened the criterion to name both shapes without re-listing all three files. Gitar's second finding on this PR (that dropping the "(version check and the post-command update notice)" gloss on notification.ts removed the only record of that timing fact) is not applied: notification.ts's own class docblock already says "the post-command 'new version available' stderr notice" almost verbatim, so the cut correctly followed this stack's own established criterion (don't duplicate what the source's header comment already states) — the same criterion #815 used throughout. Gitar's premise that this fact existed nowhere else in the repo's docs is factually incorrect; replied on the review thread with the source excerpt.
a08248a to
bdcc3f5
Compare
charafsalmi
changed the title
bdcc3f5 to
765af5c
Compare
765af5c to
723b3e0
Compare
Some paragraphs restated what a JSDoc comment, a type union or a short function next to them already said. That is two places to keep in sync, and the copy in the docs is the one that goes stale without anyone noticing. Each removed claim was checked against the code that supersedes it: the dispatch semantics are in `runClaudePostToolUseDispatch`'s JSDoc, the alpha/beta conditions in `isAlphaEligible`/`isBetaEligible`, the caching rules in `resolveProjectUuid`'s header, the logout early-return in `authLogout`. Where the code says it better, the doc now points at it instead of repeating it. Two enumerations go for the same reason: the list of API clients and their count, which has to be edited on every client added, and the list of files under `framework/features/`, which an `ls` answers.
723b3e0 to
e51a16c
Compare
|
Code Review ✅ Approved 3 closed / 3 findingsRemoves 27 clauses across 7 files that verbatim restate JSDoc comments or header documentation already present in the referenced source code, reducing duplication-maintenance costs. A second commit restored three cuts that removed non-derivable guidance (state-and-auth staging rules, enumeration context, Cursor/Codex handler file names), keeping only the genuinely redundant material. ✅ 3 closed✅ Quality: Two state-and-auth cuts remove guidance absent from the source
✅ Quality: "all seventeen of them" now refers to a removed enumeration
✅ Quality: Cursor/Codex prompt handler files no longer named, stranding text
VerificationFunctional validation No results Rules No rules evaluated Auto-approval Not enabledOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
Stacked on #814.
Some paragraphs restated what a JSDoc comment, a type union or a short function next to them already said. That is two places to keep in sync, and the copy in the docs is the one that goes stale without anyone noticing.
The diff is 27 deleted lines across 7 files, so it is readable in full. The table is a shortcut to the ones worth a second look, not a substitute for it. Every pointer below was checked against the code on this branch.
isAlphaEligible/isBetaEligibleJSDocSonarOption.stage()JSDoc, almost word for wordTelemetryFacttimestamp default and payload opacityPostToolUsedispatch and short-circuit semanticsrunClaudePostToolUseDispatchJSDoc +ClaudePostToolUseResultresolveProjectUuidcaching rules and edge casessonar auth logoutearly returnauthLogoutbodysonarcloud-region.tsimport-cycle rationaledetectInstalledAgentsglosssrc/core/server/, and their countlsframework/features/lsThe rest of the diff is shortening rather than removal: a few passages that explained a mechanism in prose now name the function and stop there, so the fact survives and the restatement goes.
Already settled: the two enumerations go because they have to be edited on every client or file added. #795 had to bump "seventeen" to "eighteen" when
EnterprisesClientlanded, and this branch was still carrying the stale "seventeen" until today's resync — the failure mode, not a hypothetical one.