janpeter/Ops-dashboard
1
0
Fork 0
Code Issues Pull requests 1 Projects Releases Packages Wiki Activity Actions

feat(hub-settings): instellingenpagina voor de S4M Hub-hooks per host #143

Merged
janpeter merged 38 commits from feat/hub-hook-settings into main 2026-08-20 08:09:57 +02:00
Conversation 2 Commits 38 Files changed 37 +5173 -5
janpeter commented 2026-08-20 07:54:05 +02:00
Owner
Copy link

Eén pagina /settings/hub die per host (mac, max2, srv) toont wat er van de twee S4M Hub-hooks geregistreerd en actief is, en waarmee de twee wachttijden op de eigen host gezet kunnen worden.

Architectuur

Elke dashboard-instance leest zijn eigen host via de lokale ops-agent en hangt een JSON-snapshot aan de bestaande hartslag; de pagina rendert de fleet uit de gedeelde ops_dashboard-database. Er gaat niets cross-host — dat is het expliciete niet-doel uit 2026-05-28-multi-host-fleet-design.md.

De veiligheidsgrens ligt in de agent-whitelist, niet in het formulier: de toegestane wachttijden (0/120/300) staan in args.allowed, de agent voert geen shell uit en interpoleert niets. Zelfs een gecompromitteerd dashboard kan geen willekeurige waarde zetten. Het ingest-secret verlaat de host nooit — de snapshot draagt alleen secret_present, de hostnaam uit HUB_URL (niet de volledige URL) en de soort van elk hookcommando (niet het commando).

Herkomst

Spec (SPEC-GO, revisie 12, 11 reviewronden) en plan (PLAN-GO, 9 planronden) zijn met twee cross-model reviewers doorgelopen; in geen van beide loops is een finding verworpen. Beide documenten dragen hun volledige ## Review record.

Stand van de uitrol

  • Migratie 20260819210000_instance_hub_snapshot is toegepast op de live ops_dashboard-database (drie nullable kolommen + instance_hub_snapshot_seq), centraal en vóór enige code-uitrol.
  • De hub-module is geïnstalleerd op scrum4me-server en max2: beide hosts rapporteren schema 2 met alle acht readiness-vlaggen true en geen whitelist-drift. Op beide is de agent-herstart apart bewezen door de key via /agent/v1/exec uit te oefenen.
  • De app-code is nog niet gedeployd; /settings/hub bestaat nog op geen enkele host. Dat is de volgende stap na deze merge.

Gate

npm run typecheck groen · npm test 1178 groen (baseline 1106) · npm run build slaagt. De 4 falende tests in test/scrum4us-deploy-trigger.test.ts zijn pre-existing op main en niet door deze branch geraakt.

Drie defecten die pas bij het draaien bleken

  1. De sudo-stub in de plan-testcode logde met echo "$@", en bash-echo eet een eerste argument -n op als vlag — de vormassertie kon nooit matchen.
  2. De testcode gebruikte BigInt-literals terwijl deze repo ES2017 target; tsc --noEmit weigert die.
  3. install-hub-module.sh schreef naar /etc/sudoers.d/ en valideerde pas daarna — de vorm die install-network-module.sh ook heeft. Een syntaxfout breekt dan sudo host-breed terwijl de back-up onbereikbaar is. Nu wordt een kandidaat gevalideerd en pas bij exit 0 het echte bestand aangeraakt; de test bewijst dat sudoers byte-identiek blijft als de validatie faalt.

🤖 Generated with Claude Code

Eén pagina `/settings/hub` die per host (`mac`, `max2`, `srv`) toont wat er van de twee S4M Hub-hooks geregistreerd en actief is, en waarmee de twee wachttijden op de **eigen** host gezet kunnen worden. ## Architectuur Elke dashboard-instance leest zijn **eigen** host via de lokale ops-agent en hangt een JSON-snapshot aan de bestaande hartslag; de pagina rendert de fleet uit de gedeelde `ops_dashboard`-database. Er gaat niets cross-host — dat is het expliciete niet-doel uit `2026-05-28-multi-host-fleet-design.md`. De veiligheidsgrens ligt in de agent-whitelist, niet in het formulier: de toegestane wachttijden (`0`/`120`/`300`) staan in `args.allowed`, de agent voert geen shell uit en interpoleert niets. Zelfs een gecompromitteerd dashboard kan geen willekeurige waarde zetten. Het ingest-secret verlaat de host nooit — de snapshot draagt alleen `secret_present`, de hostnaam uit `HUB_URL` (niet de volledige URL) en de *soort* van elk hookcommando (niet het commando). ## Herkomst Spec (SPEC-GO, revisie 12, 11 reviewronden) en plan (PLAN-GO, 9 planronden) zijn met twee cross-model reviewers doorgelopen; in geen van beide loops is een finding verworpen. Beide documenten dragen hun volledige `## Review record`. ## Stand van de uitrol - Migratie `20260819210000_instance_hub_snapshot` is **toegepast** op de live `ops_dashboard`-database (drie nullable kolommen + `instance_hub_snapshot_seq`), centraal en vóór enige code-uitrol. - De hub-module is geïnstalleerd op **scrum4me-server** en **max2**: beide hosts rapporteren `schema 2` met alle acht readiness-vlaggen `true` en geen whitelist-drift. Op beide is de agent-herstart apart bewezen door de key via `/agent/v1/exec` uit te oefenen. - De app-code is nog **niet** gedeployd; `/settings/hub` bestaat nog op geen enkele host. Dat is de volgende stap na deze merge. ## Gate `npm run typecheck` groen · `npm test` 1178 groen (baseline 1106) · `npm run build` slaagt. De 4 falende tests in `test/scrum4us-deploy-trigger.test.ts` zijn pre-existing op `main` en niet door deze branch geraakt. ## Drie defecten die pas bij het draaien bleken 1. De sudo-stub in de plan-testcode logde met `echo "$@"`, en bash-`echo` eet een eerste argument `-n` op als vlag — de vormassertie kon nooit matchen. 2. De testcode gebruikte BigInt-literals terwijl deze repo ES2017 target; `tsc --noEmit` weigert die. 3. `install-hub-module.sh` schreef naar `/etc/sudoers.d/` en valideerde pas daarna — de vorm die `install-network-module.sh` ook heeft. Een syntaxfout breekt dan sudo host-breed terwijl de back-up onbereikbaar is. Nu wordt een kandidaat gevalideerd en pas bij exit 0 het echte bestand aangeraakt; de test bewijst dat sudoers byte-identiek blijft als de validatie faalt. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Eén pagina die per host toont wat er van de permission- en AskUserQuestion-hook
geregistreerd en actief is, en die de twee wachttijden laat zetten binnen een
opsomming die in de agent-whitelist staat en niet in het formulier.

Kern van het ontwerp is dat er niets cross-host gebeurt: alle drie de hosts
draaien al een instance met een eigen agent (mac, max2, srv, alle drie met een
verse hartslag), dus elke instance leest zijn eigen host en stuurt een snapshot
mee met de bestaande hartslag. Daarmee blijft het niet-doel uit het fleet-ontwerp
van 2026-05-28 overeind en komt er geen nieuwe schrijfweg tussen hosts bij.

De iOS-kant vergt geen nieuwe API: het dashboard heeft al een read-only
verbinding naar de scrum4me-DB en de rol ops_readonly heeft SELECT op alle
hub_-tabellen.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8 taken, TDD, met exacte code: snapshotbouwer + reporterscript op de host, een
setterscript waarvan de toegestane waarden in de agent-whitelist staan, twee
snapshotkolommen op Instance, verzamelen via beide hartslagpaden (CLI voor
mac/max2, API-route voor de gecontaineriseerde srv), de fleet-pagina, de mutatie
die alleen de eigen host raakt, het iOS-paneel uit de read-only scrum4me-DB en
de uitrol per host.

Twee dingen zijn expliciet als onzekerheid benoemd in plaats van weggeschreven:
het exacte antwoordformaat van /agent/v1/exec (de agent streamt regels; de
uitpaklogica moet op het echte formaat worden aangepast, niet het script op de
test) en de naam van de slug-export in lib/instance/current.ts.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
De twee BLOCKERs raakten de dragende aannames:

- ops-agent draait als gebruiker ops-agent en kan /home/janpeter (750) niet eens
  binnenlopen, laat staan het 0600-envbestand lezen. Zelf nagemeten met
  sudo -u ops-agent test -x/-r: beide nee. Beide commando's krijgen nu de
  sudo -n-vorm die 17 bestaande whitelist-entries al gebruiken, met een
  sudoers-regel als installatiestap. Op de Mac speelt dit niet — het defect is
  asymmetrisch en zou bij een Mac-only test onzichtbaar blijven.
- De claim over SCRUM4ME_DATABASE_URL klopt op srv (rol, grants en container-env
  geverifieerd), maar de Mac heeft diezelfde variabele wijzend naar
  ops_dashboard. Aanwezigheid bewijst dus niets: het iOS-paneel gaat nu achter
  een positieve probe op to_regclass('public.hub_devices').

Verder: bundler-vrije verzamelmodule (server-only resolvet niet in het
ts-node-hartslagpad), harde deadline van 5 s op de agent-call met een
onvoorwaardelijke upsert, uitrolvolgorde voor de migratie op de gedeelde DB,
sub-paneel hub-serverconfig geschrapt (die waarden staan niet in de DB en twee
van de drie zijn code-defaults), agent_url gecorrigeerd van "ongebruikt" naar
load-bearing voor Control Room v2, verouderingsdrempel terug naar het bestaande
ONLINE_WINDOW_MS van 120 s, reader met statusuitkomst, capability-guard op de
route, en de mutatieroute die geen slug uit de client accepteert.

De secretclaim is vervangen door de eis die het risico echt afdekt: atomaire
rewrite in dezelfde map met mode/eigenaar behouden en een controle achteraf dat
HUB_INGEST_SECRET en HUB_URL nog bestaan.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Twee vondsten kwamen voort uit de fix van ronde 1 en waren van buiten niet te zien:

- hub_hook_status miste read_only: true. De agent classificeert alles zonder dat
  veld als mutating en neemt daar een host-gescopete lease voor; dit commando
  draait elke 60 s op elke host, dus dat werd een permanente lease met
  409-botsingen tegen deploystappen.
- Het setterscript draait als root en valideerde zijn eigen argument niet, terwijl
  een sudoers-regel zonder argumentenlijst elk argument toestaat. Het envbestand
  wordt door beide hookwrappers ge-source-d, dus een waarde met een newline erin
  schrijft shell die in elke Claude Code-sessie wordt uitgevoerd. Nieuwe stap 0
  valideert hook|ask en ^(0|120|300)$ in het script zelf.

Verder: twee klokken uit elkaar getrokken (veroudering meet uitsluitend
hub_snapshot_at, want de hartslag slaagt bewust ook als de snapshot faalt, plus
een test die die invariant vastlegt), de readiness-probe verbreed tot een echte
SELECT op elke tabel die het paneel gebruikt en op dezelfde pool (to_regclass
toetst geen rechten), het tijdelijke bestand wordt met O_EXCL en mode 0600
aangemaakt vóór de eerste byte en pas na validatie hernoemd, en de foute telling
van sudo-entries is vervangen door de gemeten 10 van 13 — met de waarschuwing dat
de vier repo-kopieën van commands.yml uiteenlopen.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Beide reviewers wezen op hetzelfde gat met verschillende severity: r3 maakte de
argumentvalidatie in het setterscript load-bearing en daarmee een derde bron van
de toegestane waarden, maar de drift-test vergeleek nog steeds alleen whitelist
en UI. De test dekt nu drie bronnen.

Daarbij is de te lezen kopie expliciet gemaakt. Een build-time test kan alleen in
de tree kijken, en er blijken vijf uiteenlopende kopieën van commands.yml te
bestaan; alleen deploy/ops-agent/baseline/commands.yml is byte-identiek aan de
live server (md5 gecontroleerd) omdat check-ops-agent-drift.sh hem daartegen
diff't. Leest de test een andere kopie, dan bewijst hij niets over de host.

Verder: de readiness-probe is geschrapt ten gunste van de echte queries (een
losse probe bewijst hetzelfde en kan achterlopen op de paneelbreedte), het
tijdelijke bestand krijgt een unieke naam zodat O_EXCL na een afgebroken run niet
permanent blokkeert, het auditvolume van een commando per minuut is benoemd met
de filtertip, en de sudo-telling is vervangen door een tabel per bestand met de
live host erbij.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
De MAJOR was terecht en niet met documentatie te dekken: deploy/ops-agent/
baseline/commands.yml is volgens zijn eigen README de "154 half"; max2's
whitelist leeft in scrum4me-docker en is bij de build hier niet leesbaar. Een
groene drift-test bewees dus niets over de host waar de knoppen straks actief
zijn.

Opgelost door de borging te verplaatsen naar de snapshot: het statusscript
rapporteert de args.allowed van zijn eigen host als allowed_waits. De pagina
toont per rij alleen knoppen voor waarden die die host echt accepteert en
markeert een afwijking van de UI-enum. De build-time test blijft, maar alleen
voor wat hij kan bewijzen.

Verder: 42P01 toegevoegd aan de unavailable-tak (het kanonieke Mac-geval gaf
juist die code, niet 42501), de scripts krijgen een plek in de repo onder
ops-agent/wrappers/hub/ met een install-module naar het model van het
netwerk-equivalent, de wijzigingsvolgorde bij de clamp is expliciet gemaakt
(host eerst, dan de baseline herbevriezen — die is een spiegel, geen bron), en
de kopieëntelling klopt nu inclusief het patch-template.

De statusregel stond nog op revisie 3 doordat mijn tekstvervanging stil faalde;
elke vervanging assert nu dat hij iets raakt.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
De MAJOR was precies en terecht: de agent valideert args.allowed op de concrete
command-definitie die hij uitvoert, en deze feature heeft twee setters. Eén
hostbrede allowed_waits-lijst kon er dus hoogstens één beschrijven — en drift die
de twee entries verschillend raakt is juist het soort out-of-tree drift waarvoor
het veld bestaat. allowed_waits is nu per setter.

Daarbij drie randen die de server-reviewer aanwees en die alle drie hetzelfde
patroon hadden: één waarde die meerdere oorzaken absorbeert. null (entry niet
gevonden), [] (entry zonder args.allowed) en whitelist_read: false (bestand niet
te lezen) zijn nu uit elkaar getrokken, met de padresolutie erbij — die verschilt
per platform en komt niet door sudo heen omdat env_reset aanstaat. De renderregel
is vastgelegd als de doorsnede met de UI-enum, zodat er nooit een knop bestaat die
het setterscript op diezelfde host in stap 0 weigert. En schema gaat naar 2, omdat
er tijdens de uitrol per definitie twee scriptversies naast elkaar draaien.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Beide reviewers raakten dezelfde blinde vlek van twee kanten. Het ontwerp
modelleerde wat de agent met de waarde doet, maar niet wat hij met het commando
doet, en niet hoe het antwoord terugkomt.

De mutatieroute was nooit vastgelegd, en beide bestaande patronen breken een
belofte: het huispatroon retourneert ok:true ongeacht de afloop (execAgent gooit
alleen bij een niet-2xx status, en de agent stuurt 200 zodra hij streamt, waarna
exit- en error-frames worden weggegooid), en het alternatief laat de client de
command-key kiezen en hangt aan capability flows, die op darwin niet bestaat. De
spec kiest nu POST /api/settings/hub/wait met hardgecodeerde keys op capability
settings, plus de eis dat de agent-aanroep exitcode en error-frame teruggeeft en
stderr apart houdt — anders is de beloofde foutmelding onhaalbaar.

En allowed_waits dekte alleen args.allowed, niet de cmd-vector die de agent
daarna spawnt: een halve module-installatie kan een perfecte allowed-lijst hebben
en toch de verkeerde wrapper aanroepen of de andere knop verzetten. Per setter
komen er nu vier readiness-vlaggen bij, waaronder sudo_ok dat met sudo -n -l
alleen het beleid raadpleegt zonder iets uit te voeren.

Verder: 409 van de admissielease geclassificeerd als "tijdelijk geblokkeerd" in
plaats van fout, en de niet-geregistreerd-alinea aangevuld met het Mac-geval dat
laat zien waarom "geen entries" en "geen whitelist" uit elkaar moeten.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
De assert in mijn eigen bewerkstap ving dat vier verwijzingen in de body nog de
oude veldnaam gebruikten; de review-recordsectie houdt de naam van toen, want die
beschrijft wat er in die ronde speelde.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Beide MAJOR's gingen over de vier readiness-vlaggen uit r7, en beide luidden:
zoals gespecificeerd is die vlag niet te bepalen zonder schade of zonder de
verkeerde vraag te stellen.

regex_ok was gedefinieerd als "het script accepteert dezelfde waarden". Dat is
gedrag, en gedrag stel je vast door te draaien — hier dus door de setter elke 60
seconden voor elke waarde aan te roepen, waarmee het statuscommando het
envbestand zou herschrijven en de wachttijd op de laatste testwaarde zou zetten.
De vlag wordt nu uit de bron van het geinstalleerde script gelezen; wil je toch
executie, dan eerst een neveneffectvrije --check-modus.

sudo_ok toetste de verkeerde identiteit. Het statusscript draait op linux zelf
als root, dus een kale sudo -n -l raadpleegt root's beleid terwijl de latere
mutatie door de agent-user wordt gestart. Nu expliciet met -U <agent-user>, per
waarde die de UI zou tonen, en drietallig: null zodra cmd[0] geen sudo is, want
op darwin draait de agent als de gebruiker zelf.

Verder: de mutatieroute schrijft zijn verse snapshot terug naar de eigen rij
(anders komt bij een refresh binnen een minuut de oude waarde terug), en de route
krijgt hasValidCsrf — deze knop kan met waarde 0 de goedkeuringspoort van elke
Claude Code-sessie op die host uitzetten, en dat hoort bij de verhoogd-risico
-conventie van de twee reveal-routes, niet bij die van restart.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Revisie 8 liet de mutatieroute zijn verse snapshot terugschrijven en gaf daarmee
hub_snapshot een tweede schrijver. Beide reviewers zagen de race: de hartslag
verzamelt eerst en schrijft daarna, dus een mutatie in dat venster wordt door de
daaropvolgende hartslag-write overschreven — en met hub_snapshot_at als
schrijftijd kreeg juist die verouderde inhoud de nieuwste stempel, waardoor de
verouderingsmarkering niet meer aanslaat. Nu: hub_snapshot_at is de meettijd, en
beide schrijvers updaten alleen voorwaardelijk.

claude_version stond er sinds revisie 1 en bleek onbepaalbaar in de context die
dit ontwerp zelf voorschrijft: sudo vervangt de PATH door secure_path en de CLI
staat daarbuiten. Nu via het absolute pad, en liever readlink dan uitvoeren.

regex_ok las de bron van een ander script en leunde daarmee op de vorm van diens
validatie. Het setterscript declareert nu zijn verzameling op een
machineleesbare regel waaruit het zijn eigen validatie afleidt; het statusscript
leest een contract in plaats van code.

Verder: de sudoers-regels voor de setter worden geenumereerd per combinatie —
anders staat een Cmnd zonder argumentenlijst elk argument toe en kan de
per-waarde-probe niets onderscheiden — en de foutafhandeling splitst
"statuscommando ontbreekt" van "hub-entries ontbreken", precies het onderscheid
waarvoor whitelist_read bestaat.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
De assert ving dat de aangehaalde foutafhandelings-bullet net anders luidde dan
aangenomen, waardoor deel 2 van de bewerking niet was geschreven. Nu compleet:
de testbullet-zin staat terug op zijn plek met de null-uitzondering, en
'statuscommando ontbreekt' is gesplitst van 'hub-entries ontbreken'.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Twee MAJOR's, allebei over een fix die maar half was doorgevoerd.

Sectie 1 beloofde dat het setterscript zijn validatie uit de declaratieregel
afleidt "zodat de twee niet kunnen uiteenlopen", maar sectie 2 stap 0 noemde de
verzameling nog als literal regex. Dan staan er twee bronnen in hetzelfde script
en leest regex_ok juist degene die niet bepaalt wat er gebeurt: groene vlag boven
een setter die weigert. Stap 0 leest nu die regel, en een test legt vast dat een
gedeclareerde waarde ook echt geaccepteerd wordt.

En collected_at bleek geen sluitende totaalordening: secondeprecisie in het
voorbeeld, geen tie-break bij gelijkheid, en het is de klok van een andere
machine dan die rendert. De tree gaf het antwoord al — Control Room markeert een
hartslag uit de toekomst als offline. De ordening loopt nu via een DB-sequence
(hub_snapshot_seq, genomen voor de agent-aanroep), collected_at krijgt
ms-precisie en dient alleen nog voor weergave, met een toekomstmarge als
klokfout. De conditie geldt uitsluitend voor de drie snapshotkolommen;
last_seen_at wordt altijd geschreven, anders valt een host offline precies
wanneer zijn agent faalt.

De zwerfzin uit ronde 8 is definitief weg. Hij landde drie rondes op rij
verkeerd omdat mijn assert alleen bewees dat een vervanging iets raakte, niet dat
het origineel verdwenen was; dat controleer ik nu ook.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Beide reviewers vonden onafhankelijk dezelfde MAJOR, voor het eerst in tien
rondes: de schrijfconditie uit r10 matcht nooit op een rij waar
hub_snapshot_seq nog NULL is. In SQL is 5 > NULL niet onwaar maar onbekend, dus
een WHERE die onbekend oplevert selecteert niets — geen enkele host zou ooit een
eerste snapshot wegschrijven, alle rijen blijven leeg en gelden als verouderd,
alle knoppen blijven uit, en er is geen foutmelding want er is geen fout. De
conditie krijgt nu een IS NULL-tak en een test op een verse rij.

Verder: CREATE SEQUENCE staat expliciet in het datamodel (Prisma maakt geen
sequence uit een nullable BigInt), nextval is als nieuw faalpunt vóór de
agent-aanroep afgedekt met dezelfde regel als een falende agent (snapshotkolommen
overslaan, hartslag gaat door), en de datamodel-kop telt weer kloppend.

Deze revisie is niet gereviewd: de cap van tien rondes is bereikt.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Beide reviewers keurden revisie 11 goed in de door JP toegestane
bevestigingsronde. De server-reviewer mat de schrijfconditie empirisch door op de
draaiende Postgres: NULL+1 schrijft, 7+9 schrijft, 7+5 niet, en 5 > NULL::bigint
is inderdaad unknown.

Hun vier GO-verenigbare MINOR's zijn verwerkt. Eén telt inhoudelijk: de conditie
hoort in de WHERE van de UPDATE en niet in applicatiecode, anders zit er een
venster tussen lezen en schrijven en is de race terug — terwijl alle sequentiële
tests gewoon slagen. Verder: de precedent-claim over handgeschreven DDL is
gecorrigeerd (die bestaat niet in deze repo; wel een met de hand aangevulde
migratie, nu bij naam genoemd), de nextval-alinea had de monotonie-zin
opgeslokt, en "DB onbereikbaar" is vernauwd tot sequence-specifieke fouten.

Eindstand: elf rondes, nul verworpen findings, van 2 BLOCKER / 8 MAJOR naar
dubbel-GO.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Het oude plan (aa9cefd) was onbruikbaar geworden: het kende geen sudo-vorm, geen
readiness-vlaggen, geen sequence-ordening, geen CSRF en geen eigen mutatieroute.
Dit is een herschrijving, geen bijwerking.

Structuur volgt de huisstijl die de spec noemt: de wrappers wonen in
ops-agent/wrappers/hub/ en worden geinstalleerd door een install-hub-module.sh
naar het model van install-network-module.sh.

Eén ding staat er expliciet in dat de spec niet noemt: install-network-module.sh
kopieert uit ops-agent/commands.yml.example (71 keys) terwijl de drift-test de
baseline leest (101 keys, byte-identiek aan de live server). Die twee lopen dus
uiteen; Task 3 zet de entries in beide en benoemt het als bestaande schuld in
plaats van het te laten ontdekken tijdens de uitrol.

De eerste test van Task 5 controleert de vórm van de query, niet alleen de
uitkomst: de conditie moet in de where van de updateMany staan. Als read-then-
write gebouwd slagen alle sequentiële tests terwijl de race terug is.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
De BLOCKER was van mij: ik schreef ~/.claude/hub-hook.env in het plan terwijl de
spec overal absolute paden gebruikt. De agent start die scripts via sudo, en
sudo heeft env_reset — gemeten op scrum4me-server geeft sudo -n printenv HOME
gewoon /root, waar dat bestand niet bestaat. Elke linux-host zou zichzelf dus
als "niet geregistreerd" rapporteren terwijl de hooks er draaien, en mijn eigen
tests maskeerden het omdat ze HOME injecteren. Alle paden komen nu uit
variabelen met absolute defaults, met een test die draait met HOME=/root.

Verder vier MAJOR's van de andere reviewer, alle vier raak: nextval werd ín de
writer genomen en dus ná de agent-aanroep, waarmee precies de race terugkwam
waarvoor de sequence bestaat; de routetest liet een vreemde slug slagen terwijl
de spec weigeren eist; schema 1 naar null maakte de "knoppenset onbekend"-tekst
onmogelijk; en de readiness-vlaggen werden nergens getest op de plek waar ze
ontstaan — een implementatie die ze altijd true emitteert kwam door de suite.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Elke bevinding ging over een ronde-1-fix die maar half was doorgevoerd.

De scherpste: mijn fix "geen $HOME in de scripts" maakte de testsuite gevaarlijk,
want die stuurde nog wél via HOME. Een script dat zich aan de nieuwe regel houdt
valt dan terug op zijn absolute default — het echte envbestand van de host, dat
schrijfbaar is voor het account dat npm test draait. Task 2's tests zouden de
live wachttijden hebben omgezet, met een moment waarop de permission-hook op 0
staat. Nu geven beide harnassen HOME=/nonexistent mee plus expliciete S4M_-paden,
zodat een terugval de test laat falen in plaats van de host te herconfigureren.

Verder: Task 5's tests stonden nog op de 3-argumentvorm en importeerden de
allocator niet, Task 7 net zo; de readiness-"test" bestond uit alleen commentaar
en was dus altijd groen; en het collect-proza zei nog schema 1 naar null terwijl
de parser inmiddels een union teruggeeft.

Eén inhoudelijke correctie op mijn eigen ordening: voor de mutatieroute moet de
sequence juist NA de agent-aanroep worden getrokken, want daar is de meting het
eind van het setterscript. Vooraf trekken laat een hartslag die later begint met
oude data winnen.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
De BLOCKER van ronde 2 is dicht: beide reviewers bevestigden dat de harnassen
niet meer via HOME sturen en dat de readiness-tests kunnen falen. Wat overbleef
was drie keer hetzelfde: een fix die op één plek landde.

Task 7 stond nog op de drie-argument-writer, noemde de allocator niet, en miste
juist de afwijkende volgorde die ronde 2 invoerde — terwijl Task 7 de enige
aanroeper is waarvoor die uitzondering geldt. Nu vastgelegd in Interfaces, proza
en een test die agent -> seq -> write assert.

De twee-klokken-invariant was alleen in Task 1 verwerkt, niet in Task 6, en er
bestond geen functie die zo'n test kon aanroepen. Task 6 krijgt toRowView; de
pagina rendert daar uitsluitend uit, zodat de verschuiving van hub_snapshot_at
naar last_seen_at toetsbaar wordt in plaats van onzichtbaar.

En de readiness-tests schelden uit naar de echte sudo van de testmachine — niet
deterministisch, en juist het geval waarvoor de vlag bestaat (beleid weigert de
agent-user) was niet gedekt. Er staat nu een stub-sudo vooraan in PATH plus de
twee ontbrekende gevallen.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
currentInstanceSlug() bestaat nergens in deze repo; de echte helper is
getHostSlug(). Die valt blijkens zijn docstring bewust terug op 'srv' wanneer
INSTANCE_SLUG ontbreekt — prima voor lezen, fout voor een schrijfpad dat een rij
selecteert, want dan overschrijft een host zonder slug de snapshot van srv. De
route weigert nu met 500 zodra INSTANCE_SLUG leeg is en gebruikt getHostSlug
pas daarna.

De sudo-stub bewees alleen iets over de waarde, niet over de vorm. Een
implementatie die root's beleid toetst (geen -U), die de policy-vlag vergeet
(geen -l) of die de setter echt aanroept, gaf dezelfde uitkomsten — precies de
false-positive waarvoor de vlag bestaat. De stub valideert nu -n -l -U
<agent-user> --, logt elke aanroep, en een test assert de vorm, het uitblijven
van directe uitvoering, en één policy-check per toegestane waarde.

Verder: toRowView werd aangeroepen zonder import, de verouderingsdrempel stond
als tweede literal in het plan (ONLINE_WINDOW_MS wordt nu geexporteerd en
geimporteerd), en er was geen test voor een vreemde rij.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Twee BLOCKERs, één per reviewer.

Mijn server-only-uitzondering noemde nog één module, terwijl het
ts-node-hartslagscript er inmiddels drie laadt. Zou een uitvoerder de constraint
letterlijk volgen, dan crashen mac en max2 bij het laden en stoppen ze met
melden — en de hele verificatieketen maskeert het: npm test is groen omdat
test/setup.ts server-only mockt, typecheck is groen omdat TS het type wel
oplost. De regel is nu een eigenschap van het pad, met een regressietest die de
bestanden als tekst leest; dat is de enige controle die de mock niet wegneemt.

En ONLINE_WINDOW_MS importeren uit registry-read.ts trekt prisma mee bij
module-load. De reviewers spraken elkaar hier half tegen — de server verwierp de
verdenking met bewijs dat CI een DATABASE_URL-placeholder zet, codex mat dat het
lokaal crasht — dus ik heb de oplossing gekozen die de vraag wegneemt: de
constante verhuist naar een DB-vrije module die beide kanten importeren.

Verder: de INSTANCE_SLUG-weigering stond alleen in proza en heeft nu twee tests,
en de sudo-stub controleert nu ook het wrapperpad en de selector — zonder die
twee zou een implementatie die naar de verkeerde wrapper wijst alsnog sudo_ok
true krijgen.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Beide reviewers vonden dezelfde zaak vanuit een andere hoek. Task 7's testblok
gebruikte zes symbolen die nergens gedefinieerd staan, en sinds de
INSTANCE_SLUG-guard vooraan in de keten staat, lopen zes van de acht tests op een
500 — want die variabele wordt in CI nergens gezet. Een uitvoerder die dat ziet,
wordt uitgenodigd juist die guard te verwijderen, en dat is precies de fix die
ronde 5 opleverde om een schrijfactie op de srv-rij te voorkomen. Er staat nu een
volledige preambule met mocks, een beforeEach die de slug zet, en een afterAll
die de oude waarde herstelt.

En de regressietest die de server-only-regel moet bewaken, codeerde die regel als
lijstje van drie bestanden — terwijl de constraint deze ronde juist algemeen was
gemaakt. Hij is nu fail-closed: hij leest de map en gebruikt een allowlist van de
twee modules die server-gebonden mogen zijn, zodat een nieuwe module standaard
gedekt is. Dat is exact het gat dat tussen ronde 1 en 5 ontstond.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Alle drie de findings zitten in de preambule die ronde 6 juist toevoegde.

De mock-factories lazen top-level const-spies, terwijl vi.mock gehoist wordt —
de factory evalueert dan voor de initialisatie. De bestaande tests in deze repo
gebruiken daarvoor vi.hoisted; die vorm is nu overgenomen.

De request-helpers bouwden een plain Request, terwijl hasValidCsrf
request.cookies.get() gebruikt. Op een web-standaard Request is dat veld
undefined en gooit de guard een TypeError — in alle acht tests, inclusief de
CSRF-test, die dan zou slagen om de verkeerde reden. Nu NextRequest, plus een
mock voor next/cache omdat revalidatePath nergens anders in deze codebase
voorkomt en er dus geen patroon is dat laat zien of hij buiten een
request-context veilig is.

En de allowlist van de server-only-regressietest gebruikte het criterium
"server-gebonden", terwijl de regel over het hartslagpad gaat. exec-setter.ts
valt daar precies tussen: server-gebonden, maar niet in dat pad, dus voor hem is
server-only juist correct. De set heet nu NIET_IN_HARTSLAGPAD en noemt hem met
reden.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
De routetest miste een mock voor @/lib/prisma. Dat bestand bouwt de client bij
module-load en gooit zonder DATABASE_URL; vitest laadt geen envbestand en
test/setup.ts mockt alleen server-only, dus de import van de route zelf faalt al
voordat een assertie draait.

En de criteriumwijziging van ronde 7 was alleen in de test geland: de Global
Constraint zei nog "alleen waits.ts en scrum4me-hub-reader.ts", terwijl de test
sinds die ronde ook exec-setter.ts toestaat. Een uitvoerder baseert zijn keuze op
die zin, dus dan verliest juist de module die met de agent praat en het
bearer-secret draagt zijn guard. Beide reviewers zagen het onafhankelijk.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex 0/0/0, server-claude 0/0/2, beide GO. De twee GO-verenigbare MINOR's zijn
alsnog verwerkt: het spec-geval "POST zonder sessie" landde nergens en was met de
oude preambule niet eens te schrijven (getCurrentUser werd als anonieme vi.fn
binnen de factory gemaakt, dus geen enkele test kon hem op null zetten), en de
redactietest dekte alleen het secret terwijl de spec ook de volledige HUB_URL en
het volledige hookcommando noemt.

Eindstand van de planloop: negen rondes, nul verworpen findings, van 1 BLOCKER +
5 MAJOR naar dubbel-GO. Het terugkerende patroon was een fix die op één plek
landde en op de andere niet — twee keer met gevaarlijke gevolgen: de testsuite
zou de live wachttijden van de host hebben omgezet, en een server-only-import
zou twee hosts uit de fleet hebben gehaald terwijl npm test en typecheck groen
bleven.

Volgende stap is de ceremonie, en die vereist JP's akkoord — hardstop.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Plan Task 1. hub-hook-status.sh + lib-snapshot.sh bouwen de snapshot (schema 2)
van deze host: wachttijden, registratievorm, whitelist en per setter de vier
readiness-vlaggen. Het script leest, meet en print -- het voert de setter nooit
uit en laat mtime en mode van het 0600-envbestand ongemoeid.

Geen $HOME en geen ~: sudo zet HOME op /root, dus alle paden komen uit een
variabele met een absolute default. sudo_ok toetst policy-only (-l) en expliciet
het beleid van de agent-user (-U), want het statusscript draait zelf als root.

Eén defect in de plan-testcode gevonden door hem te draaien: de sudo-stub logde
met `echo "$@"`, en bash-echo eet een eerste argument `-n` op als vlag -- de
drie aanroepen kwamen zonder newline en zonder `-n` in het log, waardoor de
vormassertie nooit kon matchen. Vervangen door printf, in de test en in het plan.
Plan Task 2. hub-hook-wait.sh leidt zijn validatie af uit de eigen
`# s4m-allowed`-declaratieregel -- één bron, geen tweede lijst in de body -- en
herschrijft het 0600-envbestand atomair: mktemp in dezelfde map met mode 0600
vóór de eerste byte, de kandidaat valideren (secret en URL aanwezig, doelsleutel
precies één keer, geen andere regel gewijzigd), en pas dan rename() met dezelfde
eigenaar.

De declaratiecontrole staat bewust vóór het sourcen van lib-snapshot.sh: de test
draait een kopie van dit script buiten de wrappermap, en die moet zijn diagnose
kunnen printen in plaats van te struikelen over een ontbrekende library.

Sluit af met `exec hub-hook-status.sh`, zodat setter en status niet twee
JSON-vormen kunnen ontwikkelen die uit de pas lopen.
Plan Task 3. De drie command-keys staan nu in ops-agent/commands.yml.example
(67 keys) én deploy/ops-agent/baseline/commands.yml (79 keys). Die twee lopen
historisch uiteen -- install-network-module.sh kopieert uit de example, terwijl
de drift-detector de baseline gebruikt -- dus een entry in één van beide is
onzichtbaar voor de andere helft. Bestaande schuld, hier expliciet benoemd.

hub_hook_status draagt read_only: true; de setters niet, want dat zijn mutaties
en die horen onder de host-scoped lease te vallen.

install-hub-module.sh schrijft GEËNUMEREERDE sudoers-regels: 1 voor het
statusscript (met "" zodat argumenten verboden zijn) en 6 voor de setter
(hook|ask x 0|120|300). Een Cmnd zonder argumentenlijst staat elk argument toe,
en dan kan sudo_ok per waarde niets onderscheiden -- de readiness-vlag zou
"klaar" melden voor een beleid dat in werkelijkheid hook 86400 toestaat. De
waarden komen uit de s4m-allowed-declaratie van het setterscript zelf, niet uit
een tweede lijst in dit script.
Plan Task 4. Drie nullable kolommen op Instance plus instance_hub_snapshot_seq.

Nullable is een keuze die vastzit aan Task 5: de schrijfconditie krijgt daarom
een `IS NULL OR`-tak. Zonder die tak schrijft geen enkele host ooit een eerste
snapshot weg -- `x < 7` matcht een NULL-rij nooit -- en dat zou stil gebeuren,
fleet-breed, met alle knoppen uit en zonder foutmelding. De spec staat ook
NOT NULL DEFAULT 0 toe; wie daarvoor kiest moet Task 5 in dezelfde beweging
meenemen.

De sequence staat in de migratie en niet in het schema, omdat Prisma er geen
genereert voor een kolom zonder autoincrement-default.
Plan Task 5. snapshot-schema.ts (Zod + parseSnapshot met de drie uitkomsten),
collect.ts (kale fetch, 5 s deadline, alles gevangen -> null) en
write-snapshot.ts (allocateHubSnapshotSeq + writeHubSnapshot), gehaakt in beide
hartslagpaden: het ts-node-script en de in-process API-route.

De conditie staat in de WHERE van één updateMany -- geen read-then-write, want
dan slagen alle sequentiële tests terwijl de race terug is. De IS NULL-tak is
niet optioneel: x < 7 is in SQL onbekend voor een NULL-rij, dus zonder die tak
schrijft geen enkele host ooit een eerste snapshot weg.

De sequence wordt door de aanroeper getrokken, vóór de agent-aanroep, omdat de
ordening bij het moment van de méting hoort. De mutatieroute (Task 7) doet het
omgekeerd, en om precies dezelfde reden.

De upsert blijft onvoorwaardelijk: een falende nextval, een onbereikbare agent,
een timeout of een geweigerde write laten last_seen_at gewoon bijwerken.

Twee defecten in de plan-testcode, beide alleen zichtbaar door te draaien:
- De fail-closed regressietest scant lib/hub-settings/ als TEKST en sloeg aan op
  mijn eigen comments die de marker-naam noemden. De test heeft gelijk om ruw te
  zijn; de comments zijn herschreven in plaats van de test verzwakt.
- De testcode gebruikt BigInt-literals (5n), maar deze repo target ES2017 en
  `tsc --noEmit` weigert die. Vervangen door BigInt(5), in de tests en in het
  plan, zodat een latere extractie hetzelfde oplevert.
Plan Task 6. /settings/hub rendert de fleet uit de DB via één pure rij-afleiding
(toRowView) plus waits.ts, en ONLINE_WINDOW_MS verhuist naar de DB-vrije module
lib/instance/constants.ts die registry-read.ts en waits.ts allebei importeren.

toRowView bestaat opdat de invariant TOETSBAAR is: veroudering hangt aan
hub_snapshot_at, nooit aan last_seen_at. Met alleen een losse isStale kan een
test die verschuiving niet zien, en dan is het fleet-overzicht een host offline
aan het noemen terwijl de hub-rij zijn knoppen nog aanbiedt.

Knoppen verschijnen alleen op de eigen rij, alleen voor de doorsnede van de
hostlijst en de UI-enum, en alleen als de rij vers is én de setter gereed. Een
afwijkende hostlijst of een false readiness-vlag markeert de rij. sudo_ok null
telt niet mee (op darwin loopt geen sudo), false wel.

toRowView leest de snapshot bewust soepel in plaats van via het strikte
Zod-schema: het strikte schema bewaakt het SCHRIJFpad, en een half ingevulde of
nieuwere snapshot moet nog steeds een leesbare regel opleveren.
Plan Task 7. POST /api/settings/hub/wait plus exec-setter.ts.

exec-setter.ts kan execAgent niet zijn: die gooit alleen bij een niet-2xx status,
en de agent stuurt 200 zodra hij begint te streamen. Daarna gooit execAgent
`event: exit` en `event: error` weg en mengt stdout met stderr in één string --
een mislukte setter zou als succes terugkomen met de foutmelding in de JSON.
Vandaar de SSE-lus die currentEvent bijhoudt.

De route weigert 401 zonder sessie, 403 zonder capability of geldig CSRF-token,
en 500 zodra INSTANCE_SLUG leeg is. Die laatste is geen scherpslijperij:
getHostSlug() valt bewust terug op 'srv', wat prima is voor een leespad maar
fout voor een schrijfpad dat een rij selecteert -- een host zonder slug zou
anders de snapshot van srv overschrijven.

Een body met slug/host/host_slug/command_key wordt geweigerd met 422 en niet
genegeerd: stil op de eigen host landen terwijl de aanroeper een andere host
bedoelde is erger dan weigeren. De commandosleutel is hardgecodeerd; alleen de
waarde komt uit de client.

Volgorde: agent -> sequence -> write, omgekeerd aan de hartslag, omdat de meting
hier aan het EIND van het setterscript zit.
Plan Task 8. scrum4me-hub-reader.ts volgt lib/worker-insights/scrum4me-reader.ts
(lazy pool, max 3, statement_timeout, eigen application_name) met één verschil
dat er toe doet: hij degradeert NIET naar leeg maar geeft {status, rows} terug.
Een lege tabel leest als "er zijn geen toestellen", en dat is iets heel anders
dan "ik kan er niet bij".

De echte queries zijn de probe; er is geen aparte voorcontrole. 42P01, 42501 en
3D000 zijn `unavailable` en geen `error` -- dat is precies het Mac-geval, waar
SCRUM4ME_DATABASE_URL naar ops_dashboard wijst en de hub_*-tabellen dus niet
bestaan. Die configuratie zelf hoort in Task 10 bij JP te liggen, niet hier
stilzwijgend gerepareerd te worden.

Het paneel zegt bij unconfigured/unavailable waaróm er niets staat en wijst naar
een instance die de gegevens wél heeft.

De limit gaat als queryparameter mee in plaats van geïnterpoleerd, en is
begrensd; dat is geen reactie op een gevonden lek maar de gewoonte die voorkomt
dat er ooit één ontstaat.
Plan Task 9. Knoopt de drie bronnen van de toegestane wachttijden aan elkaar.

Leest expliciet deploy/ops-agent/baseline/commands.yml en niet de example: een
build-time test kan alleen in deze tree kijken, en de twee kopieën lopen
uiteen -- de baseline is de enige die byte-identiek aan de live server is.

Deze test slaagde meteen, wat op zichzelf niets bewijst. Daarom apart
gecontroleerd dat hij rood wordt bij drift in elk van de drie bronnen
afzonderlijk (baseline 300->600, declaratie 0 120 300 -> 0 120, UI-enum
300->900); alle drie rood, na herstel weer groen.

Over max2 bewijst hij niets: die whitelist leeft in scrum4me-docker, buiten deze
repo. Die borging komt van setters[knop].allowed in de snapshot.
Gevonden tijdens de uitrol op scrum4me-server. Het script schreef eerst naar
/etc/sudoers.d/ops-agent en draaide `visudo -c` pas daarna -- de vorm die
install-network-module.sh ook heeft. Dat is een val: een syntaxfout in
/etc/sudoers.d/ breekt sudo host-breed, en juist dan is de back-up onbereikbaar,
want niemand krijgt nog root. De uitvoerder ving het op met een droogloop op
kopieën; het script hoort het zelf goed te doen.

Nu: kandidaat in een tempbestand, `visudo -c -f` daarop, en pas bij exit 0 het
echte bestand overschrijven (via cat, zodat mode en eigenaar blijven staan).
Faalt de validatie, dan exit 6 en blijft sudoers byte-identiek.

De test draait het script met een visudo die altijd weigert en eist dat het
sudoers-bestand daarna byte-identiek is. Apart gecontroleerd dat die test rood
wordt op de oude volgorde.
docs(hub-settings): veldkennis uit de uitrol — agent op docker-bridge, fallback-envpad, ts-node-pad is alleen de Mac
Some checks failed
CI / Ops-agent checks (pull_request) Has been cancelled
Details
CI / Deploy artifact checks (pull_request) Has been cancelled
Details
CI / Docker image build (pull_request) Has been cancelled
Details
CI / Root app checks (pull_request) Has been cancelled
Details
b13271f010
s4m-codex-reviewer requested changes 2026-08-20 07:56:37 +02:00
Dismissed
s4m-codex-reviewer left a comment
Copy link

Verdict: REQUEST_CHANGES

Geen gekoppeld plan gevonden — beoordeeld op codekwaliteit + product-standaarden.

Findings

  • ERROR — app/settings/hub/page.tsx:64 — Nieuwe UI-code gebruikt bg-amber-500/10. De actieve productdoc patterns/md3-theme-styling markeert rauwe Tailwind-paletkleuren in nieuwe code als hardstop; gebruik semantische MD3/shadcn tokens in plaats van amber-*.
  • ERROR — app/settings/hub/page.tsx:81 — Nieuwe UI-code gebruikt text-amber-700 dark:text-amber-400 voor drift-waarschuwingen. Dit valt onder dezelfde hardstop tegen rauwe paletkleuren; vervang dit door bestaande semantische status-/destructive/warning-tokenisatie of voeg een passende semantische token toe volgens het thema.
  • ERROR — app/settings/hub/page.tsx:112 — Nieuwe UI-code gebruikt opnieuw text-amber-700 dark:text-amber-400 voor verouderde metingen. Ook dit moet via semantische MD3-tokens lopen.

Opmerkingen

De functionele kern rond lokale host-mutatie, CSRF/capability-guards, monotone snapshot-writes, schema-2 parsing en tests ziet er zorgvuldig opgezet uit. Ik heb geen aanvullende blokkerende functionele bevinding gevonden in de diff. De styling-hardstop is echter expliciet normerend voor nieuwe UI-code, daarom geen approval.

# Verdict: REQUEST_CHANGES Geen gekoppeld plan gevonden — beoordeeld op codekwaliteit + product-standaarden. ## Findings - **ERROR** — `app/settings/hub/page.tsx:64` — Nieuwe UI-code gebruikt `bg-amber-500/10`. De actieve productdoc `patterns/md3-theme-styling` markeert rauwe Tailwind-paletkleuren in nieuwe code als hardstop; gebruik semantische MD3/shadcn tokens in plaats van `amber-*`. - **ERROR** — `app/settings/hub/page.tsx:81` — Nieuwe UI-code gebruikt `text-amber-700 dark:text-amber-400` voor drift-waarschuwingen. Dit valt onder dezelfde hardstop tegen rauwe paletkleuren; vervang dit door bestaande semantische status-/destructive/warning-tokenisatie of voeg een passende semantische token toe volgens het thema. - **ERROR** — `app/settings/hub/page.tsx:112` — Nieuwe UI-code gebruikt opnieuw `text-amber-700 dark:text-amber-400` voor verouderde metingen. Ook dit moet via semantische MD3-tokens lopen. ## Opmerkingen De functionele kern rond lokale host-mutatie, CSRF/capability-guards, monotone snapshot-writes, schema-2 parsing en tests ziet er zorgvuldig opgezet uit. Ik heb geen aanvullende blokkerende functionele bevinding gevonden in de diff. De styling-hardstop is echter expliciet normerend voor nieuwe UI-code, daarom geen approval.
fix(hub-settings): MD3-warning-tokens in plaats van rauwe amber-paletkleuren
All checks were successful
CI / Root app checks (pull_request) Successful in 5m47s
Details
CI / Ops-agent checks (pull_request) Successful in 15s
Details
CI / Deploy artifact checks (pull_request) Successful in 12s
Details
CI / Docker image build (pull_request) Successful in 1m19s
Details
742bd87e65 
Review van s4m-codex-reviewer op PR #143, alle drie de findings terecht.

Ik heb eerst tegengeworpen dat rauwe paletkleuren de bestaande conventie in deze
repo zijn (15+ bestanden, amber-400 22x, amber-500 19x). Dat verweer houdt geen
stand: de productdoc `patterns/md3-theme-styling` (status active, geldt voor dit
product) noemt die ~353 voorkomens met zoveel woorden een bewust buiten scope
gelaten legacy-uitzondering, en stelt een hardstop op rauwe paletkleuren in
NIEUWE code.

Het thema heeft er ook gewoon tokens voor:
  bg-amber-500/10                    -> bg-warning-container/40
  text-amber-700 dark:text-amber-400 -> text-warning   (2x)

Dat is niet alleen conform maar beter: --warning is #735b00 in light en #efc047
in dark, dus de handmatige dark:-varianten kunnen weg -- het token flipt zelf.
Geverifieerd dat de build beide utilities daadwerkelijk in de CSS zet.

De test is fail-closed op app/settings/hub/, zodat een nieuw bestand in die map
er standaard onder valt. Apart gecontroleerd dat hij rood wordt op een rauwe
kleur, met de bestandsnaam en de kleur in de melding.
janpeter commented 2026-08-20 08:02:45 +02:00
Author
Owner
Copy link

Alle drie de findings verwerkt in 742bd87.

Ik heb ze eerst tegen de tree gehouden voordat ik ze toepaste, en mijn eerste tegenwerping bleek fout. Rauwe paletkleuren zijn in deze repo wijdverbreid — 15+ bestanden, amber-400 22x, amber-500 19x — dus mijn aanname was dat het de conventie is. De productdoc patterns/md3-theme-styling (status active, gekoppeld aan dit product) zegt echter het tegendeel: die ~353 voorkomens staan er expliciet als "bewust buiten scope gelaten bij de eerste restyle", met daarnaast een hardstop op rauwe paletkleuren in nieuwe code. Legacy is dus geen precedent. De findings kloppen.

Omgezet naar de bestaande MD3-tokens:

was is
bg-amber-500/10 (regel 64) bg-warning-container/40
text-amber-700 dark:text-amber-400 (regel 81) text-warning
text-amber-700 dark:text-amber-400 (regel 112) text-warning

Dat is niet alleen conform maar strikt beter: --warning is #735b00 in light en #efc047 in dark, dus de handmatige dark:-varianten vervallen — het token flipt zelf. Ik heb geverifieerd dat de build beide utilities daadwerkelijk in de CSS zet, want een @theme inline-token met opacity-modifier is niet vanzelfsprekend.

Toegevoegd: test/hub-settings-styling.test.ts, fail-closed op de hele map app/settings/hub/, zodat een nieuw bestand daar standaard onder de regel valt. De melding noemt bestand en kleur. Apart gecontroleerd dat de test rood wordt zodra er een rauwe kleur in staat.

Gate na de wijziging: tsc --noEmit groen, 73 hub-tests groen, npm run build compileert.

🤖 Generated with Claude Code

Alle drie de findings verwerkt in `742bd87`. Ik heb ze eerst tegen de tree gehouden voordat ik ze toepaste, en mijn eerste tegenwerping bleek fout. Rauwe paletkleuren zijn in deze repo wijdverbreid — 15+ bestanden, `amber-400` 22x, `amber-500` 19x — dus mijn aanname was dat het de conventie is. De productdoc `patterns/md3-theme-styling` (status `active`, gekoppeld aan dit product) zegt echter het tegendeel: die ~353 voorkomens staan er expliciet als *"bewust buiten scope gelaten bij de eerste restyle"*, met daarnaast een hardstop op rauwe paletkleuren in **nieuwe** code. Legacy is dus geen precedent. De findings kloppen. Omgezet naar de bestaande MD3-tokens: | was | is | |---|---| | `bg-amber-500/10` (regel 64) | `bg-warning-container/40` | | `text-amber-700 dark:text-amber-400` (regel 81) | `text-warning` | | `text-amber-700 dark:text-amber-400` (regel 112) | `text-warning` | Dat is niet alleen conform maar strikt beter: `--warning` is `#735b00` in light en `#efc047` in dark, dus de handmatige `dark:`-varianten vervallen — het token flipt zelf. Ik heb geverifieerd dat de build beide utilities daadwerkelijk in de CSS zet, want een `@theme inline`-token met opacity-modifier is niet vanzelfsprekend. Toegevoegd: `test/hub-settings-styling.test.ts`, fail-closed op de hele map `app/settings/hub/`, zodat een nieuw bestand daar standaard onder de regel valt. De melding noemt bestand en kleur. Apart gecontroleerd dat de test rood wordt zodra er een rauwe kleur in staat. Gate na de wijziging: `tsc --noEmit` groen, 73 hub-tests groen, `npm run build` compileert. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
s4m-codex-reviewer approved these changes 2026-08-20 08:06:55 +02:00
s4m-codex-reviewer left a comment
Copy link

Verdict: APPROVED

Geen gekoppeld plan gevonden — beoordeeld op codekwaliteit + product-standaarden.

Findings

Geen blokkerende of error-severity findings gevonden.

De diff volgt de relevante productstandaarden voor lokale agent-boundaries, capability/CSRF-guards, MD3/Tailwind-styling en operationele documentatie. De risicovolle stukken rond shell-side effects, whitelist-drift, monotone snapshot writes en heartbeat-best-effort gedrag zijn met gerichte tests afgedekt.

# Verdict: APPROVED Geen gekoppeld plan gevonden — beoordeeld op codekwaliteit + product-standaarden. ## Findings Geen blokkerende of error-severity findings gevonden. De diff volgt de relevante productstandaarden voor lokale agent-boundaries, capability/CSRF-guards, MD3/Tailwind-styling en operationele documentatie. De risicovolle stukken rond shell-side effects, whitelist-drift, monotone snapshot writes en heartbeat-best-effort gedrag zijn met gerichte tests afgedekt.
Sign in to join this conversation.
Reviewers
No reviewers
s4m-codex-reviewer
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
janpeter/Ops-dashboard!143
No description provided.