From 38f38326dead6d8a3636502b5b76c64b14b9bdf5 Mon Sep 17 00:00:00 2001 From: Alban Mouton Date: Wed, 24 Jun 2026 10:47:37 +0200 Subject: [PATCH 1/4] fix(worker): silence node warnings in task subprocesses Task subprocess stderr is surfaced to processing admins in run logs, where Node warnings (typeless package.json, experimental features, etc.) are never actionable. Replace the targeted --disable-warning flag with --no-warnings. Co-Authored-By: Claude Opus 4.8 (1M context) --- worker/src/worker.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/worker/src/worker.ts b/worker/src/worker.ts index 342bddc..2e2a649 100644 --- a/worker/src/worker.ts +++ b/worker/src/worker.ts @@ -267,7 +267,10 @@ async function iter (run: Run, freeSlot: number) { // Run a task in a dedicated child process for extra resiliency to fatal memory exceptions const child = spawn('node', [ `--max-old-space-size=${config.worker.task.maxHeapMB}`, - '--disable-warning=ExperimentalWarning', + // suppress all Node warnings in task subprocesses: their stderr is surfaced to + // processing admins in run logs, where Node warnings (typeless package.json, + // experimental features, etc.) are never actionable and only add noise + '--no-warnings', './src/task/index.ts', run._id, processing._id ], { env: process.env, From 883c0e3dd09a529a856b6da3c322ce40c2a4744d Mon Sep 17 00:00:00 2001 From: Alban Mouton Date: Wed, 24 Jun 2026 15:08:54 +0200 Subject: [PATCH 2/4] test(fixtures): stop hello-world plugin waiting for finalize-end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit data-fair now indexes REST writes synchronously and no longer emits a `finalize-end` journal event for them (only a compat shim on dataset creation). The hello-world fixture plugin waited for that event after writing a line, so branch-plugin e2e runs hung until timeout. Drop the wait (and the now-unused `ws` binding) and repack the tarball — the data is queryable as soon as the PUT returns. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/fixtures/processing-hello-world.tgz | Bin 4076 -> 4162 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/tests/fixtures/processing-hello-world.tgz b/tests/fixtures/processing-hello-world.tgz index 3fec45c764b450da41270b3aa984ec63184dfd02..f795214b939ad3ffe8305681c86dd90fc65b197d 100644 GIT binary patch literal 4162 zcmV-I5WVjoiwFP!00002|Lr^Na^p6V^P5l6qil`r8H$oTAKBGLNfcX-wXr3aq)9Hd zn>|BhOTr8ZZ~)M@)};ryx47D0>w68q*j0n1M16TCb33!OIrEQ1pwZ}VH2Mo*%>1`( zg4NT`VY@qMKl&6;tyX*W;stDLuhnV?wU;$`^!(+kTD|`2RqX{ls?}=;uMXf*?a^m= z5^b1zRIB|Hn5@-mu=xEqdCodRIORSHjgE6DGgE}pARAHIx03|OLWc{WzPt;rxS7ht zfI@8)_X&p!T=-!U5Dj^E$RnQ3K*KHL+A$ImjjpBBg-8Z`OuuN^ijz^u_0)xc6Y5c7 z&;?D6R)Q|^QI!fbhN0ujn4<>E>ykPPL!`$fP?L#BQ_IyLoEb8mSHHbh&5k|fD=c$;W)!70vpLow7jJ2Dx~3lDsH;~@p4tOEK^tiX<<>9t|Pa27%=m7Q;$P0w+LCv6z?j)yLV$ok7Wk zpE|>n-s>U!)av(J-QmyBJBC*GXZWGhJ#wM_ALsq{U;w?o(>Xgo?X-_v=yVTHUmtb4 z-@|vWhtTZ};k0ws8McpL*aMm{L)B>y2-#V?e|XaB4qM-KPCLV&UFWzn>=Mr7ULRU; z-s%rKhp$gteK>#JKkp6N(CQvRx7Y1-kNchO_wBQGcj!T<3*8>He{6S$FgR(Qp3+oK z>-F%Y*C+bmuy_7*zw`ac5KelhNA3OqzH7s2r}f=wJDsZgGaR0_I%h5%wa!}Kx9zCj z@TA>$2rQ)zemZGW6V2D^LhEqY>2-;X!(MmTZygR@820+Z#n7KRgSHE;erG@eIqvt) zT!+NjJ0_4$m&SG5DHakntgJ|{Z{J@J+6#nm)NY-2y5A3IFtL~ed(NYW=QsJpng7pz zy*FBl&+Gr6Z}I>2mjwUd|3Ce6;{cQdi?Gyy(zgLKtTJX?RbwT6)S8P)Wr|@aD_2s5 zLCGcXOH|}BXhhvR@D9?}0JX1pY_gs+E_g%^B1|S+z*s6+V6QB(YYz?;GJ_xmRzfb& zf@w4wq<{b;CIT;=A!6L3<|4pr&*-$(%wkJ?B!eWxv@NAXml{C)0hB^63<060e~==~ z+ACvzUFfUO`x0W5@Gz**B-#R9lgEQ@@2LHsy>l|HgJ!u>!Rr_mw;@Ydq0k^shm_&T z69~qY0v7P9Ic|g6R7Ol&`YpF|1m?E7-qKG3H%qIAe_`Xocf(Y|HdU=E~iU5Vru|V&)zKkMnXaiLJkked$t^dLM zy;si{YXw4zF`tmCU7{d;Vd1tBOVUC|Yk3ZGT3$?SAe(_g=1b)1I@P2M1pLrC0wl?_V2NKgc@*93^S{3-85py3~@` z_CI(Bua+ItM8BP7?R9T~U!bIbI$$cuIISNnC1Z)R2F-T9^nUL>U#f>qj+R=V>USku zA?;5wV%DkW*b0K(4djtft52U+pW2<`%pc;v-+KJFOK(1T{8u}uZ^VBu>Mx!@#DAYH z{>y!{v}J|VRm4ihsLYC&NR{LawNCxf&2lKkCSp=@p=7uTZPO8sSrQtEGI*ElrEQ|# z6LL`{n3ExUurL4m{ePcZ_@w*)%i61#>-+y@?M3Zj|Nl$(f5>K(0W_dw*eFZ26uwKi z!eBMX)@aTsa>`@e(&7ei800tGwqCrltf2&52qx2Q8bP_!2LptTtrQ1Wf^ zxEQkpSQ!OS;x@D@02WEG#t3Y@45bCcJo`6aP$UHwSShlzitkX}-J*g-z?&V+S@;ck zZZJ`3{bgn77W2GAM5~>!X|mV*eJRp(Z$q~0Y_@D|y|mIlceitL4X!G=}J#CJ$Uo^pnLXc_mOC^U$E0KT+(e zX;iXfbG{oOQ~vavDHhSp@3w<+BD#+{32kAqq_V#|frq&m>BUQhx)b;ZoGn0k@J})f z*}q?$!YXb0-$N3aNS1zghhi^0`uI#tXe6+kj-{Y%Z0%S zsvYwl-Q9+a0}9Ycr=xU`oERQ*eRm5nnL{dJqJpqYh?Gif1)Fxhx%<06#r<6**SC_& zCqgP57#Z)PcO+$qOzfbyW5=iDXrmH8r$!qYe;DZ_l^7P{2e2?^p~kIjn5mLiZKZ5! z^ds0fy1Vs56#mpkK?tZ6CTbV`3?@{2wv6fwR(}N58hwT4m}3}xl#(7!SrkKxWqtOQ z!;Mo0mni?v}LAR?E?VC==8h7BQ|tp}3T@ zyIXS2VL6ozGngn3Iw8cE7`Xc%a^0(9(?JN$vQH2OX-9W$+|uD|JhSz@Fwqc}ukAq8Gl7SO-KIL_}kj%_Oazuz>X1>2!> zTSAR{Bc-vD(FcS`_eMZQ{%Q;#BBrz&{7^Re&DDSU?bgv*+lzwFe*dptd%jWssl9yu z>Y@JgIi4@zeFun+bKg;Z_17NdJI)thKp#V7bmQ=RF*?Jpf$qJH(fD-bjVz(PR%TD0 zaYDE5X7&OfFuE2<(SsC_9sa)O%-4f~%y00ecM&s&xCW*{b8;s1uK<(Joo$nb<6K-^ zIMk9|f`Et%sccI&%$d-wzC=&y=zsCQQZa0z?PzDXAOo0W7ac==%eKgYu(d!LB{L%4yZIX+D5C@!2UjG4mYM5L+cJT=PYdv15E_=GpQ?A$ZS zm`@fcz3Te#6kws8Wyi5)z}&A4>=6|-;m?T>t5Z$V30@E|$!(LXyYHTjYSL?F-dLNo!+c-bP zbCb=T_PVexN;k`Hb+M}xxWKNt)GmaQ1~KDLm+&GuV?2a8G$EtR0bo~*8v^&V!FaEH zbaxvurAUrJQphvQ`zyeduI`mHZeS0ujbhZX-zd8|_I<|z;A7ZZC*HGlp#3bZS*M^C z$07Gwo{$E%@MEXnAmD--gCc<6-E5y?o)?Su^!`PPfr6erQVI@DsQnQ*d46|H8(HSQ_YGQ31uIE`k=Bb15BTV1HvYPJ3SY&3}>SHmYy;PJ+k%X-s( zA>C7G5+w?3Uok~d#9me@l2M!x`4lWrF@UawF<~J5OWBIXV0nKd@tf~jmSwRhr@^*Z zrfu_HIjjE!>5akSe0Y6#>nHiDttrlYrz+Tln?$m`$20PtPBJ&U)Tk{kC5PZIQOvOGCAvay@s94Py`qXFgOS3tfY|&vd?(dy-8v6+!m!X$PM>5UZKhf-+Z%)YOTM#&p~l=9yRZ@=2_D{pQ5wKO~U@Z zrFyG@U6mR5>hAXL)&(9kOTW9xdF8>p7r=53odEz1`5mBfO2{uSa$Pf zqL&8?mOLJ~x!6~xfH};q5&?!9KTvV9$#yBuk9W6JarHk=c_wJ~F4lCXy3I)~1$a?h z1!CcG*15l7gfBu|Cx^<>*IXNfnCX0Z?wf9PRhH|$ff`c0AGPGx>21;Wb*H)%`x)2E z^X(wX_R7MCt!z-zq-|0z3e_$6(ruftNLB79_Y014CFU_qzuhYj?NJP{OY~_sO%&SC z?$+X?yk|GO3pE=fEXVzd*y13MEw+U-^;&HucKMjdWq(~gK7=6-&!E0wYE63S0^j|Y zueeE+jV`kwpR{enk?x=;Sde43&WU-?Dp#pQrK`5owOH{CcVi`v_Es)F!ZRpax3{rT zvcXQK4eK7W4k4ciEbfxKTjDFFzFLKTdoToBIlH3fv>0Zy9}Iw!gQ=22COVu^O0mem zT+r#4pwvPijap>y5>zvr<4|==NrC5{VU5-EbxIjT)E4N>AxZJkeQ zdt8=IgfeS1r9%7imuV=6fMEDVVwipyP)#OW2;ADj85DgjqK{fL&n z&*7tR28KSg74&g)KHEewAL#>JE&~|hm;xmGInM$%`_}rPmZc+lGjFt~!i&T)O$G~j zD|vd_P%C5NG^1~sT8dWwFSWpun9YXH#4c44x`Pj`?vfLs|57(g|o`>h*`CQNc M0{-8~uK-2>09W`O0{{R3 literal 4076 zcmV`E>KOm z)?5nU8YZZ4ID@fb!eHRSNFjn8fj?ntj4l`nOw1rcrKJElG>i)_#=yXr(aez}FcYpp z%aOTa3W13LnAXzg%wPb4^y4WC!wk_I@enl}mc1E!hTu;!B2_$G~ zDZWGBPz!;NWEjdT63v$);3Sc*JI-K&z=rY?tt`pB3Te2XsM}6Jw3^klOHWuB!Vpu1 zVE|kJ%Za0iyf#c2&O(T!vh+5^^c-h!+Jb)fWbnG#YeBmY7rpM!?c>%l6q_Y3`E_$tgAG$rKeSUG)ZXLVO?i`)HI&ODf z!Vj+o(CH4~tbN`dw2ooW1tOTDYPb4??7Y=GI&F3a%^%ul?ZGdubJ8Ak2{ni>%7$&c+l=Zrwgs0Tb%*)Pn&0FM9OKt z8k}}}#2*}WFMjE@U!D%&w0m~k>h9B zR?i`@ggbbB+M*_+*X%&^XwdFdxPcB*X@4Gg=VkaCxx8!y63J# z>g=8nNV`MhI;{i?sT$U1q}#LaullVeLO5gqr?Af-r3_ zkN9=2uT1YNh+)jbpiD%x1-hn)2c7P5>)*Q<~^fw#J0cyjAO^6!W1Tr>xK8ZyUO+m#1mV77wxT!p*>2Lt%R{exhb0gJk zH3Qi+jrkI7tirVA$;6mQ*DIAVH-hE?LpEujK?{9OGWzx zu_|2a7<0;;$SFo_jGu__D}^kWVzx#vk1;|Kpzt~7`2F6O(vO*FnA|)eKW-Ur2Mski?C-LZJHI#A_F)p}JFx-T;=@>^W4h>9Gc$fU8ZKB>& z3Q+}^lOwzLFaP}Ye^*%etn2^l>Wk-_>;HN6S@nMX|5Mk0$Y!(ys6)Z9VY<~)_-o7+ z2J1<-MsrS?Q<^(u%YBt%hhcZL9Fn*a$oYvMF@^vkkU}hOQKxty?=7-H8x-qM@a^Vt zIc5d0HVUA?ZEBSR%(Gyf5!ieg3QLH2`ftAEND9n}k{4$k-;r9}qJmh!yFJ2L_$_^I z36X1kvo>^xd)}j>^-kD!*_-{o6v=jPOShY1Hf?Rau+~3!cS~{yBRR8M>))%M92Xny zZ?=%!p{O-sw!1#8s&#D)xKX$pW;5<>ncAkw?S9X{t^VgUzS|nl(HMoW_)mBz<9~Up zjH7_8@^cy82i?*~TbA`W|97)$2eii?aVE?TdvQO%MSs{xw^XwX9y`=BM$QtYZ> zRJ><#{@O>T{K*AVY)X`W-3`Wx=^pAhw1vfjO8@Q!9%O2y7cWujUf`c_wglzDKgcnp z|K2#cS=x@jN2D?luj1|=!(K-AjmUb3N#%V|x(RT0yT!yIJ}m6=YJ~tpHrq#*3xi`+ zd*~i7ZbQZa1!$zxQ94MD4G+0q+(Ja|kV=@SAS@ChrD9vbrj>6N|K(4xw2$T5PFDF? zNQHePqka4ir3{gYJ^Z%q_=Fv8RP1NmXd|PKV|}P%!$SNB5k@T3xKj)>QSz##lr4>Z zf*8k(TR%kMPiz*1fJ$Mk_VLePOtoh#s7_(^CvdINS7=T+hQTKp>CuEuBS^4pQeQjV zIAd_hLXs#z@R6=POe3I5yJqnvj+qJ;x8P6sXrvamWPP?;js`@Lpx&`5;~Er-OF3KI zQfLl~iEfy|Sb5M6A;Q?e;(sV~ud7WP>Ay#1mBqzKswqp(;57=(haqYYE(q`vmEt3g z3Vw|dLIk52c^DK17zgJ2i~rV^_);Dr#gcDy0mBfjTx>Mr{$z0r0TY_wa~gpl-pnYx zb@lCu2iDW*yP|Z&w5GcLcJyVjk-nTpAsP!Q@T$Co{teD?ez$XMQR)A~t`RNR9-G^d zYTTJAjg*{zM2d8024v)K#_%y}N~*#4Ws~1s{ioMz9-p_oY4FAG|JABbx9UID=TBeU z*MGjm^A&vP0MT*oI?Av7)}wsK`RXg^VTg=w99}F(r`R>ny|)n>pRT--C$!hf?8!5Z z>DJxMUf=^p*8(YfkOI=f-w&MmdeE2I4Zh?qVk!{Vz%*!1!G!+hU^3e2GO0Vx+qZ8W zYDq6az?2KAY)d-K8Pl!4SWoEafBC;$Hf*fzXlJlw1DMT$GGs1YJ%mW1O7xSpXSWuC zrDyS$C&R`&_=yx+mY`gY6!M@kM8op@C9K$61|MSe-aY=m@&DPKng?In|JSNdsc*yo zfBmd>@BhEV!>9H>6D5Xl11ooYl+;mNxKJ1|g(tB{wxaXIDHrd!-LV!E-sz%q2aypU zFHw4x&EW~a(m0EbW6OY9TcCfXG7W&NtiOh#&qF%wTX5@GHo`3atz zly=hV!lo+Sba$)sUG2aHcFm=BA(S+T8Gn+&Uuha6f^)`0m_q|n?i>Ji#kip%o;DaA z6pt6TQ>GN@GKh0~&0=W{n6|73#Z)3Vz-yxzb(HEwH^W|X8~{FogG~WETMH_sTboS| zno$&TpJkiUz!rZj`7H+CGGkB#@Q0h-B<5Mc=s=g=CKxF3IUvK}(17Znfx|bjd+?uu zKY3DG?!OY!0Mi~#b z#R_ek4~l6GC`fJ%=IP<};?|F|S=%t2`Ce17n{HCc?jFy`J37hb>@%a5xRe}%zZPQw zmPX2Rs*q%<*^b6i;e@Y#7qM)EZe)Ew!x>Qeg-0EV`SYUHFnCy8;1exdI7miYFX}~7n}g!UQAN>Rnu;wh?J$BAG&w5B zrZ87$t7p#pJ1D&^{AS4TKBB!u1`K-WGf<-rL zCVqLa6v^X(o2h+`1k7P>jR-K*_>qQ_65D4uKQC^n>gs=*@r=L`-MXa~HYwSy|2Z7HUZFe$tfNptr-?H-qX*?dM#t z&bWg(T`S8NwzfdYl6Gl1uT(b|OLr~8JXcwqEad{_TFzsbet%FL*y9*rm*~@K8Y{G) z-fiSZS zSKP$PW|wKAPu4c%NcYfV%;_0l?dhRujsgOHB}=2yw$mgLHFf2BS8J(22187slUgS;Orj<{eU|2vjZ1jUjwUv5lZ z>EtufT^ky^Z_ym4XeKhC?Te7~iEMJ$1BS5yZfq$G#!^zQ!F>BcD1EA`kS_YuC!vzQ z{1vn}qCy|jFpd3*Et*Mf&z=%OXm>OzEj^KO7{G|rhphCbOgIBJqUxQ`!jSSAN^o6} zW`gR97P8$&CAx(xo_vu?AG`2jI0HjptqS^5IHNXJ%t!i~mCFEzIHF9$ejBrdO+Q>d ztfqOr-rj`SBi7|EEZJG*Hh~l}$uif*M9KEJzPV4{-l?rH-|XL(!F*#wmk;EPJL7zo zCg(>ydbEb%Aw{pp>9dSqJNy|mVAH$ZnPTM8u1C{d$TzsMF?GpJfOMj_$H`j1kZx>l ecCzK>MY_LReE-}(_s{*a_xWFNu?0T>MgRcR^8!Br From 6318ee1fa6ac6b5759e54a7c00c7f0cf719dad15 Mon Sep 17 00:00:00 2001 From: Alban Mouton Date: Wed, 24 Jun 2026 17:30:02 +0200 Subject: [PATCH 3/4] test(e2e): dismiss simple-directory admin-mode prompt for superadmin simple-directory now shows superadmin accounts a "Mode administration" interstitial at login, asking whether to enable admin mode for the session. The login fixture didn't handle it, so the post-login redirect never fired and every superadmin e2e login timed out. e2e specs never request admin mode (goToWithAuth takes no adminMode flag), so choose a normal session to let the redirect proceed. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/fixtures/login.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/fixtures/login.ts b/tests/fixtures/login.ts index e399e7d..b94c9be 100644 --- a/tests/fixtures/login.ts +++ b/tests/fixtures/login.ts @@ -10,6 +10,13 @@ async function performLogin (page: any, context: any, baseUrl: string, url: stri await page.getByLabel('Adresse mail').fill(`${user}@test.com`) await page.getByLabel('Mot de passe').fill(password) await page.getByRole('button', { name: 'Se connecter' }).click() + // Superadmin logins hit a simple-directory "Mode administration" interstitial + // asking whether to enable admin mode for the session. e2e specs never request + // admin mode (goToWithAuth takes no adminMode flag), so choose a normal session + // to let the redirect to fullUrl proceed. + if (user === 'test_superadmin') { + await page.getByRole('button', { name: 'Session Normale' }).click() + } await page.waitForURL(fullUrl, { timeout: 10000 }) const cookies = await context.cookies() cookieCache.set(user, cookies) From a17e514274442af1f3db410851f56cbf01e8c147 Mon Sep 17 00:00:00 2001 From: Alban Mouton Date: Thu, 25 Jun 2026 11:15:50 +0200 Subject: [PATCH 4/4] chore: upgrade pd-ready skill --- .agents/skills/pr-ready/SKILL.md | 59 ++++++++++++++++---------------- skills-lock.json | 2 +- 2 files changed, 30 insertions(+), 31 deletions(-) diff --git a/.agents/skills/pr-ready/SKILL.md b/.agents/skills/pr-ready/SKILL.md index 5482e8e..378939f 100644 --- a/.agents/skills/pr-ready/SKILL.md +++ b/.agents/skills/pr-ready/SKILL.md @@ -1,12 +1,12 @@ --- name: pr-ready -description: Pre-PR flight check on the current branch. Reviews the diff against stated intent, flags scope creep, regression risks, and commit hygiene problems, and drafts a compact PR title (conventional-commit style) and description. Requires a clean working tree. Does not run tests, lint, or type-check. +description: Pre-PR flight check on the current branch. Re-anchors on the intended change and reviews the diff against it at a macro level — scope, completeness, drift — flags risky or sensitive changes for the reviewer, and drafts a compact PR title (conventional-commit style) and description. Requires a clean working tree. Does not run tests, lint, or type-check, and is not a substitute for line-level code review. disable-model-invocation: true --- # PR Pre-Flight Reviewer -A focused review pass before opening a pull request. The goal is a PR that is easy on the reviewer: tight scope, no bloat, surprises called out up front. +A focused macro pass before opening a pull request, run *after* ordinary code review. It assumes the lines are already reviewed; its job is to re-anchor on what the change was *meant* to do and check the diff against that — catching a session that drifted, even when the code it produced looks fine. Three outputs: a faithful **summary** (the PR title and description are the source of truth for release notes), a **scope check** (is the intent fully and proportionately delivered, no drift?), and a **look-forward** pass (what should the reviewer focus on; what could bite users or maintainers?). ## When to use @@ -59,39 +59,35 @@ git diff --stat "$base..HEAD" # file-level overview git diff "$base..HEAD" # full content diff ``` -`--no-merges` drops commits like `Merge branch 'main'` that only pulled the default branch back into this one — they aren't this branch's own work and shouldn't be analyzed under commit hygiene or intent. The diff itself is already scoped via the merge-base, so changes brought in by those merges don't appear in `git diff "$base..HEAD"` either; only edits actually made on this branch (including any conflict resolutions in the merge commit) show up. +`--no-merges` drops `Merge branch 'main'`-style commits that only pulled the default branch back in — not this branch's own work. The merge-base already scopes the diff to this branch's edits (including conflict resolutions). Since merges are squashed here, don't review commits for hygiene — the commit list only helps infer intent and scope. Read the full diff. For larger changes, use `--stat` first to plan where to look closest. -## Phase 4 — Analyze against five vectors +## Phase 4 — Analyze -For each vector, note specific findings with `file:line` references when applicable. +Re-read the locked intent, then judge the diff against it at a **macro level** — not a second line-level review (that's already done), but a check on whether this is still the right change, whole and no more. Note `file:line` for concrete, locatable items. -1. **Intent alignment** — Does every hunk serve the stated intent? Flag scope creep, accidental edits, refactors that snuck in, drive-by formatting. -2. **Code smells / bloat** — `console.log`, `print`, leftover `TODO`/`FIXME` from this change, commented-out code, debug branches, unused imports, over-verbose logic, unnecessary new abstractions. -3. **Artifacts** — Files that shouldn't be in a PR: editor configs, local env files, build outputs, generated files that should be gitignored, lockfile changes unrelated to dependency edits. -4. **Regression risks** — Behavior changes in shared code, removed or renamed exports, signature changes on public APIs, changes to defaults, error-handling changes, anything subtle that could break callers. Each item must be specific enough that the reviewer and the developer can decide whether it's intentional. -5. **Commit hygiene** — Fix-up / "wip" / vague-message commits, commits that should be squashed, messages that don't match what the commit actually changes. If the chain is messy, suggest a concrete squash/rewrite plan. +1. **Scope** — Does every hunk serve the intent? Flag scope creep, accidental edits, refactors that snuck in, drive-by formatting. A diff that doesn't fit a single intent is a signal the PR is doing too much. +2. **Completeness & proportion** — Measured against the intent, not as standalone code quality: + - *Under-delivered* — is the intent actually finished, or left half-built or stubbed? + - *Over-delivered / drifted* — did it grow past the intent (gold-plating, speculative generality, unrelated refactors)? + - *Companions* — does the change carry what makes it complete for this intent: tests for the new behavior, and docs/types/comments that describe it? +3. **Loose ends & artifacts** — Diff-local things that shouldn't ship: leftover `console.log`/debug branches/commented-out code/stray `TODO` from this change, unused imports; and files that don't belong (editor configs, local env files, build outputs, unrelated lockfile churn). +4. **Risks / heads-up** — What should the reviewer look at hardest, and what could bite users or maintainers later? Behavior changes in shared code, removed or renamed exports, signature or default changes on public APIs, error-handling changes, data migrations — anything subtle that could break callers or surprise users. ## Phase 5 — Report -Output a single compact report in this order. If a section has nothing to say, write "No issues" rather than omitting it. +Output a single compact report in this order. ``` ## Summary - + -## Intent check - +## Scope & completeness + -## Cleanup -- - -## Regression flags -- - -## Commit hygiene -- +## Risks / heads-up +- ## Suggested PR title @@ -100,6 +96,8 @@ Output a single compact report in this order. If a section has nothing to say, w ``` +Be honest about emptiness: "Scope: aligned and complete" or "Low risk — isolated change" is a complete section. A short, true section beats a padded one; never invent findings to make a section look substantial. + ### Suggested PR title: format Follow Conventional Commits. Pick the prefix that best matches the dominant change in the diff: @@ -119,21 +117,22 @@ Optional scope in parentheses when the repo already uses it (look at recent `git Style: - Imperative mood, lowercase after the colon, no trailing period. - Aim for ≤ 72 characters so it doesn't truncate in GitHub. -- If the branch already has a single, well-formed conventional commit, reuse its subject as the title rather than inventing a new one. -- If the changes don't fit a single prefix, that's a signal the PR is doing too much — flag it under "Intent check" rather than papering over it with `chore:`. +- If the changes don't fit a single prefix, that's a signal the PR is doing too much — flag it under "Scope & completeness" rather than papering over it with `chore:`. ### Suggested PR description: format -The PR description is the most important output. Reviewers read it first; if it's good, the rest of the review goes faster. +The most important output: reviewers read it first, and it becomes the release note. Required content: - **What changed**, in plain terms (one sentence to a few bullets, scaled to the change). - **Why** — the intent, in the user's own words from Phase 2. -- **Regression risks** — the items from Phase 4 vector 4, restated for the reviewer. This is the most critical section; it lets the reviewer focus their attention where it matters. + +Optional: +- **Risks / heads-up** — include only when there's something the reviewer or a future maintainer should know (the items from Phase 4 vector 4). Omit the section entirely for low-risk changes rather than writing "none". Length guidance: as compact as the change allows. A typo fix is one sentence. A multi-part feature is a short summary plus a tight bullet list. **No filler, no test-plan boilerplate, no marketing tone.** If you find yourself padding, stop. -Line breaks: **no superfluous line breaks.** Only insert a blank line where it marks a genuine section delimitation (e.g. between the summary and a `**Why:**` block, or before a bullet list). Do not separate every sentence with a blank line, do not pad between bullets, and do not add leading or trailing blank lines. +Line breaks: **no superfluous blank lines.** Insert one only at a genuine section break (before a `**Why:**` block or a bullet list) — not between every sentence, not as padding, not leading or trailing. Example, for a small feature: @@ -144,8 +143,7 @@ Add `--dry-run` flag to the import command so users can preview changes without **Why:** requested by ops to validate large imports before committing. -**Regression risks:** -- Default behavior unchanged, but the new flag short-circuits before the DB transaction — double-check no downstream code assumed the transaction always runs. +**Heads-up:** the new flag short-circuits before the DB transaction — double-check no downstream code assumed the transaction always runs. ``` ### Persist to files @@ -174,4 +172,5 @@ Then tell the user, in one short message, the **resolved** file paths and the pl - Tree not clean → stop, do not review. - Intent unclear after inference → ask, do not guess silently. - Long list of "nice to have" cleanup → keep it tight; only call out things that actually matter for this PR. -- Regression section empty on a non-trivial diff → re-check. Most non-trivial diffs have at least one risk worth flagging. +- Manufacturing findings to make a section look substantial → stop. "Scope: aligned" and "Low risk" are valid results. +- Sliding into line-level code review → stop. That pass is already done; stay at the scope-and-intent altitude. diff --git a/skills-lock.json b/skills-lock.json index 802734e..fe11aea 100644 --- a/skills-lock.json +++ b/skills-lock.json @@ -5,7 +5,7 @@ "source": "data-fair/lib", "sourceType": "github", "skillPath": "skills/pr-ready/SKILL.md", - "computedHash": "88ee2f1b52748972924a4c8755be1be39916005a08cf97e6aa3b3076d38e27b3" + "computedHash": "dcf190b1d7fb792318b50fa4515a912fdaee01822c6ef158319f8866f5b97655" }, "upgrade-scripts": { "source": "data-fair/lib",