Repository navigation
Darling README: move six sections to task pages (part of #6416, phase 1) - #6722
Conversation
Darling/README.md was too large to render on GitHub. This commit moves six sections that no open PR edits into the docs/darling/ pages and leaves a stub heading with the same anchor for each moved heading that a link points at. - Quick Start reference: docs/darling/install.md, docs/darling-quick-start.md (Linux), docs/darling/admin.md (upgrade and rollback) and the new docs/darling/monitored-servers.md. - PostgreSQL Targets: new docs/darling/postgresql-targets.md. - Connect a Remote Viewer: docs/darling/viewer.md. - How It Runs (Reference): docs/darling/admin.md. - Managed Bundled PostgreSQL: new docs/darling/managed-postgresql.md. - Per-server threshold overrides: docs/alerts.md. Docs links to the moved anchors now point at the new pages. Five existing test pins read moved text, so they now read the page that holds it.
# Conflicts: # Darling/README.md
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: LGTM
Claims
- VERIFIED — "Every moved heading that a link points at keeps a stub heading in the README, with the same text" — confirmed via heading grep:
Darling/README.mdstill has## PostgreSQL Targets(:997),## Connect a Remote Viewer(:1389),## How It Runs (Reference)(:1458),## Managed Bundled PostgreSQL(:1464),## Per-server threshold overrides(:1815), plus every sub-heading stub listed in the body (Prerequisites, Configure darling.json, Install as a Windows Service, etc.) — only the bodies were replaced with "moved to" pointers. - VERIFIED — "
docs/darling/install.mdis now about 59 KB" —wc -c docs/darling/install.md= 60,759 bytes (≈59.3 KiB). - VERIFIED — the "Tests" table (5 pins retargeted, assertions unchanged) — diffed all 5 files (
DarlingRuntimePreflightTests.cs,NewerStoreWarningTests.cs,PgLogEventMetricsTests.cs,RetentionReevaluationTests.cs,StoreObjectConvergenceTests.cs); each now reads exactly the page the table names, with the sameAssert.Containsliterals, and those literals are present verbatim in the new pages (e.g. the V130recent_runsline indocs/darling/postgresql-targets.md:77, the store-object-convergence row indocs/darling/admin.md:254). - VERIFIED — "every file I changed is CRLF in the working copy" —
fileon all nine touched/newdocs/darling/*.mdplusDarling/README.mdreports CRLF line terminators throughout. - VERIFIED — "
CHANGELOG.mdanddocs/changelog/… I left their links alone" — neither appears ingh pr diff 6722 --name-only. - VERIFIED (spot check) — "Sub-headings that nothing links to have no stub… I checked that no tracked file links to any of them" — grepped the repo for
#the-sessions,#clones,#what-gets-collected,#if-it-does-not-connect; no inbound references outside the destination pages' own TOC-adjacent prose. - UNVERIFIED — "I built
Darling.TestsandLite.Testswith-p:EnableWindowsTargeting=trueon macOS: 0 errors for each" — requires running a .NET build; not done in this read-only review. - UNVERIFIED — "I ran
scrubcheckon the full diff… 0 hits" — tool/output not available to this review. - UNVERIFIED — "Link check: … 0 broken" (the author's own exhaustive sweep) — I independently re-verified ~80 of the relative links and GitHub-slug anchors by hand across every changed file (all resolved), but can't confirm the claimed automated sweep itself ran clean.
- REFUTED — "The README goes from 2,457 lines and 545,401 bytes … to 1,755 lines and 404,734 bytes" —
wc -l/wc -con the currentDarling/README.mdreport 1,817 lines / 409,707 bytes, 62 lines and ~4.9 KB more than the stated final figures. Purely cosmetic (likely stale from before the final rebase/commit), no functional impact — the directional claim ("about a quarter smaller") still holds.
Riskiest lines
docs/darling/monitored-servers.md:80-97— the relocated grant table that gates SQL Server collection; a silent future edit here would ship unnoticed — UNCOVERED (no behavior test pins this page).Darling/README.md:115(](../docs/darling/monitored-servers.md#permissions-on-monitored-servers)) — the stub link operators actually click post-move; if the target heading's slug ever drifts, nothing in CI fails — UNCOVERED.docs/darling/admin.md:254— the "Store object convergence" cadence row moved fromDarling/README.md— covered byStoreObjectConvergenceTests.TheReadme_NamesTheHourlyConvergenceandRetentionReevaluationTests, both updated in this PR to readadmin.md.
Traps near this diff
.gitattributes says text=auto eol=crlf: … A diff that touches every line of a file is a line-ending conversion until proven otherwise— does not apply: every changed file shows a targeted content diff (913 additions / 777 deletions across 23 files, not whole-file rewrites), andfileconfirms CRLF survived in both old and new docs.Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding— does not apply: this PR only reorganizes Darling-specific operator documentation underdocs/darling/; it touches no Lite doc, code, or test, and Lite has no equivalent oversized-README problem being solved.
Divergence (blind read vs PR body)
One divergence, already captured as REFUTED above: the PR body's stated final line/byte counts for Darling/README.md don't match the tree as checked out (1,817/409,707 vs. the claimed 1,755/404,734). Everything else — the six-section move list, the stub inventory, the 34 inbound-link retargets, the five test-pin retargets, the "sections left alone because open PRs touch them" list — matches the diff exactly; I found no section claimed as moved that wasn't, no stub claimed as kept that was dropped, and no inbound link claimed as fixed that still points at the old location.
Reviewer checklist
- Run the author's claimed link-check script (or an equivalent) and the Windows-targeted test build, since neither could be executed from this read-only review.
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
(Correcting my previous review on this PR, which wrongly posted as LGTM without the required footer — same findings, corrected verdict and footer below.)
Claims
- VERIFIED — "Every moved heading that a link points at keeps a stub heading in the README, with the same text" — confirmed via heading grep:
Darling/README.mdstill has## PostgreSQL Targets(:997),## Connect a Remote Viewer(:1389),## How It Runs (Reference)(:1458),## Managed Bundled PostgreSQL(:1464),## Per-server threshold overrides(:1815), plus every sub-heading stub listed in the body — only the bodies were replaced with "moved to" pointers. - VERIFIED — "
docs/darling/install.mdis now about 59 KB" —wc -c docs/darling/install.md= 60,759 bytes (≈59.3 KiB). - VERIFIED — the "Tests" table (5 pins retargeted, assertions unchanged) — diffed all 5 files (
DarlingRuntimePreflightTests.cs,NewerStoreWarningTests.cs,PgLogEventMetricsTests.cs,RetentionReevaluationTests.cs,StoreObjectConvergenceTests.cs); each now reads exactly the page the table names, with the sameAssert.Containsliterals, and those literals are present verbatim in the new pages (e.g. the V130recent_runsline indocs/darling/postgresql-targets.md:77, the store-object-convergence row indocs/darling/admin.md:254). - VERIFIED — "every file I changed is CRLF in the working copy" —
fileon all nine touched/newdocs/darling/*.mdplusDarling/README.mdreports CRLF line terminators throughout. - VERIFIED — "
CHANGELOG.mdanddocs/changelog/… I left their links alone" — neither appears ingh pr diff 6722 --name-only. - VERIFIED (spot check) — "Sub-headings that nothing links to have no stub… I checked that no tracked file links to any of them" — grepped the repo for
#the-sessions,#clones,#what-gets-collected,#if-it-does-not-connect; no inbound references outside the destination pages' own TOC-adjacent prose. - UNVERIFIED — "I built
Darling.TestsandLite.Testswith-p:EnableWindowsTargeting=trueon macOS: 0 errors for each" — requires running a .NET build; not done in this read-only review. - UNVERIFIED — "I ran
scrubcheckon the full diff… 0 hits" — tool/output not available to this review. - UNVERIFIED — "Link check: … 0 broken" (the author's own exhaustive sweep) — I independently re-verified ~80 of the relative links and GitHub-slug anchors by hand across every changed file (all resolved), but can't confirm the claimed automated sweep itself ran clean.
- REFUTED — "The README goes from 2,457 lines and 545,401 bytes … to 1,755 lines and 404,734 bytes" —
wc -l/wc -con the currentDarling/README.mdreport 1,817 lines / 409,707 bytes, 62 lines and ~4.9 KB more than the stated final figures. Purely cosmetic (likely stale from before the final rebase/commit), no functional impact — the directional claim ("about a quarter smaller") still holds.
Riskiest lines
docs/darling/monitored-servers.md:80-97— the relocated grant table that gates SQL Server collection; a silent future edit here would ship unnoticed — UNCOVERED (no behavior test pins this page).Darling/README.md:115(](../docs/darling/monitored-servers.md#permissions-on-monitored-servers)) — the stub link operators actually click post-move; if the target heading's slug ever drifts, nothing in CI fails — UNCOVERED.docs/darling/admin.md:254— the "Store object convergence" cadence row moved fromDarling/README.md— covered byStoreObjectConvergenceTests.TheReadme_NamesTheHourlyConvergenceandRetentionReevaluationTests, both updated in this PR to readadmin.md.
Traps near this diff
.gitattributes says text=auto eol=crlf: … A diff that touches every line of a file is a line-ending conversion until proven otherwise— does not apply: every changed file shows a targeted content diff (913 additions / 777 deletions across 23 files, not whole-file rewrites), andfileconfirms CRLF survived in both old and new docs.Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding— does not apply: this PR only reorganizes Darling-specific operator documentation underdocs/darling/; it touches no Lite doc, code, or test, and Lite has no equivalent oversized-README problem being solved.
Divergence (blind read vs PR body)
One divergence, captured as REFUTED above: the PR body's stated final line/byte counts for Darling/README.md don't match the tree as checked out (1,817/409,707 vs. the claimed 1,755/404,734). Everything else — the six-section move list, the stub inventory, the 34 inbound-link retargets, the five test-pin retargets, the "sections left alone because open PRs touch them" list — matches the diff exactly; I found no section claimed as moved that wasn't, no stub claimed as kept that was dropped, and no inbound link claimed as fixed that still points at the old location. This is a cosmetic PR-description inaccuracy, not a code or content defect — update the stated counts, or note they predate the final rebase.
Reviewer checklist
- Run the author's claimed link-check script (or an equivalent) and the Windows-targeted test build, since neither could be executed from this read-only review.
- Reconcile or drop the PR body's Darling/README.md line/byte-count figures against the final tree.
|
Claude posting for Erik Darling. Answers to the review at 8954435. No code change.
I added |
Claude posting for Erik Darling. The one refuted claim was the body's README line and byte counts, now corrected for the dev merge. No code change. Details in the PR comment.
erikdarlingdata
added
the
re-review
|
Non-blocking observation, not in this diff: Everything else checks out: verified by hand (and cross-checked with two independent passes) that every "moved to" stub heading's link resolves to a real heading in its target file (~25+ anchors sampled, including the trickier ones like |
There was a problem hiding this comment.
Verdict: LGTM
Claims
- VERIFIED — "the README goes from 2,519 lines ... to 1,817 lines ... on this branch" — current
Darling/README.mdis exactly 1817 lines (wc -l). - VERIFIED — "Every moved heading that a link points at keeps a stub heading in the README, with the same text" — spot-checked ~15 stub headings (e.g.
Darling/README.md:101"Windows stops at start: the CET error",:113"Permissions on Monitored Servers") against their pre-move text in the diff; identical. - VERIFIED — the "Where each section went" table matches the diff:
install.md,admin.md,monitored-servers.md,postgresql-targets.md,viewer.md,managed-postgresql.md,alerts.mdeach receive exactly the content the table says, confirmed by direct comparison of deleted/added hunks. - VERIFIED — "I added no new tests" — all 5 touched test files only change the
ReadRepoFile(...)call and the failure-message string inside an existing[Fact]; no new test methods. - VERIFIED — "Link check: ... Result: 0 broken" — independently resolved ~30 relative links/anchors added or rewritten by this PR (across
Darling/README.mdstubs and the new/changeddocs/darling/*.md,docs/darling-quick-start.md,docs/alerts.md,docs/permissions.md,docs/troubleshooting.md, etc.) against GitHub's heading-slug rules; all resolve, including tricky ones with punctuation (#configure-darlingjson,#check-hoststore-sizing-settings) and the explicit<a id="run-on-linux-docker-compose-or-systemd-1804">anchor. - UNVERIFIED — "I built
Darling.TestsandLite.Testswith-p:EnableWindowsTargeting=trueon macOS: 0 errors for each" — requires running a build; not checkable from a read-only tree. - UNVERIFIED — "Scrub: I ran
scrubcheckon the full diff ... 0 hits" and "Rebase: ... I ran all the checks again" — both are live tool-run/workflow claims not reproducible from a static checkout.
Riskiest lines
Darling/README.mdstub "moved to" links (e.g.:103,:150,:1466) — a stub pointing at a dead anchor would silently 404 for every reader following an old bookmark. Covered by no automated test (REVIEW-TRAPS.md's tests-and-censuses ruling explicitly disallows a source-text/markdown-spelling test as "coverage"); mitigated instead by the PR body's own link check plus this review's independent link-resolution pass. UNCOVERED by test, but the PR body states why none is needed (the census-style check it describes, plus REVIEW-TRAPS' ruling against adding one).Darling/Darling.Tests/PgLogEventMetricsTests.cs:42-43(RepoFile.ReadRepoFile("docs", "darling", "postgresql-targets.md")+ the V130recent_runsassertion) — a page split that dropped or reworded that line would red this test. Covered byPgLogEventMetricsTests.TheRunbookAndTheReadme_NameBothFamiliesitself (a real xunit assertion that fails if the string is missing) — confirmed the string is present verbatim atdocs/darling/postgresql-targets.md:77.docs/darling/install.md:52(ReadRepoFile("docs", "darling", "install.md")consumed byDarlingRuntimePreflightTests.Readme_QuotesTheCetMessage_...) — the CET message and exit code moved into a much larger new file; a transcription slip here would be user-facing (operators searching for the literal error text) and would also red the test. Covered by that test; confirmed exact match atdocs/darling/install.md:548.
Traps near this diff
- "Ruling (#5833): tests are load-bearing. ... never answer a trap with a test that reads source text or asserts how code is spelled" — does not apply as a new violation: the 5 touched tests (
DarlingRuntimePreflightTests,NewerStoreWarningTests,PgLogEventMetricsTests,RetentionReevaluationTests,StoreObjectConvergenceTests) are pre-existing source-text-reading tests, only re-pointed at the new file path; no new test of this shape was added in this diff. - "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding" — does not apply:
Lite/README.mdis 24 lines and was never split this way, noLite.TestsreadsDarling/README.mdor any of the moved sections, anddocs/troubleshooting.md's shared Lite content is untouched by this PR (only Darling-side links were repointed). - "
.gitattributessaystext=auto eol=crlf: ... a diff that touches every line of a file is a line-ending conversion until proven otherwise" — does not apply: every diff hunk in this PR (Darling/README.md, the newdocs/darling/*.mdfiles, etc.) is a targeted, line-level content move, not a whole-file rewrite; the PR body's own CRLF check (git show HEAD | grep -c $'\r'= 0) is consistent with a clean line-ending story.
Divergence (blind read vs PR body)
The body's "Product-code references that the stubs keep working" section audits every .cs/.ps1/.json string and every docs link that names a moved section, but doesn't mention CONTRIBUTING.md:46 and :104, which describe Darling/README.md as covering "installation" and having "the full procedure" for viewer setup — both now moved out to docs/darling/install.md and docs/darling/viewer.md. Minor and non-blocking (flagged separately in a PR comment); the body's stated audit scope (product code + docs links) just didn't extend to CONTRIBUTING.md's own prose. Otherwise the body's account matches the diff on every claim graded above.
Reviewer checklist
- Confirm the actual Windows CI run for
Darling.Tests/Lite.Testsis green (the PR body's macOS cross-compile build check doesn't run these suites; they only execute on Windows CI).
github-actions
Bot
removed
the
re-review
|
Follow-up pass note: Everything the diff touches — For the record, since I did read it: the merged-in change looks sound on its own terms — |
There was a problem hiding this comment.
Verdict: LGTM
Claims
No new claim to grade: the follow-up range 89544354818e9663cfe6976d142374af8013219a..0149aa6f85440926e9446df3a158b153a7627212 holds a single commit, 0149aa6f8, a merge of origin/dev into HEAD with an empty body. The PR's only authored commit (68d5e50, the README split) sits before the range's BASE and was already graded in the prior full-pass review at head=89544354818e9663cfe6976d142374af8013219a.
Riskiest lines
Not applicable — no authored commit in range. The merge brings in dev's own #6598 forced-plan/query-history work (DarlingMcpForcedPlanTools.cs, DarlingMcpQueryStoreHistoryTools.cs, query-store-history.js), which per the follow-up protocol is not this PR's change and is out of scope since nothing else in the range interacts with it.
Traps near this diff
None evaluated — the only diff in range is base-branch content brought in by a merge, explicitly out of scope for this pass, and it does not interact with this PR's own README-split changes (checked: no overlap in files or content).
Divergence (blind read vs PR body)
None — the follow-up range contains no new authored content for the PR body to make claims about.
Reviewer checklist
- None beyond what the prior full-pass review already listed (Windows CI build/test run, link-check reconciliation) — this range adds nothing new to verify.
Claude posting for Erik Darling.
Part of #6416. This is phase 1.
#6416stays open.What this does
Darling/README.mdis too large for GitHub to render in full. This PR moves six of its sections into the task pages underdocs/darling/. It moves only the sections that no open PR edits. Every moved heading that a link points at keeps a stub heading in the README, with the same text, so the old anchors still land. Each stub gives a link to the new page.Measured on the committed files, which have LF line endings: the README goes from 2,519 lines and 550,638 bytes (538 KB) on
origin/devto 1,817 lines and 407,890 bytes (398 KB) on this branch. Both counts are at 8954435. Dev grew by 62 lines after the first count (2,457 lines), in sections that stay in the README. A CRLF working copy has one more byte per line, sowc -cthere reads 409,707.Where each section went
Line ranges are from
origin/devat 1393e9a, where I took them.docs/darling/install.md, in a new "Install reference" part before "Where to go next"docs/darling-quick-start.md, under "Install on Linux"docs/darling/admin.md, under "Upgrade"docs/darling/monitored-servers.mddocs/darling/postgresql-targets.mddocs/darling/viewer.md, before "Read-only versus read-write seats"docs/darling/admin.md, at the enddocs/darling/managed-postgresql.mddocs/alerts.md, at the end"When to Choose Darling vs. Lite" stays in the README as its introduction. The two Quick Start pointer lines (to
install.mdanddarling-quick-start.md) stay as well.I carried the text over as it was. I changed only what the move broke:
#anchorlinks to sections that stay in the README back atDarling/README.md.The new text (the page intros, the stubs, the "Install reference" lead-in) has no tool or collector counts.
The new pages and the reading order
The three new pages start with
Audience: operators.under the H1, in the format of #6719.docs/darling/README.mdlists them as steps 7 to 9, before Troubleshooting. #6719 has not merged, sodocs/README.mddoes not exist on dev yet. When #6719 merges, add these three to its "Run" or "Configure" list:docs/darling/monitored-servers.mddocs/darling/postgresql-targets.mddocs/darling/managed-postgresql.mdStubs kept in the README
Each stub is a heading with exactly the old text, followed by one line that links to the new page.
## Quick Start(it keeps its two pointer lines and adds a list of where the content went),## PostgreSQL Targets,## Connect a Remote Viewer,## How It Runs (Reference),## Managed Bundled PostgreSQL,## Per-server threshold overrides.<a id="run-on-linux-docker-compose-or-systemd-1804">Sub-headings that nothing links to have no stub. Examples are "The sessions", "Clones", "What gets collected" and "If it does not connect". I checked that no tracked file links to any of them.
Inbound links updated in docs
I pointed every docs link to a moved anchor straight at its new page. That is 34 links in 11 files:
docs/authentication.mddocs/darling-quick-start.mddocs/darling-store-sizing.mddocs/darling/admin.mddocs/darling/cli.mddocs/darling/install.mddocs/editions.mddocs/performance-impact.mddocs/permissions.mddocs/postgres-first-target-runbook.mddocs/troubleshooting.mdIn the runbook I also changed two "README's ..." phrases, because the links no longer go to the README. Links inside the README to moved anchors now point at the new pages.
CHANGELOG.mdanddocs/changelog/are history, so I left their links alone. The stubs keep those links working.Product-code references that the stubs keep working (not changed)
None of these quote an anchor. Each names a section heading or "the README" in a string or a comment, and the README still has the heading.
Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs:9558,RollbackReadmeSection, which reads"Rolling back to an older build" in Darling/README.md. This text goes into the newer-store warning. The stub heading is there, and its link goes toadmin.md. This file is off-limits for this PR. A later PR can namedocs/darling/admin.mddirectly.Darling/tools/install-darling.ps1:2088: "the README (Prerequisites) has a workaround". The workaround is the CET section, which is now ininstall.md. The Prerequisites stub covers the old wording.DarlingWorker.cs:13760and:13837: "see the monitoring-login grants in the README". Covered by the Permissions on Monitored Servers stub.MonitoringGrantScript.cs:260: "the service's Log On account was changed (see Darling/README.md)". Covered by the gMSA stub.DarlingServiceConnectionTest.cs:77: "with the permissions in the README". Covered by the same stub.darling.sample.json:61, 80, 389, 396, 492,DarlingCollectorRunner.cs:4137andDarlingWorker.cs:8180, 8916, 11671, 16593. These name sections that did not move: Background workers, LAN endpoints, Logs and the alerts section.Tests
Five existing pins read text from sections that moved. Each one now reads the page that holds the text. The assertions did not change.
DarlingRuntimePreflightTests.Readme_QuotesTheCetMessage_...(Darling case)docs/darling/install.mdNewerStoreWarningTests.The_readme_rollback_list_names_only_rungs_the_ladder_shipsdocs/darling/admin.mdPgLogEventMetricsTests(the V130recent_runsline)docs/darling/postgresql-targets.md. The other two asserts in that test still read the READMERetentionReevaluationTests(the cadence row)docs/darling/admin.mdStoreObjectConvergenceTests.TheReadme_NamesTheHourlyConvergencedocs/darling/admin.mdI added no new tests. I checked the other README pins by script. Every literal they assert that was in the README before the move is still in it.
I built
Darling.TestsandLite.Testswith-p:EnableWindowsTargeting=trueon macOS: 0 errors for each. These suites run only on Windows, so CI is the first place they run.Checks I ran
.mdfile. Each path is ingit ls-files, or is one of the three new pages. Each anchor is checked against the headings of its target, with GitHub's rules for duplicate slugs. Result: 0 broken. A run over all tracked docs found only two image links indocs/features.md, which were broken before this PR and are not in it.documentationandfinops-index-analysis-per-database-grants, which were never README anchors. No anchor later in the README was renumbered.git show HEAD | grep -c $'\r'is 0.Darling/README.mdin each open PR before I started. Their hunks are only in Configuration Reference, Operations, Agent Changes, Opt-in Network Endpoints and Security & Least-Privilege Roles. One exception: Darling collects Query Store hints: get_query_store_hints lists them and Change Watch sees one set or cleared (#6581) #6619 changes the intro paragraph at line 9, which this PR does not touch. Darling collects Query Store hints: get_query_store_hints lists them and Change Watch sees one set or cleared (#6581) #6619 adds no link to a moved anchor.scrubcheckon the full diff, the commit message and this body. 0 hits on each.Out of scope
docs/darling/install.mdis now about 59 KB. That is near the issue's target of 60 KB for each page. Phase 2 can split its new "Install reference" part out to its own page if needed.CHANGELOG entry
Darling/README.md) is about a quarter smaller. Its install reference, PostgreSQL targets, remote viewer, service cadences, managed store and per-server threshold override sections moved to the task pages underdocs/darling/. The old links still work (Split Darling/README.md (509 KB, 2,269 lines, 106 headings) into task pages and a generated reference #6416).