doctor(Settings): stop the comments describing the world before the cutover - #1778
Conversation
…ents The Status doc said deleting sets Deleted and the row is never removed; nothing sets Deleted and the seeding teardown really removes the row. The offsets block said the four sub-period boundaries were non-strictly ascending; Validate rejects equal ones, and [Range] is what pins them below zero. Year gains the one thing the code cannot say — it is derived from the gate-opening date.
The rebuild removed its GET and its view and deleted the carry screen beside it, but six comments still describe the section as owning two admin screens with forms of their own. /Settings/Admin is a POST endpoint; the form it binds lives in the /Settings#event tab, which is also where the admin-exemption (memory/code/localization-admin-exempt.md) now attaches.
The #1104 cutover shipped, so nobody is still reading the calendar off Shifts. The clock-rule remark named the EF entity as the forwarder when the DTO is what forwards. Three consumer enumerations — the leaf's csproj, the key/value rationale and the listener summary — had each gone stale again; they are gone rather than re-listed, since a comment cannot hold a consumer list true.
data-access.md said the service was registered twice (it is three ways — ISettingsService, ISettingsWriteService, IEventSettingsSeeding), that SaveEventSettingsAsync held one invariant (it holds two: one-active and the EarlyEntryStartOffset range), listed two key/value consumers of three (Workgroups' Drive root was missing), and repeated the entity's false "never a row removal" claim — IEventSettingsSeeding.DeleteEventAsync really deletes. Settings.md carried the same removal claim and an admin-controller sentence that outlived the second controller. Also qualified the bare issue refs the rebuild left in SectionAdminNav, SettingsAdminController, EventSettingsViewModel and both docs (memory/process/issue-refs-qualified.md). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014DscesvhuwCEnH5SgXdGyY
EventSettingsViewModelTests only ever called Validate() directly, so the [Range] attributes on the four sub-period offsets — the "< 0" half of the build-window rule — ran in no test. Two tests now go through Validator.TryValidateObject(validateAllProperties: true), the path MVC takes. Also pinned: the Challenge() branch when the principal carries no usable id (every save is audited by actor, so it must not save anonymously), that /Settings/Admin exposes no GET, and that /Settings is [Authorize] with no policy rather than AdminOnly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014DscesvhuwCEnH5SgXdGyY
…over left stale CENTRAL-33 is fixed: dependency-graph.md:126 styles DriveMon :::monitor and the graph has a Monitor section. Its surviving sibling concern, the duplicate classDef, is already CENTRAL-48. Filed CENTRAL-63 (design-rules.md §8) and CENTRAL-64 (service-data-access-map.md): both still say settings_event has no readers and is populated by /Settings/Admin/Carry — a screen the cutover deleted. Central regulations, so they go to the ledger rather than this run's diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014DscesvhuwCEnH5SgXdGyY
Second pass. Structure still matches target, so the target is largely the previous one re-derived; the corrections are the seam bullet (nine sections contribute an ISectionSettings tab, not one), the event-cycle readers (Shifts and Camps read ISettingsService directly), and a structure bullet for the way in — SectionChrome's user-menu slot, since nothing else links to /Settings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014DscesvhuwCEnH5SgXdGyY
"Carried" named the deleted carry service, not anything the test does — the helper builds a saved row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014DscesvhuwCEnH5SgXdGyY
Every file:line cite in the target re-read against the tree it now describes — five had drifted by a line or two as the strikes added and removed comment lines above them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014DscesvhuwCEnH5SgXdGyY
API-equivalent $, list rates; run under subscription quota. Measured Phase 1 to PR creation; PR create/backfill and Phase 8 excluded. Peak main-thread context: 288,996 tokens (section docs). Generated by Claude Code |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014DscesvhuwCEnH5SgXdGyY
|
Reviewed commit 448623e — no issues found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 448623ee79
ℹ️ 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".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
PR Surface ReportCompared Summary: 21 changed file(s) | EF migrations: 0 added file(s), max 0/1 per context Reforge Surface Score
Section DeltasNo section score changes. Section Size & Complexity Deltas
Rule DeltasNo rule score changes. Corpus Size & Complexity
At head: largest class Published Write Surface14 of 48 sections publish write capability, 23 interfaces (0). Interface SurfaceNo new interfaces or interface methods. Diff Size
New Files
|
… readers Codex, round one, both correct. The no-GET test asserted BeEmpty() over methods a section does not have — exactly the per-section absence test memory/architecture/no-tests-for-absences.md forbids, from the skill the atom already names twice as the source. The claim belongs in the target and the invariant doc, cited to source, and it is there. The target's event-cycle reader list named nine sections; the real count is past twenty. Replaced with the shape rather than a longer list — an enumeration going stale on every rebuild is finding 13 of this same run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014DscesvhuwCEnH5SgXdGyY
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8994def19e
ℹ️ 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".
…readers Review round two, both findings correct. The `[Authorize]`-has-no-policy test on `SettingsController` is the same forbidden shape as the one withdrawn a round earlier, wearing an authorization label. The atom's carve-out is for a request that exercises a route and pins the deny, not for reflection over attribute properties. Finding 25 goes back to carried: the controller still has no tests, and the one worth writing exercises the page against the tab composer, which is behaviour this run did not change. The invariant doc's cross-section table listed six inbound sections where nineteen reference `ISettingsService` in production code. The event-cycle readers now get the same treatment as in the target shape — described, and said out loud to be unenumerated. The rows bounded by `SettingKeys` and by the seeding seam are complete, so they stay listed. The atom now says the carve-out means a request test, and records both instances from this run.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a115108ba3
ℹ️ 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".
Review round three. Cutting the stale reader enumerations left a sentence explaining why the list is gone, and "each rebuild has left a shorter one behind" is PR archaeology in a doc bound by `memory/architecture/current-state-docs-no-history.md` — the History table is the only exempt part of `health.md`, and the invariant doc has no exemption at all. Both now carry the standing instruction, derive the readers from the call sites, and nothing about how the list got cut. Same class, third instance, in the seams section: the dead Shifts columns are described by what is true now rather than by the cutover that made them dead.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b019a7959
ℹ️ 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".
All four rulings landed 2026-09-21. nobodies-collective#1104 closed as delivered, with its four departures from the written criteria recorded on the issue as the better outcome and the still-present Shifts columns named as the one loose end. nobodies-collective#864 keeps its third pillar too — the tabbed settings console is the /Settings page — and stays open for OpenWindow and the analyzer rules, with a comment restating what OpenWindow was specified to be. Two rulings touch the target shape. The blank new-cycle form that discards a rejected submission stays as it is: once-a-year path, and the fix would be code that never runs. The Deleted status stays too, for removing a test event; the bullet claiming it exists for withdrawal was wrong and now says why it is really kept, closing an item first raised on 2026-08-28.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abc969fd54
ℹ️ 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".
CENTRAL-63 and CENTRAL-64 each abbreviated the second cutover issue to a bare `#1631`, which is the rule this run spent findings 17-21 enforcing everywhere else. Both now name the repository, per `memory/process/issue-refs-qualified.md`. Swept the whole branch diff rather than the two lines quoted: these were the only bare refs the branch adds. The bare refs elsewhere in the ledger are pre-existing rows this PR does not touch.
2 participants
What
Section-doctor run on Settings, 2026-09-21 (unattended daily). Run file:
docs/health/runs/2026-09-21-Settings.md.Target shape:
src/Sections/Humans.Settings/Docs/health.md.UPCOMING: Gdpr, CityPlanning, Email, Expenses.
No behaviour changed. The only new executable code is tests.
Why
Settings was rebuilt twice over since its last run — #1628 put the member-facing
/Settingspage and its contributed tabs in place, and nobodies-collective/Humans#1630 andnobodies-collective/Humans#1631 repointed every reader off the Shifts-owned row, moved cycle
minting here and deleted the carry screen.
The structure that came out of that matches the target shape this run derived, so there is no
architecture work here. The drift is what the rebuild left behind it: comments and docs still
describing the world before the cutover — a deleted carry screen,
/Settings/Adminas a screenwith views and a GET, readers still going through Shifts, and three consumer enumerations that had
each gone stale. A comment naming a deleted screen is not cosmetic; it is what a reader reaches
for when deciding whether a second GET already exists.
Worked (findings 1–23, 27–29):
Deletedis set by nothing and the row really isremoved by the seeding seam; the sub-period rule is a strict
<, with the upper bound comingfrom the
[Range]s, notValidate./Settings/Adminis a screen" class, across the contract, the csproj, both controllers,the tab view component and
_ViewImports.three consumer enumerations cut rather than re-listed — each had gone stale once per rebuild.
Workgroups consumer, the Cross-Section Dependencies table, and the "never a row removal" claim
in its second home.
#NNNrefs qualified.[Range]s now run underValidator.TryValidateObject(validateAllProperties: true), the path MVC takes; and theChallenge()branch on a save whose principal carries no usable id.memory/architecture/no-tests-for-absences.md— its authorization carve-out now says it means arequest test, and its instance list records this run's two.
dependency-graph.mdstylesDriveMon:::monitor).CENTRAL-63 and CENTRAL-64 filed:
design-rules.mdandservice-data-access-map.mdbothstill say
settings_eventhas no readers and is filled by/Settings/Admin/Carry.Skipped: findings 24 and 25 — both withdrawn in review, one per round. Each ended in a
per-section absence test, the second under an authorization label;
SettingsControllerstill hasno tests, and the test worth writing there exercises the page against the tab composer rather than
re-reading its
[Authorize]. Finding 26 (redundant tests inServiceTestsandEventSettingsViewModelTests) — a reviewer-gated deletion and the lowest-value item on the rankedlist; carried. CENTRAL-21 — verifying it needs a read outside this section's blast radius.
Findings 30 and 31 are below.
Existing surface checked
No new durable surface: no interface, DTO, endpoint, entity or DI change. The new tests use the
section's existing
HumansFactharness and theSettingsAdminControllerTestssubstitute setup.UI changes / screenshots
None — no view, resx or rendered markup changed.
Checklist
mainonpeterdrier/Humans.origin/main(0aa945756).EF migrations— none.NuGet packages updated?— none.no-tests-for-absencessharpened in place, so itsmemory/INDEX.mdline still holds.dotnet test Humans.slnx -v quietgreen;dotnet format whitespace --verify-no-changesclean.Nav coverage— no new pages.Dates/times via NodaTime, icons via FA6— no date or icon changes.Reviewer notes
The deleted consumer enumerations are the one judgement call worth a second opinion. Each listed
the sections that reference this one, and each had gone stale on every rebuild; they were cut
rather than re-listed, on the grounds that a list nothing keeps in step is worse than no list. Two
review rounds pushed back on the same call and both were answered the same way — describe the
shape, say out loud that it is not enumerated. Where a set is genuinely closed (the key/value
callers, bounded by
SettingKeys; the seeding seam; the change listeners) the rows stay listed.Needs Peter — all four answered
Answered by Peter 2026-09-21 and applied in
abc969fd5.criteria (table keeps the
system_settingsname,ISettingsServicerather thanISettingsServiceRead,/Settingsrather than/Admin/Settings, carry screen deleted ratherthan used) are accepted as the better outcome and recorded on the issue, along with the one
loose end — the dead app-wide columns still sitting on the Shifts row.
it specified is the
/Settingspage, so pillar three is shipped alongside pillar one. It staysopen for the
OpenWindowprimitive and the two analyzer rules; the issue now carries a commentrestating what
OpenWindowwas specified to be.runs about once a year and only when the operator fights the field rules. Recorded in the
target's Deliberately not done so the next run stops finding it; not ledgered, because it is
a decision rather than debt.
EventSettingsStatus.Deleted— kept. It is there for deleting a test event once one ismade. The target's bullet claimed the state existed for withdrawal, which was wrong; it now
gives the real reason. Closes the item first raised on 2026-08-28.
🤖 Generated with Claude Code
https://claude.ai/code/session_014DscesvhuwCEnH5SgXdGyY
Generated by Claude Code