9p+|B
zs9VbaMRW3n?!>uyy>*hgYN>V*Gtta(h~M3W5C5K?wNYZJS5~*)L>&*U_056`fAM2s
zuw2|Tm(OitrhE2c=xyKni~{$8dSk23mRBF+5vldY)Vw4wcArG8Qw37r&*fFW)KS$L12QLw6D
z8t*H-Esij2R=&4#L3-i3)^tlnC){{^Gza3TKSRPUcAKz_d-1+(N8(DTkZ`8u%I!*R
zl8`bP)5V{C-Ld<&pC}{GC%_c&mwT_JFjuH+7OTd)61&$}FI#aDwNt9ccdtzZjP>7|
z`^uoW!fi_k2?S4qg%B)Qa0mf{LkOsXAYu^U2<8t-bbc&X2_a_BrJ|pEy9MYTZc^LfuI2kHloL
zXSfi8rYI>6*yC1W^v4L^tt7oSv-?&m4hZbDX^E(hA;)(lXtw;Nx(vc-1;?>tC-1X^bwET2rz5QXbB%s4pXdv`R0lD^!^3#0
zBCIotwe*-2sVs~v{AcSPz_#%3${3@YZoh`Of{g)G0tY4Ls%;IB%O#_({zrd-dYHrg
zS22M490%ZWpUvPS!@457V^Xj#Lo7;V~3=lLz&=+1fJ@;fUx?N@2^(S|js58FA2SHtQnkmpT54
zg0g@yEa~UrMdc|Sb*ChsLHpw5IZ!!?vWUL?A5Lao_Pq%FhK6C_Jj{J|q^_yYZPbQx
zlZ=WY+I$TkWQX4DGT&T=KS-p(_wx6ZkEupF6oi}(YGw@F7hRRDQA8iTk(rp%G*n2U
zViv^;`TE`~>TF5rsm?%RK;+$3w?02)X1qM;DUq_{9
z+}4!&vNAGa{>DZ5N=~asyG(1$qSz%>zXJ(OBVTq0iVwDVXbowNY0X$Dy2R=s?zn)i
zzimo-PT|mj*cqq=*q95p20jNmt=R^gPTQn{Zn`6KRvJarFyL{dX=HcOho`(~qF*!o
zU6YRzTh7LVH|qo#>V2<+lLuN!u2EZcCHZ8{w8EbC=RKbL(pxDiLUmC|th$~||7Cp*
z|50dv9G%?ua*fSQrTy1ixI@0rF)A5icz$-?p(;~+;5KmV=zc#N*Fg@H;Cr-H1hP+u
zAK(sp=ZcQHQ)$yU-rk+Uw2OZfcH=T&jyj*Lb&rfFPP5wI|Fyh8k~uW9Cp3{{-Jyey
zLvSnK#PbLM|E#u4Yr$COr5^>RSLo5AGqd--g{d{@^j4&P%X{BW!K~k#(dHq-LddzIF3eg5kUg^
z#JsJK_P6r#@&u3D&~{I6|E4?SgEUKY=nz1y!QCeC=?}KIpQcBT9)K|elM`-lMXvtB
z=RqR}hI2~96>mytyO;08DxCrlsN@T-!{^tKWWTRFP@Idi?M*WC80*e1^pP{ORdlMy
zhJM#(pLQ&DoCwK=>N}X4@)!r^RaIZwR10+O_>)>KeeQFgwVZ9F@Y##dl{wFzqx$xv
z@M|xU%QZ$hh8wZq3@Zc)-yL_pr}ew20#{hBOvK%vbG5`cWfMi&!&MpPGvKuosNT}8
zQpqh{(|qtlzn}h!MG5^S6eo
z%dK~}bxftUy=Y9dNd9}C%Wjv}GK#zF-4MSE3i>L6>Mq=J#Y~n{HQhi`?7nD?6dO~J?RP#3IVM>OE6AfU!PHh
z@k-mb{{GL+l>x28;`lPRHY~MWHw6)35YjWCxE2!yMMz5LQ%ZM|7B(;GqQ1WV
z727kEQ4A`^+l_c7t)Woy<7ZCj-H<|{EGAs~H+gc|&vkgZJoouYX*rqWE`(ob8T7
zC`;`Yr|orM{w8-C
zR#>T1<;wV?)(pj8IyAjo$ZcQi#{C+;2`nZ$J3ZGabzo+ki6GZSYHMqE<6T`{Tdx16
zhXCrozWmzrp|DVc@oWI-zuV!HVd;+-@14}&>C`^`W;b*Y^S89mO3zSFkNLjFt-V$q
z*lkg$WY^3c<5ec0an^EVFu_xKU*8r+($tM;p$jc1r%F}f`A5UH9*yRP@%B|TQfXDq
z*k?^~X=uKpb)Euvf+d2{)_^uLcWz#Th(!DkDp1rs(QCIBuOvL%>{(^C1?(h-=c*iGmA6n;paiq1iObsbh#Pr~_t;s(()`20E*8%~f5Q|%r_9_Te`TxMv{>uB
zvytyte)5eb{Oxe6ytPI&lZ&$J|7M^Q5v9&eb_ly}Y%?))
zGnooovo`8epiuqo8eXH|FcPKhN5Ye#9NmKM{s{t%7LB&f?8oM@j{4qN6>{92R+^gY
zsNGFe*RLK6z~uF^-GlML!XYivx7@RduvDSDJ};}IfSTQF3_<7jxx!nkA>D=fdZ*yD
z+6L8_$Du{$up2SiyErqAk)UCo>z)3td=0Z*w)R
zck(tRKDFlA^cj3vvJl~!xdKhbriNXn8p3m}vY%`gp9En0YE=stMKtn;+>Fr$at+9d
zW>ZMUKpGPn#bVKq(tN-qqJaHrUHPXVrF4OMI=UPgIT`u}6Rl~TjPHZnv|wdg?Jrj@
z)Zh6@J|0Y9q*a;gx~iUbX*q57)QGx{{+Se?>-U((O3>5fE-6t;9?)nhB%U4!4iO>u
zki6=9jN(4V;OC#~CFvw6|M`BS~qH#~-xD6q-R*MlAP6P`>{NLMsmkDAd-%@8<
zSst^zlao9N*;m3Z2I1$ljre=NjZ|mLfAsp>c8Ry3e#P}&PmZs&F#YCUZ{8p06rUm|
z?7&B)y()Q$<_;o@FhjF=R75-TU3-FXQ7vNueO?RkBWSDC(|jAV>
zA};v3=26D5_QdK2dK=wYEVvmP*7w>WX=#4!r753tbEoUTPywf_#%@4ORh0{PX4c}a
z+5qnq>?Ip3(OJMGpUyP|+FcE;jD+dahY_DvruYgG7ZYuzF(uzm>Qu#q;n&qbx37)7
zc3(1+Wj;|Vtb*`w387Gph0bsJQv%@xLRVRNbZLp=&FnDmviyCfpl4iAuxVMEsmB(R
z9Z6~)e%VTue;P$>;es@m$A%a&Nb&;nbw4!$gMvt9YxR_z!IMo>_0#_T=cAM{P9?rK
z5>A405bh&d8U59<-S%mX!Sj_JR)KD*fSRPRjbe4#=Ua*P*xIvbEbr&>%4PK(uUxUp$Y5`h2AznZ}*@`8%B*#0^pc{-Yy;)0LOAtx%9GQ@>(tqifF`-E&Q)Yg@;}G
z6Iw^(1BLyg;GuM1l-Iu-h}QlMSO33MM*cs$)7ME{8br2lh6=yp-NU4JfNDYt|yukXWjIV2P
zjULUtyNkABc447-d5=%I**cwCVkY*yl5Xhx{1>+*m!(*mP6pXR$f;Z9>^L<7Q^$+n
zq#&V&?({X^SlI3^(VOQP+(N%hbIh}UmK6o2nV90bSSG)6;S$p
zV%+XKo{cNwyDtoxS~C=E`yOtNh%IG{9j6Vsd4pzfcxh0sQPf%X>NM9n3)yzl^OGoz
zQpjq}rLbKfy~%6Fd0U
zQ%b9Zx2(4^Y`s*7wO4CMzB8VYO);kUY)7spz(BCWDPPVyIdA)mo!wgpH>B!>tAxES
zor9S|vcv?^wio>s-Z4VkK%${30gHMQn!~Dl`q1QtBdW(IEnGh5-!urOI
zvjAPiO-6fxGWT!?EAC2Xpe|u&fmlB0(Wt|ZqOZK@P5B`*IUn8?tc@O=mm|@d+@*n5T4w
z_BMy_YOm@Wd}m+BOuDqc?oQeC3oM=u(@l9jg-%(AJO5~dT|>Y9?(wuoRuW0!t)y<1
zy;(>^_HT?=sja_V43Mfet|OaVB{iSlkEUc)P!xOHp`x6Tt=+0)&KB9))$G5sT#}h^
zq(@6!fI9!SaMldI^2iJK+*pJ}$eUV$W@0m>8qK}muuD6K+uo{|1Flv3Mcjl|cY5Ni
zD!-W;#QID)3v@+jW5)jJJMzZQ8dD)d3YH%W#ZAj}EvEEL!{z10euu1I&8<_g}>4`U&jh#2XxM
zAIK))=hq#42C}2cIi;ajtNpSqR;Wq%Thr6CLA>hCF06}c0C`{#_yS7i6(-eY+`T=P
zYccZEbC`Rkd}*c3{_IK;r{LZ)kdEgB(J!#bg3VB*PguO%=%~pYj)ok-0;SNZO#x0M
z%$;<5W2E+7nj8<-Zk&E4w5+cymfzdIIrM6Z7pwEUS`9gBy|th7SdZj2@YXYi4UeeO)zENuoCZ=E$|y_JAP>d>x$;BNu>Ie
z*@j@#{7xb|$Bh)2IT%O^X6ywrTHLST$u^_GBz+x-=<
z(8l~S^5P08jLhIqOii7go6|8gR1#1E3nThyqDMzQ=Sm`Wlt6P0&J26RhW9r!_~iU&
z`&F>C3s@@G*6s2=ves_??bh0w@tn9s)pi2|^E$Eu#0@Y}-7t{?5%55hIOr^tu@0Z~
ztY$(#*+5{rzF-qANt$+=>8M01T}zC~j*xBe=#S24#zp=O;ymx9ilH|S3Qr23)C~>l
zCJaAXW_uOfs^TZ5`IcALarN^7j~x4uMM7G*a^pz5bbrFfafa`OGo_KbsZ3l6I)zwJ
z(mbqROT-Pb!CWk(o)S(OpC};0AP88t@DCj&5sW=wuKP&5!atx))asXIB)xbKfd1ET^2@@<715uYzY$M?DhG0Xmr|aEVoEIb5$-jPv%NC_a@u9_`r50zbv^oiVmw=I_Auj
z8)q$czCzgugg+%BjTJkCXMY+wqe1v1(kV_#9DKuvmPGDW@%i}plG*gVZgvVF2$Ac4
zrN1z+l0g4(3igJxOqF8beWyB72kEKxx3f02VyirCa1BnDC@TCYa!yy^GOCB_&
zBZ~y~%%&@<&o@LFc=_!nOvLBR7uO>H#vA%vZ*sYBvM;p2c-ko`Q*Ot{?&t2?Ls;6S
zSyt)IzL0N(O4qceUYN2OXmzgbg=F0&rR}QGO20KJzVKRAT}0LubHx&mdm5ZrSJVwJvQwcO2>hD#%I*{
z)d*|iTXA6j6Jay=FIUN5?bWuJDRr)~=&SN`OBy3Q@o4$DSyK%d%aZ0I$(SE4!Iy46
zgwt}qmBA#UXixssgZ|f|_VHa_4qPARethrT8=EFOJfqpSCx421&f*=gbZWAoB(x?Y
z!<{L6kuoPI*wBhHCeve2NSapZ%A{Y-xc%Hg#2Rg%YHxC#m3Bhp+0w8zFOCmnL0h!-
zps#fin~$SWwKZ4@WXCdG(a>QmAYFz($59GAN|Ez&HBosNvUDYDG``_D46hymuZ
z7?ox3qn2Hs0~icu(b8V)ZO%4ujzn}VfSN4mT(3cJP$
z_sw<4vZQu!w=36cUPRy9)7?;;Iv*q+P}hW?gB2M`4mJvQ;M_+JHHC4d$$6!g>P6J<
zRi$K`g!#6r2XYp1lwFf3y-~F8du+tSgB&rw&1$S5T+nWJnYW0?H>SIl
z5`ZQQEH9<%*@n8K9tQuUy+6jkKL#OT_n-CY3Y5U~HRL?Df4E?R;uG{6=lL1$t*#H7
zjw-jcYpjVv;f=0)k`XUY%Iz5nYe}M)U7k8Y&AAn(Ws?2dc>+S*6q4{bpTq^2yc18$3
zCGzMV=^#5Tw5pL!s(5pWygd?e-JOKq4MNYR)e5}7eGN>Z_I*#VM1!)>aeoK9ANt96
z3c5eKSLl%SvvabDSK&G_`S~!h3P3Qh$o+X4$QPC=RBZ8Dz->R2A(P6vbxULyQ*7ee
z=oX>`!A<%jS}!*AgQ^ncD-rY_apcf)KAn(|aJdW;5fVa3iQJ6kMhnZ{f_>oq7Z+}G
zd|qeUXc&0S=hM2yx;`h}Bxo1|uUzl091BcM*Q=I{mPUBx$uO}W*bs%jhP#J>CDzKc
z@;WT-pI%E_tE#G6(N}Lxb2ezV0uQ^N5J9N^cyH1Z;dwr#sqMaVm5~0YP7j0#0s9#9
zzqjstc5_)Q9b@_6j@}|FCDr?|iDZk-LlFlK-rWT!032-qunMM=6W*(8BpN%;B-J=jLjj+Dl2Y-~^>k3*O>O9A*DANG^a
z8F_bY#PMfoX=!@;B9{{Jmo=%cD<*&tW#n9hnOzW1p#3*
zqU`=0Ixsl+FgZnjN-hun8y0YX4P&9Db*ZXKb0;Qom~boBS8lQ<3+HQQk}v5GsG)7E
zQJQ%NBsQ#TR*uV#z2vE>se98Uw-aEbnzHgSu;jpm@42~$Q#f<8v%jqOL;@4^oN&GWX6*hUuSWg}nVwifSuoD>pH7CB*h6Y$#bG0BC#*py_XHZjM(wtA$*)
zN6UNX3s2~yzOsXmf*16G!Rc>-9tx#&dnyHgK0M}ebnMb`)yf#$Y;
ze`kTF7utCCxl}MrL+H=7((o5{>sxUzaNU!%czWU2<036a2VtQWeW@=}k*d~^9su9W
zfp-9u8@*#yeZSI2C;6mv;G5@>_8s{idBDIi)!I_2!t&P3w9o|@Y@v27_IcxPvm-=a)Cc0GoSec0G7Lak_-cjUt&QF!l*FJfBzq#G%V
zd;4+^zh=`C5GcC#|64aoz8+#)hvi=I)(Pfb74zc(%WT$`#E6Jlv8)!jFDxLov>O*P
z1FUG0JpAykfIQ7@JWpmYnSHtZOJ}KmQy-@jOz3iSXoIzGq0(w1ACrJy5Z*`Q07YE1
z`rdYIelqTZ7Oisy46xmnUA+CWu9|4gubT5WgS|3j(`2r|eOwUYqIjz4q+fRYy?7ve
zft+n7(222@zk2F=kWU~Sz%Xl+>PBMkByt)bV9Yl3hq9e3N5uIIyLRP9l)&K?3^*UN
z#Y2|7_DZU&va?q^g0Ls@6$F5#0!8jY-CQNM)qyS)equj^bd3iR7~kLLqo=-!=4p`a
z8=`ei2TlQwxMv-#VtDp&OAv6>nL5fDq5|sS@;K24fRpM)`6ImlQlctCm~NqU9IgUh
zANyxh1nuZk%=knv7C0Pu{HwLM3$(VIo035;71Jlvq6MfZ6;!Rn?&XkfRl>P?SL~7|
z9u3=tsew`yjzv@y^tFAaoj->2ajE!#Cd$8$v~Mi}TG^v(pyx4ZToRYt@K$xnzOeAu
znZRqj=@|@$g2(cheX{x=W@+p&_Ufz~_C{1M=JqR%(Odp8E6UfeL8AoAR;)A*sUH<3hCyBBrZS)8BK
z!A@z`?oTaexYN_xL`1lZ0jx3niRL{eEvPHU&ET2pf<6J@H_a$C<0ElHJWy
zHK4!UAFTQFBp_k8ZwgSG2w@sII)9(i-<_XpH;&lPen#xhUuS!8qUpq(V!Hx=bJ(`h
z)6`2~N-?ev711O0S#nA~dydQHAy7Z2ua|Nt!cV=dYB-EtevXBT;{WYCAehKYCnBTKWr;$yd`Dxxfk;T}wj!m61BtFv6>=X(T@AIS3K
zSM_SX-CZkN(4kC-+%N*t)UXor>s2VKJUh#VMU2l(wMQkxwrc#CLfI>bsr?KVACPog
ztJdrUL9C~PN$>#tnL&)jKB~Mioc3UYNlI}W-oM5!%)G@*zsPSq`|%y9yZweZ$#_Io
zfGt9cEqq7NE@Y-UnS?2E;$ziMynzFefj&Krd=kWO@$~ue1srz#>HEe|zR|`#mv4G+
z-KSm(F`q>+t}XdMamkr4xiik`
z*4P%_I}t8`qHfTsmw});
zaV`xdi9%HKeZ5D|F5Xr=yV!kuRt**i-0F=^84>=Fb@1xF=gU1)MCu2fg`J@5yg4%W
z8YY3eM8qf0TcI5q!S?)Ic#y-QdST?ZwcgG`+zxRD6LlKNXx66*}O(!Rw7=Am76H>AZU!fqk)w`l={_a){orEzXB5s?`>uF`8T
zAKF;3HDi#9Y|%pXZe#)W+jL+Js*p!&YwOHdZ>I*uYa^zuHH!b*Pk1{`D6rydg2NCr
z5sQ{XBg)AxCecT{8uMPMEUcWm1N2>?2?}jw(`q!EZ14g4TG-yspJlJ=e$^EdKYm8v
zk32TN?YSf0GO9Y;Af`D@Ks7ZjPO|3qb!-g2o#%nyUtqk%UX*&(V>4&}x_cnYp4ZR=
z!OeS740(SmMo=2iGvDz=@Y3VgQ(-2nOA1n+qzs{v`&Vl3;-3IcQx#4r3yfw~{o`&H
z^GTs9?|6Kb>{nlMM;o=pU75Oeo<{FoK6Z~Bn?Dbg*VCtKpTJAfs7+SaRI3idmd&mF
z*ihe*xW)`QdGkH!f{=t_QTph6f$PUTW_?ROsg1eDqFRU`7@8DrydzvzR!Tcda+sz+
zu6tVA3Nd+1bL$pHFfClQ1^5Ljdj%vC7XdO^=b_s@LT0a-_)PlezFKbqJJM%Pc#CiE=#uE~D0iUx`
zU7DL95t8p)xnEO9NGpr$IrKpyH_SH8o7jU4xb^D
zNtivh;Wz%A4S5BHuP?c#i-!AI;^qdtA^3*67)n6=2u~%aHC2(2;7=D6QhON_11_4{WGyrt;7OC}fyC)$=Ftu+%0s|vX4b@{nHx*r>z
zhe&_PrtU&itW_uN#%0(N#5M_KU$RhSPbokB{Lp!ZIn6r8%av_a`Pi6*jRi)m5x(>j
zSCi8Fr;SxMM6y-WjNpUsr`q0iYAX$byjL({3KdQ
zQ^)-PTwdI)$Fr)N@`Z51#-=>U#=eLz)=qPFiI0^#OffA4WJ*MsDj}?rL&+3?HV;c4
zmNRE}R|LDSUmsY=m~Nuk2$C}BP{4#2f=fC0F5O4th;&An3f*eOgM3kbGt)hbIr6a-5sLn4_RPk$UJU7*q9r0QN`vR~3W2G$uOX
zDLjJ}b2U*!m4G^-xOaZ#LFc3G4YTK|J^6ZEtaMGeHuab){{M>XM=dN2Sv2}xscQ{k!5(@`+PRIXvXY|hg
zPgn5SBcWO*Zt*#W?YK7;XHa$0LLXWi_DiE-xmS;-GB7-on6P-PewNk8b)pTv~$@xO%r40>$t5}=QYNqZ-oPgmyaxw9ey+OO+NJy5^nr~V?-2KG
zi@grQ?~)=cGuM>oW!%V@y{`AIdpy?OAxRVz$GKKK_I_9>nTEO8=^nNYEwuYbMBq$A
zIY4@kL_0(St5(h=BNUzmBQN9KkvBR}N_TW=QA>V~mu-I@f7#I_t~;ywbvg+U*!AE1
zwgEs8#XWG_|9`c4)MH(+krZlTNIie#%dW?cl7&wowJMpX4`4-jCxG{wbd#5t$T*u2
zW^ptIz$L9#r@aQ|F#S8|9ETX
z1PI%i6?^_muDF@ro<2utTtU=%EAxv8D?Sm|eU+9L2hBH{
z2Pa!~(w^{-h4quCMG%o_sEDw5s5XG|RZyd@ISNS5nI#w1a;3E;Jh}be%zJHHzH4@F
z3mYexiHi_szmSIxclLN>s;g9vDD~=u(L4tkM5#_RT#i~?7VIr9Prrb)wKn>jp2kZt
z7^dqbdF!S&uWxt&IT9!Bb7GaRVbx9MlT}s$OOt-9RC8M
z>}FMiDCu4;Vop4|v@zCbKyk#$K;61t8(l5HgV1ris9d|f_yhLJ@jdTChil7Gx`13R
zsv=T5O(B^gwuNv`892vqv0~
Date: Tue, 27 Oct 2026 01:16:43 +0800
Subject: [PATCH 15/26] test(skills): keep the diagnostics story typed against
the provider state
The gallery merges arbitrary context keys at runtime through its state shim, but the shared provider type only exposes ExtensionState fields, so passing skills/skillDiagnostics as initialState failed the webview type-check and the memoized webview build. Type the story state explicitly as the provider initialState with a comment explaining the gallery-only fields.
---
webview-ui/playwright/gallery/stories.tsx | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/webview-ui/playwright/gallery/stories.tsx b/webview-ui/playwright/gallery/stories.tsx
index c44f2e2c66..70996c077f 100644
--- a/webview-ui/playwright/gallery/stories.tsx
+++ b/webview-ui/playwright/gallery/stories.tsx
@@ -469,8 +469,14 @@ export const stories: Record = {
message: "Missing required 'description' field",
},
]
+ // The gallery's ExtensionStateContext shim merges arbitrary keys, while the real
+ // provider's state type does not expose the skills diagnostics fields the settings
+ // panel reads; the cast keeps the story typed without changing production state.
+ const initialState = { cwd: "/workspace", skills, skillDiagnostics } as unknown as NonNullable<
+ React.ComponentProps["initialState"]
+ >
return (
-
+
From 691d79fcc716a1f013d114a41224502b20102e96 Mon Sep 17 00:00:00 2001
From: easonLiangWorldedtech
<136036952+easonLiangWorldedtech@users.noreply.github.com>
Date: Tue, 27 Oct 2026 03:35:37 +0800
Subject: [PATCH 16/26] fix(skills): publish through a symlinked SKILL.md and
remove only the empty new directory
Review follow-ups: updateSkillModes resolves the discovered path with fs.realpath before the temp+rename publish, so a symlinked SKILL.md is updated through the link instead of being replaced by a regular file (unresolvable paths fall back to the discovered path and surface the real write failure). createSkill failure cleanup no longer recursively removes the value mkdir(recursive) reports: it removes only this call own skill directory with rmdir while empty, so an ancestor (.roo / skills) can never take sibling skills with it. Tests cover the empty-only removal, a failed empty-only removal (original error surfaces), a pre-existing directory, and mkdir reporting an ancestor with a sibling skill preserved. SkillsManager.spec.ts 116 passed.
---
src/services/skills/SkillsManager.ts | 18 +++++--
.../skills/__tests__/SkillsManager.spec.ts | 48 +++++++++++++++++--
2 files changed, 60 insertions(+), 6 deletions(-)
diff --git a/src/services/skills/SkillsManager.ts b/src/services/skills/SkillsManager.ts
index 895a06891d..44cdcad378 100644
--- a/src/services/skills/SkillsManager.ts
+++ b/src/services/skills/SkillsManager.ts
@@ -756,12 +756,15 @@ Add your skill instructions here.
`
const skillContent = matter.stringify(body, frontmatter, SKILL_YAML_DUMP_OPTIONS)
// Publish the SKILL.md atomically, and leave no half-created skill directory behind
- // when persistence fails.
+ // when persistence fails. mkdir(recursive) can report the first ancestor it created
+ // (.roo or skills), so a recursive removal of that value could delete unrelated
+ // skills: only this call's own skill directory is removed, and only while it is
+ // empty. Any created ancestor stays behind like the empty directory it is.
try {
await this.writeSkillFileAtomically(skillMdPath, skillContent)
} catch (error) {
if (createdDir) {
- await fs.rm(createdDir, { recursive: true, force: true }).catch(() => {})
+ await fs.rmdir(skillDir).catch(() => {})
}
throw error
}
@@ -1095,7 +1098,16 @@ Add your skill instructions here.
// values from being reflowed into folded block scalars, keeping the
// file stable (see issue #859).
const newContent = matter.stringify(body, frontmatter, SKILL_YAML_DUMP_OPTIONS)
- await this.writeSkillFileAtomically(skill.path, newContent)
+ // Resolve the link target first: publishing through the temp+rename must update the file
+ // the skill was discovered through, not replace a symlinked SKILL.md with a regular file.
+ let targetPath = skill.path
+ try {
+ targetPath = await fs.realpath(skill.path)
+ } catch {
+ // Unresolvable path (already missing, or a platform without realpath): keep the
+ // discovered path and let the write report the real failure.
+ }
+ await this.writeSkillFileAtomically(targetPath, newContent)
// Refresh skills list
await this.discoverSkills()
diff --git a/src/services/skills/__tests__/SkillsManager.spec.ts b/src/services/skills/__tests__/SkillsManager.spec.ts
index ff76bba98b..8c194871fc 100644
--- a/src/services/skills/__tests__/SkillsManager.spec.ts
+++ b/src/services/skills/__tests__/SkillsManager.spec.ts
@@ -2329,13 +2329,54 @@ Instructions`)
mockWriteFile.mockRejectedValueOnce(Object.assign(new Error("ENOSPC"), { code: "ENOSPC" }))
mockUnlink.mockResolvedValue(undefined)
mockRm.mockResolvedValue(undefined)
+ mockRmdir.mockResolvedValue(undefined)
await expect(skillsManager.createSkill("failing-skill", "global", "Description")).rejects.toThrow("ENOSPC")
- // The partially written temp file and the directory this call created are removed;
- // the original persistence error is what the caller sees.
+ // The partially written temp file is removed and the directory this call created is
+ // removed only while empty (rmdir, never a recursive rm that could take ancestors
+ // reported by mkdir(recursive) with unrelated skills); the original persistence
+ // error is what the caller sees.
expect(mockUnlink).toHaveBeenCalledWith(expect.stringMatching(/\.SKILL\.md\.[^/\\]+\.tmp$/))
- expect(mockRm).toHaveBeenCalledWith(createdDir, { recursive: true, force: true })
+ expect(mockRmdir).toHaveBeenCalledWith(p(GLOBAL_ROO_DIR, "skills", "failing-skill"))
+ expect(mockRm).not.toHaveBeenCalled()
+ })
+
+ it("leaves a non-empty skill directory alone when the empty-only removal fails", async () => {
+ mockDirectoryExists.mockResolvedValue(false)
+ mockRealpath.mockImplementation(async (p: string) => p)
+ mockReaddir.mockResolvedValue([])
+ mockFileExists.mockResolvedValue(false)
+ mockMkdir.mockResolvedValue(p(GLOBAL_ROO_DIR, "skills", "busy-skill"))
+ mockWriteFile.mockRejectedValueOnce(Object.assign(new Error("ENOSPC"), { code: "ENOSPC" }))
+ mockUnlink.mockResolvedValue(undefined)
+ mockRmdir.mockRejectedValueOnce(Object.assign(new Error("ENOTEMPTY"), { code: "ENOTEMPTY" }))
+
+ // A failed cleanup (the temp file survived, another skill appeared, ...) must not
+ // mask or replace the persistence failure.
+ await expect(skillsManager.createSkill("busy-skill", "global", "Description")).rejects.toThrow("ENOSPC")
+ expect(mockRmdir).toHaveBeenCalledWith(p(GLOBAL_ROO_DIR, "skills", "busy-skill"))
+ expect(mockRm).not.toHaveBeenCalled()
+ })
+
+ it("never recursively removes an ancestor reported by recursive mkdir", async () => {
+ mockDirectoryExists.mockResolvedValue(false)
+ mockRealpath.mockImplementation(async (p: string) => p)
+ mockReaddir.mockResolvedValue([])
+ mockFileExists.mockResolvedValue(false)
+ // mkdir(recursive) reports the first ancestor it created; removing that value
+ // recursively could take the sibling skill directories under it with it.
+ mockMkdir.mockResolvedValue(p(GLOBAL_ROO_DIR, "skills"))
+ mockWriteFile.mockRejectedValueOnce(Object.assign(new Error("ENOSPC"), { code: "ENOSPC" }))
+ mockUnlink.mockResolvedValue(undefined)
+ mockRm.mockResolvedValue(undefined)
+ mockRmdir.mockResolvedValue(undefined)
+
+ await expect(skillsManager.createSkill("isolated-skill", "global", "Description")).rejects.toThrow("ENOSPC")
+
+ // Only this skill's directory is considered, and only for an empty-only removal.
+ expect(mockRmdir).toHaveBeenCalledWith(p(GLOBAL_ROO_DIR, "skills", "isolated-skill"))
+ expect(mockRm).not.toHaveBeenCalled()
})
it("does not remove a pre-existing skill directory when persistence fails", async () => {
@@ -2352,6 +2393,7 @@ Instructions`)
await expect(skillsManager.createSkill("owned-skill", "global", "Description")).rejects.toThrow("EACCES")
expect(mockRm).not.toHaveBeenCalled()
+ expect(mockRmdir).not.toHaveBeenCalled()
expect(mockUnlink).toHaveBeenCalledWith(expect.stringMatching(/\.SKILL\.md\.[^/\\]+\.tmp$/))
})
From 7163cfd89421ab344c58d2f7c02a650032f0e12d Mon Sep 17 00:00:00 2001
From: easonLiangWorldedtech
<136036952+easonLiangWorldedtech@users.noreply.github.com>
Date: Tue, 27 Oct 2026 03:35:37 +0800
Subject: [PATCH 17/26] test(webview): type the skills initial state and keep
the visual test screenshot-only
Review follow-ups: the provider initial-state contract now types skills/skillDiagnostics and seeds both provider values from initialState (empty-array fallbacks), so the gallery story passes them without a double assertion. The behaviour assertions (diagnostic text, locations, valid skill row) already live in the SkillsSettings Vitest suite; the visual test now only mounts the deterministic story, checks bounded layout and pins the screenshot. SkillsSettings.spec.tsx 22 passed; the visual test passes with the unchanged baseline.
---
webview-ui/playwright/gallery/stories.tsx | 8 +-------
.../settings/__tests__/SkillsSettings.visual.tsx | 14 +++-----------
webview-ui/src/context/ExtensionStateContext.tsx | 10 ++++++++--
3 files changed, 12 insertions(+), 20 deletions(-)
diff --git a/webview-ui/playwright/gallery/stories.tsx b/webview-ui/playwright/gallery/stories.tsx
index 70996c077f..c44f2e2c66 100644
--- a/webview-ui/playwright/gallery/stories.tsx
+++ b/webview-ui/playwright/gallery/stories.tsx
@@ -469,14 +469,8 @@ export const stories: Record
= {
message: "Missing required 'description' field",
},
]
- // The gallery's ExtensionStateContext shim merges arbitrary keys, while the real
- // provider's state type does not expose the skills diagnostics fields the settings
- // panel reads; the cast keeps the story typed without changing production state.
- const initialState = { cwd: "/workspace", skills, skillDiagnostics } as unknown as NonNullable<
- React.ComponentProps["initialState"]
- >
return (
-
+
diff --git a/webview-ui/src/components/settings/__tests__/SkillsSettings.visual.tsx b/webview-ui/src/components/settings/__tests__/SkillsSettings.visual.tsx
index b1eff02240..27330ab8d7 100644
--- a/webview-ui/src/components/settings/__tests__/SkillsSettings.visual.tsx
+++ b/webview-ui/src/components/settings/__tests__/SkillsSettings.visual.tsx
@@ -2,21 +2,13 @@ import { expect, test } from "../../../../playwright/coverage-fixture"
import { collectBoundedLayoutFailures } from "../../../../playwright/layout-contracts"
import { mountedStory } from "../../../../playwright/mounted-story"
-test("shows skill load diagnostics with paths, locations and messages", async ({ mount, page }) => {
+test("renders the diagnostics story deterministically", async ({ mount, page }) => {
await page.setViewportSize({ width: 520, height: 480 })
const component = mountedStory(await mount("skills-settings-diagnostics"))
const story = component.getByTestId("skills-settings-story")
- const alert = story.getByRole("alert")
-
- await expect(alert).toBeVisible()
- // The diagnostics are rendered verbatim: path, optional line:column, message.
- await expect(alert).toContainText("/workspace/.roo/skills/broken-skill/SKILL.md:3:12")
- await expect(alert).toContainText("Invalid YAML frontmatter: unexpected end of the stream")
- await expect(alert).toContainText("/home/user/.roo/skills/legacy-mode/SKILL.md")
- await expect(alert).toContainText("Missing required 'description' field")
- // The valid skill next to the diagnostics still renders its own row.
- await expect(story).toContainText("diagnostics-demo")
+ // Behavioural assertions for the diagnostics alert live in the SkillsSettings Vitest
+ // suite; this test only pins the deterministic state and its screenshot.
await expect(story).toHaveScreenshot("skills-settings-diagnostics.png")
await expect.poll(() => story.evaluate(collectBoundedLayoutFailures)).toEqual([])
})
diff --git a/webview-ui/src/context/ExtensionStateContext.tsx b/webview-ui/src/context/ExtensionStateContext.tsx
index ed7af1fd2d..1aef01e2d9 100644
--- a/webview-ui/src/context/ExtensionStateContext.tsx
+++ b/webview-ui/src/context/ExtensionStateContext.tsx
@@ -284,6 +284,10 @@ export const createInitialExtensionState = (): ExtensionState => ({
type ExtensionStateProviderInitialState = Partial & {
routerModels?: RouterModels
+ // The skill state lives outside ExtensionState; the fixture and tests seed it here so the
+ // initial-state contract covers every value the provider exposes.
+ skills?: SkillMetadata[]
+ skillDiagnostics?: SkillDiagnostic[]
}
export const ExtensionStateContextProvider: React.FC<{
@@ -319,8 +323,10 @@ export const ExtensionStateContextProvider: React.FC<{
global: {},
},
)
- const [skills, setSkills] = useState([])
- const [skillDiagnostics, setSkillDiagnostics] = useState([])
+ const [skills, setSkills] = useState(() => initialState?.skills ?? [])
+ const [skillDiagnostics, setSkillDiagnostics] = useState(
+ () => initialState?.skillDiagnostics ?? [],
+ )
const [rules, setRules] = useState([])
const [includeTaskHistoryInEnhance, setIncludeTaskHistoryInEnhance] = useState(
() => initialState?.includeTaskHistoryInEnhance ?? true,
From 304c647553b42700203d091e46c83c44a21922de Mon Sep 17 00:00:00 2001
From: easonLiangWorldedtech
<136036952+easonLiangWorldedtech@users.noreply.github.com>
Date: Tue, 27 Oct 2026 03:54:06 +0800
Subject: [PATCH 18/26] fix(skills): propagate a failed realpath resolution
before the atomic publish
Review follow-up: updateSkillModes no longer falls back to the discovered path when realpath fails at publish time; publishing through the unresolved path could replace a symlink with a regular file or write somewhere other than the target the caller saw. Added a test where discovery succeeds but the resolution fails: the exact error propagates and no write/rename happens. SkillsManager.spec.ts 117 passed.
---
src/services/skills/SkillsManager.ts | 10 ++-----
.../skills/__tests__/SkillsManager.spec.ts | 29 +++++++++++++++++++
2 files changed, 32 insertions(+), 7 deletions(-)
diff --git a/src/services/skills/SkillsManager.ts b/src/services/skills/SkillsManager.ts
index 44cdcad378..6dab743cca 100644
--- a/src/services/skills/SkillsManager.ts
+++ b/src/services/skills/SkillsManager.ts
@@ -1100,13 +1100,9 @@ Add your skill instructions here.
const newContent = matter.stringify(body, frontmatter, SKILL_YAML_DUMP_OPTIONS)
// Resolve the link target first: publishing through the temp+rename must update the file
// the skill was discovered through, not replace a symlinked SKILL.md with a regular file.
- let targetPath = skill.path
- try {
- targetPath = await fs.realpath(skill.path)
- } catch {
- // Unresolvable path (already missing, or a platform without realpath): keep the
- // discovered path and let the write report the real failure.
- }
+ // A resolution failure propagates: publishing through the unresolved path could replace a
+ // symlink with a regular file (or write somewhere other than the target the caller saw).
+ const targetPath = await fs.realpath(skill.path)
await this.writeSkillFileAtomically(targetPath, newContent)
// Refresh skills list
diff --git a/src/services/skills/__tests__/SkillsManager.spec.ts b/src/services/skills/__tests__/SkillsManager.spec.ts
index 8c194871fc..d1b9d704ac 100644
--- a/src/services/skills/__tests__/SkillsManager.spec.ts
+++ b/src/services/skills/__tests__/SkillsManager.spec.ts
@@ -2534,6 +2534,35 @@ Body here.`
expect(parsed2.data.description).toBe(description)
})
+ it("propagates a realpath failure instead of writing through the unresolved path", async () => {
+ const skillDir = p(globalSkillsDir, "unresolvable-skill")
+ const skillMd = p(skillDir, "SKILL.md")
+ mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir)
+ mockReaddir.mockImplementation(async (dir: string) =>
+ dir === globalSkillsDir ? ["unresolvable-skill"] : [],
+ )
+ mockStat.mockImplementation(async (pathArg: string) => {
+ if (pathArg === skillDir) {
+ return { isDirectory: () => true }
+ }
+ throw enoent()
+ })
+ mockFileExists.mockImplementation(async (file: string) => file === skillMd)
+ mockReadFile.mockResolvedValue("---\nname: unresolvable-skill\ndescription: A skill\n---\nInstructions")
+
+ await skillsManager.discoverSkills()
+ // The discovered path resolves, but the publish-time resolution fails: the write must
+ // not fall back to the unresolved (potentially symlinked) path.
+ const resolutionError = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" })
+ mockRealpath.mockRejectedValueOnce(resolutionError)
+
+ await expect(skillsManager.updateSkillModes("unresolvable-skill", "global", ["code"])).rejects.toBe(
+ resolutionError,
+ )
+ expect(mockWriteFile).not.toHaveBeenCalled()
+ expect(mockRename).not.toHaveBeenCalled()
+ })
+
it("should throw error for invalid skill name", async () => {
await expect(skillsManager.createSkill("Invalid-Name", "global", "Description")).rejects.toThrow(
"Skill name must be lowercase letters/numbers/hyphens only",
From 3f0010f21123a90799dd87342ff9f43be8c2dfdc Mon Sep 17 00:00:00 2001
From: easonLiangWorldedtech
<136036952+easonLiangWorldedtech@users.noreply.github.com>
Date: Tue, 27 Oct 2026 04:30:15 +0800
Subject: [PATCH 19/26] fix(skills): keep SKILL.md publishes inside the skills
roots and track temp sidecars
Review follow-ups: updateSkillModes resolves the SKILL.md target and rejects it when the real path falls outside every configured skills root for the source, so a direct skill-directory symlink cannot publish outside the selected folder. Atomic writes track their temp sidecar in the manager while in flight, dispose() removes any still-tracked sidecar, and initialize() sweeps stale sidecars left by a previous host under the known skills roots. Added focused tests for createSkill/updateSkillModes rename rejection (temp removed, original error rethrown), outside-root refusal (no write/rename), and dispose sidecar cleanup. SkillsManager.spec.ts 121 passed; tsc clean.
---
src/services/skills/SkillsManager.ts | 75 ++++++++++++++++
.../skills/__tests__/SkillsManager.spec.ts | 87 +++++++++++++++++++
2 files changed, 162 insertions(+)
diff --git a/src/services/skills/SkillsManager.ts b/src/services/skills/SkillsManager.ts
index 6dab743cca..578dfdaa8c 100644
--- a/src/services/skills/SkillsManager.ts
+++ b/src/services/skills/SkillsManager.ts
@@ -110,16 +110,62 @@ export class SkillsManager {
// Runs discovery passes one at a time, so an older pass can't finish after a newer one
private discoveryQueue: Promise = Promise.resolve()
private isDisposed = false
+ // Atomic SKILL.md writes in flight, tracked so dispose() can remove temp sidecars if the
+ // extension host stops between the staged write and the rename.
+ private activeTempFiles: Set = new Set()
constructor(provider: ClineProvider) {
this.providerRef = new WeakRef(provider)
}
async initialize(): Promise {
+ // A previous extension-host stop can leave a staged SKILL.md temp sidecar behind; the
+ // in-memory tracking cannot survive that, so sweep the known skills directories once.
+ try {
+ await this.sweepStaleSkillTempFiles()
+ } catch {
+ // Stale-cleanup is best effort; discovery and watcher setup still run.
+ }
await this.discoverSkills()
await this.setupFileWatchers()
}
+ /**
+ * Remove SKILL.md temp sidecars left by a previous host: the name pattern is the one
+ * writeSkillFileAtomically generates, and only the skill directories under the known skills
+ * roots are scanned.
+ */
+ private async sweepStaleSkillTempFiles(): Promise {
+ const tempPattern = /^\..*\.SKILL\.md\..*\.tmp$/
+ const roots = (await this.getSkillsDirectories()).map((entry) => entry.dir)
+ for (const root of roots) {
+ let entries: unknown[]
+ try {
+ entries = await fs.readdir(root, { withFileTypes: true })
+ } catch {
+ continue
+ }
+ const directories = [root]
+ for (const entry of entries) {
+ const dirent = entry as { name?: string; isDirectory?: () => boolean }
+ if (!dirent?.isDirectory?.()) continue
+ directories.push(path.join(root, String(dirent.name)))
+ }
+ for (const directory of directories) {
+ let files: string[]
+ try {
+ files = await fs.readdir(directory)
+ } catch {
+ continue
+ }
+ for (const file of files) {
+ if (!tempPattern.test(file)) continue
+ await fs.unlink(path.join(directory, file)).catch(() => {})
+ }
+ }
+ }
+ }
+
/**
* Discover all skills from global and project directories.
* Supports both generic skills (skills/) and mode-specific skills (skills-{mode}/).
@@ -675,6 +721,7 @@ export class SkillsManager {
directory,
`.${path.basename(filePath)}.${Date.now()}.${Math.random().toString(36).slice(2)}.tmp`,
)
+ this.activeTempFiles.add(temporaryPath)
try {
await fs.writeFile(temporaryPath, content, "utf-8")
await fs.rename(temporaryPath, filePath)
@@ -683,6 +730,8 @@ export class SkillsManager {
// failure is the one the caller needs to see.
await fs.unlink(temporaryPath).catch(() => {})
throw error
+ } finally {
+ this.activeTempFiles.delete(temporaryPath)
}
}
@@ -1063,6 +1112,26 @@ Add your skill instructions here.
* @param source - Where the skill is located ("global" or "project")
* @param newModeSlugs - New mode slugs (undefined/empty = any mode)
*/
+ /**
+ * The resolved SKILL.md target must stay inside one of the configured skills roots for the
+ * source: a direct skill-directory symlink can point anywhere, and publishing through it
+ * would write outside the user selected skills folder.
+ */
+ private async assertTargetWithinSkillsRoots(targetPath: string, source: "global" | "project"): Promise {
+ const roots = (await this.getSkillsDirectories())
+ .filter((entry) => entry.source === source)
+ .map((entry) => entry.dir)
+ const inside = async (root: string): Promise => {
+ const resolvedRoot = await fs.realpath(root).catch(() => root)
+ const relative = path.relative(resolvedRoot, targetPath)
+ return relative.length > 0 && !relative.startsWith("..") && !path.isAbsolute(relative)
+ }
+ for (const root of roots) {
+ if (await inside(root)) return
+ }
+ throw new Error(`Refusing to update SKILL.md outside the ${source} skills directories: ${targetPath}`)
+ }
+
async updateSkillModes(name: string, source: "global" | "project", newModeSlugs?: string[]): Promise {
// Find any skill with this name and source (regardless of current mode)
let skill: SkillMetadata | undefined
@@ -1103,6 +1172,7 @@ Add your skill instructions here.
// A resolution failure propagates: publishing through the unresolved path could replace a
// symlink with a regular file (or write somewhere other than the target the caller saw).
const targetPath = await fs.realpath(skill.path)
+ await this.assertTargetWithinSkillsRoots(targetPath, source)
await this.writeSkillFileAtomically(targetPath, newContent)
// Refresh skills list
@@ -1284,6 +1354,11 @@ Add your skill instructions here.
async dispose(): Promise {
this.isDisposed = true
+ // Any sidecar still tracked is a write that never reached its rename.
+ for (const temporaryPath of this.activeTempFiles) {
+ await fs.unlink(temporaryPath).catch(() => {})
+ }
+ this.activeTempFiles.clear()
this.disposables.forEach((d) => d.dispose())
this.disposables = []
this.containerWatchers.forEach((w) => w.dispose())
diff --git a/src/services/skills/__tests__/SkillsManager.spec.ts b/src/services/skills/__tests__/SkillsManager.spec.ts
index d1b9d704ac..02372f88b7 100644
--- a/src/services/skills/__tests__/SkillsManager.spec.ts
+++ b/src/services/skills/__tests__/SkillsManager.spec.ts
@@ -2379,6 +2379,26 @@ Instructions`)
expect(mockRm).not.toHaveBeenCalled()
})
+ it("removes the temp file and rethrows the original error when the create rename fails", async () => {
+ mockDirectoryExists.mockResolvedValue(false)
+ mockRealpath.mockImplementation(async (p: string) => p)
+ mockReaddir.mockResolvedValue([])
+ mockFileExists.mockResolvedValue(false)
+ mockMkdir.mockResolvedValue(p(GLOBAL_ROO_DIR, "skills", "rename-fails"))
+ mockWriteFile.mockResolvedValue(undefined)
+ const renameError = Object.assign(new Error("EEXIST: rename failed"), { code: "EEXIST" })
+ mockRename.mockRejectedValueOnce(renameError)
+ mockUnlink.mockResolvedValue(undefined)
+ mockRmdir.mockResolvedValue(undefined)
+
+ await expect(skillsManager.createSkill("rename-fails", "global", "Description")).rejects.toBe(renameError)
+
+ const temporaryPath = String(mockWriteFile.mock.calls[0][0])
+ expect(temporaryPath).toMatch(/\.SKILL\.md\.[^/\\]+\.tmp$/)
+ expect(mockUnlink).toHaveBeenCalledWith(temporaryPath)
+ expect(skillsManager["activeTempFiles"].size).toBe(0)
+ })
+
it("does not remove a pre-existing skill directory when persistence fails", async () => {
mockDirectoryExists.mockResolvedValue(false)
mockRealpath.mockImplementation(async (p: string) => p)
@@ -2563,6 +2583,64 @@ Body here.`
expect(mockRename).not.toHaveBeenCalled()
})
+ it("removes the temp file and rethrows the original error when the update rename fails", async () => {
+ const skillDir = p(globalSkillsDir, "rename-update")
+ const skillMd = p(skillDir, "SKILL.md")
+ mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir)
+ mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? ["rename-update"] : []))
+ mockStat.mockImplementation(async (pathArg: string) => {
+ if (pathArg === skillDir) {
+ return { isDirectory: () => true }
+ }
+ throw enoent()
+ })
+ mockFileExists.mockImplementation(async (file: string) => file === skillMd)
+ mockReadFile.mockResolvedValue("---\nname: rename-update\ndescription: A skill\n---\nInstructions")
+ mockRealpath.mockImplementation(async (pathArg: string) => pathArg)
+ mockWriteFile.mockResolvedValue(undefined)
+ const renameError = Object.assign(new Error("EIO: rename failed"), { code: "EIO" })
+ mockRename.mockRejectedValueOnce(renameError)
+ mockUnlink.mockResolvedValue(undefined)
+
+ await skillsManager.discoverSkills()
+
+ await expect(skillsManager.updateSkillModes("rename-update", "global", ["code"])).rejects.toBe(renameError)
+
+ const temporaryPath = String(mockWriteFile.mock.calls[0][0])
+ expect(temporaryPath).toMatch(/\.SKILL\.md\.[^/\\]+\.tmp$/)
+ expect(mockUnlink).toHaveBeenCalledWith(temporaryPath)
+ expect(skillsManager["activeTempFiles"].size).toBe(0)
+ })
+
+ it("refuses to update a SKILL.md that resolves outside the skills roots", async () => {
+ const skillDir = p(globalSkillsDir, "escaping")
+ const skillMd = p(skillDir, "SKILL.md")
+ const outsidePath = p("/outside/root", "SKILL.md")
+ mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir)
+ mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? ["escaping"] : []))
+ mockStat.mockImplementation(async (pathArg: string) => {
+ if (pathArg === skillDir) {
+ return { isDirectory: () => true }
+ }
+ throw enoent()
+ })
+ mockFileExists.mockImplementation(async (file: string) => file === skillMd)
+ mockReadFile.mockResolvedValue("---\nname: escaping\ndescription: A skill\n---\nInstructions")
+ // Discovery saw the root path, but the publish-time resolution points outside every
+ // configured skills root: a direct skill-dir symlink must not let the write escape.
+ mockRealpath.mockImplementation(async (pathArg: string) => (pathArg === skillMd ? outsidePath : pathArg))
+ mockWriteFile.mockResolvedValue(undefined)
+ mockRename.mockResolvedValue(undefined)
+
+ await skillsManager.discoverSkills()
+
+ await expect(skillsManager.updateSkillModes("escaping", "global", ["code"])).rejects.toThrow(
+ /Refusing to update SKILL\.md outside the global skills directories/,
+ )
+ expect(mockWriteFile).not.toHaveBeenCalled()
+ expect(mockRename).not.toHaveBeenCalled()
+ })
+
it("should throw error for invalid skill name", async () => {
await expect(skillsManager.createSkill("Invalid-Name", "global", "Description")).rejects.toThrow(
"Skill name must be lowercase letters/numbers/hyphens only",
@@ -3967,4 +4045,13 @@ Instructions`)
})
})
})
+ it("removes tracked temp sidecars on dispose", async () => {
+ mockUnlink.mockResolvedValue(undefined)
+ skillsManager["activeTempFiles"].add("/mock/skills/foo/.SKILL.md.123.abc.tmp")
+
+ await skillsManager.dispose()
+
+ expect(mockUnlink).toHaveBeenCalledWith("/mock/skills/foo/.SKILL.md.123.abc.tmp")
+ expect(skillsManager["activeTempFiles"].size).toBe(0)
+ })
})
From 29019356f962e235222957138781b6b0543c93bf Mon Sep 17 00:00:00 2001
From: easonLiangWorldedtech
<136036952+easonLiangWorldedtech@users.noreply.github.com>
Date: Tue, 27 Oct 2026 04:54:43 +0800
Subject: [PATCH 20/26] fix(skills): exclusive staging, pre-rename
revalidation, and cancellation-safe init
Review follow-ups: staged SKILL.md temp files are created with flag wx so a pre-created symlink at the generated name cannot be followed, and updateSkillModes re-resolves the target immediately before the rename and re-validates it against the source skills roots. The rename-rejection tests now also assert the wx flag, that create cleanup removes only its own directory (rmdir, no recursive rm), and that the existing SKILL.md is not replaced. initialize() is idempotent/cancellation-safe: it tracks the initialization promise and stops after the stale sweep or discovery if the manager was disposed, and setupFileWatchers/watchDirectory return early when disposed (test: initialize after dispose starts no watchers). SkillsManager.spec.ts 122 passed; tsc clean.
---
src/services/skills/SkillsManager.ts | 48 ++++++++++++++-----
.../skills/__tests__/SkillsManager.spec.ts | 15 ++++++
2 files changed, 50 insertions(+), 13 deletions(-)
diff --git a/src/services/skills/SkillsManager.ts b/src/services/skills/SkillsManager.ts
index 578dfdaa8c..a15b8780ab 100644
--- a/src/services/skills/SkillsManager.ts
+++ b/src/services/skills/SkillsManager.ts
@@ -118,16 +118,25 @@ export class SkillsManager {
this.providerRef = new WeakRef(provider)
}
- async initialize(): Promise {
- // A previous extension-host stop can leave a staged SKILL.md temp sidecar behind; the
- // in-memory tracking cannot survive that, so sweep the known skills directories once.
- try {
- await this.sweepStaleSkillTempFiles()
- } catch {
- // Stale-cleanup is best effort; discovery and watcher setup still run.
- }
- await this.discoverSkills()
- await this.setupFileWatchers()
+ private initialization: Promise | null = null
+
+ initialize(): Promise {
+ if (this.initialization) return this.initialization
+ this.initialization = (async () => {
+ // A previous extension-host stop can leave a staged SKILL.md temp sidecar behind; the
+ // in-memory tracking cannot survive that, so sweep the known skills directories once.
+ try {
+ await this.sweepStaleSkillTempFiles()
+ } catch {
+ // Stale-cleanup is best effort; discovery and watcher setup still run.
+ }
+ // Disposal can land during the sweep: stop before touching discovery or watchers.
+ if (this.isDisposed) return
+ await this.discoverSkills()
+ if (this.isDisposed) return
+ await this.setupFileWatchers()
+ })()
+ return this.initialization
}
/**
@@ -715,7 +724,11 @@ export class SkillsManager {
* must not leave a truncated SKILL.md that the loader would silently misparse.
* The temp file is removed when the write or rename fails.
*/
- private async writeSkillFileAtomically(filePath: string, content: string): Promise {
+ private async writeSkillFileAtomically(
+ filePath: string,
+ content: string,
+ validateTarget?: (targetPath: string) => Promise,
+ ): Promise {
const directory = path.dirname(filePath)
const temporaryPath = path.join(
directory,
@@ -723,7 +736,11 @@ export class SkillsManager {
)
this.activeTempFiles.add(temporaryPath)
try {
- await fs.writeFile(temporaryPath, content, "utf-8")
+ // Exclusive creation: a pre-created symlink at the generated name cannot be followed.
+ await fs.writeFile(temporaryPath, content, { encoding: "utf-8", flag: "wx" })
+ // Re-validate the final target immediately before the rename: the target (or an
+ // ancestor) can change between the caller's resolution and this publish.
+ await validateTarget?.(filePath)
await fs.rename(temporaryPath, filePath)
} catch (error) {
// Best effort: a partial temp file must not be left behind. The original
@@ -1173,7 +1190,10 @@ Add your skill instructions here.
// symlink with a regular file (or write somewhere other than the target the caller saw).
const targetPath = await fs.realpath(skill.path)
await this.assertTargetWithinSkillsRoots(targetPath, source)
- await this.writeSkillFileAtomically(targetPath, newContent)
+ await this.writeSkillFileAtomically(targetPath, newContent, async (target) => {
+ const freshTarget = await fs.realpath(target).catch(() => target)
+ await this.assertTargetWithinSkillsRoots(freshTarget, source)
+ })
// Refresh skills list
await this.discoverSkills()
@@ -1264,6 +1284,7 @@ Add your skill instructions here.
}
private async setupFileWatchers(): Promise {
+ if (this.isDisposed) return
// Skip if test environment is detected or VSCode APIs are not available
if (process.env.NODE_ENV === "test" || !vscode.workspace.createFileSystemWatcher) {
return
@@ -1303,6 +1324,7 @@ Add your skill instructions here.
}
private watchDirectory(dirPath: string): void {
+ if (this.isDisposed) return
const watcher = this.createSkillsWatcher(dirPath)
if (watcher) {
this.disposables.push(watcher)
diff --git a/src/services/skills/__tests__/SkillsManager.spec.ts b/src/services/skills/__tests__/SkillsManager.spec.ts
index 02372f88b7..b10edc903e 100644
--- a/src/services/skills/__tests__/SkillsManager.spec.ts
+++ b/src/services/skills/__tests__/SkillsManager.spec.ts
@@ -2608,7 +2608,12 @@ Body here.`
const temporaryPath = String(mockWriteFile.mock.calls[0][0])
expect(temporaryPath).toMatch(/\.SKILL\.md\.[^/\\]+\.tmp$/)
+ expect(mockWriteFile.mock.calls[0][2]).toEqual(expect.objectContaining({ flag: "wx" }))
expect(mockUnlink).toHaveBeenCalledWith(temporaryPath)
+ // The existing SKILL.md was never replaced: the only write went to the temp sidecar and
+ // the failed rename never committed.
+ expect(mockWriteFile).toHaveBeenCalledTimes(1)
+ expect(mockRename).toHaveBeenCalledWith(temporaryPath, skillMd)
expect(skillsManager["activeTempFiles"].size).toBe(0)
})
@@ -4045,6 +4050,16 @@ Instructions`)
})
})
})
+ it("does not start watchers when initialization runs after disposal", async () => {
+ const createWatcher = vi.mocked(vscode.workspace.createFileSystemWatcher)
+ await skillsManager.dispose()
+ createWatcher.mockClear()
+
+ await skillsManager.initialize()
+
+ expect(createWatcher).not.toHaveBeenCalled()
+ })
+
it("removes tracked temp sidecars on dispose", async () => {
mockUnlink.mockResolvedValue(undefined)
skillsManager["activeTempFiles"].add("/mock/skills/foo/.SKILL.md.123.abc.tmp")
From 30e0e01962f8c679c1359eea04f13f3eda4275ec Mon Sep 17 00:00:00 2001
From: easonLiangWorldedtech
<136036952+easonLiangWorldedtech@users.noreply.github.com>
Date: Tue, 27 Oct 2026 05:15:22 +0800
Subject: [PATCH 21/26] fix(skills): match the real sidecar name, guard watcher
setup, and only clean staged files this write owns
Review follow-ups: the stale-sidecar sweep pattern now matches the generated name (.SKILL.md...tmp) with a restart-cleanup test; a focused watcher-guard test runs setupFileWatchers under NODE_ENV=development, then disposes and asserts no watcher is created again; the atomic writer cleans up a staged sidecar unless exclusive creation failed with EEXIST (an existing file at the generated name belongs to someone else), with a test that preserves it. SkillsManager.spec.ts 125 passed.
---
src/services/skills/SkillsManager.ts | 16 ++++--
.../skills/__tests__/SkillsManager.spec.ts | 51 +++++++++++++++++++
2 files changed, 63 insertions(+), 4 deletions(-)
diff --git a/src/services/skills/SkillsManager.ts b/src/services/skills/SkillsManager.ts
index a15b8780ab..761466def8 100644
--- a/src/services/skills/SkillsManager.ts
+++ b/src/services/skills/SkillsManager.ts
@@ -145,7 +145,7 @@ export class SkillsManager {
* roots are scanned.
*/
private async sweepStaleSkillTempFiles(): Promise {
- const tempPattern = /^\..*\.SKILL\.md\..*\.tmp$/
+ const tempPattern = /^\.SKILL\.md\..*\.tmp$/
const roots = (await this.getSkillsDirectories()).map((entry) => entry.dir)
for (const root of roots) {
let entries: unknown[]
@@ -735,17 +735,25 @@ export class SkillsManager {
`.${path.basename(filePath)}.${Date.now()}.${Math.random().toString(36).slice(2)}.tmp`,
)
this.activeTempFiles.add(temporaryPath)
+ // Only a write that actually created the sidecar through exclusive creation may remove it:
+ // when wx fails because the path already exists, that file belongs to someone else.
+ let ownsTemporaryPath = false
try {
// Exclusive creation: a pre-created symlink at the generated name cannot be followed.
await fs.writeFile(temporaryPath, content, { encoding: "utf-8", flag: "wx" })
+ ownsTemporaryPath = true
// Re-validate the final target immediately before the rename: the target (or an
// ancestor) can change between the caller's resolution and this publish.
await validateTarget?.(filePath)
await fs.rename(temporaryPath, filePath)
} catch (error) {
- // Best effort: a partial temp file must not be left behind. The original
- // failure is the one the caller needs to see.
- await fs.unlink(temporaryPath).catch(() => {})
+ // Best effort: a partial temp file must not be left behind, but an EEXIST failure means
+ // exclusive creation never acquired the path and the existing file belongs to someone
+ // else. Any other failure may have created a partial sidecar and is cleaned up; the
+ // original failure is the one the caller needs to see.
+ if (ownsTemporaryPath || (error as { code?: string })?.code !== "EEXIST") {
+ await fs.unlink(temporaryPath).catch(() => {})
+ }
throw error
} finally {
this.activeTempFiles.delete(temporaryPath)
diff --git a/src/services/skills/__tests__/SkillsManager.spec.ts b/src/services/skills/__tests__/SkillsManager.spec.ts
index b10edc903e..d181a7e1f2 100644
--- a/src/services/skills/__tests__/SkillsManager.spec.ts
+++ b/src/services/skills/__tests__/SkillsManager.spec.ts
@@ -2379,6 +2379,24 @@ Instructions`)
expect(mockRm).not.toHaveBeenCalled()
})
+ it("preserves an existing file when exclusive staging fails because the name exists", async () => {
+ mockDirectoryExists.mockResolvedValue(false)
+ mockRealpath.mockImplementation(async (p: string) => p)
+ mockReaddir.mockResolvedValue([])
+ mockFileExists.mockResolvedValue(false)
+ mockMkdir.mockResolvedValue(p(GLOBAL_ROO_DIR, "skills", "exists"))
+ const existsError = Object.assign(new Error("EEXIST: file already exists"), { code: "EEXIST" })
+ mockWriteFile.mockRejectedValueOnce(existsError)
+ mockUnlink.mockResolvedValue(undefined)
+
+ await expect(skillsManager.createSkill("exists", "global", "Description")).rejects.toBe(existsError)
+
+ // wx failed before this write owned the path: the existing file must not be unlinked.
+ expect(mockUnlink).not.toHaveBeenCalled()
+ expect(mockRename).not.toHaveBeenCalled()
+ expect(skillsManager["activeTempFiles"].size).toBe(0)
+ })
+
it("removes the temp file and rethrows the original error when the create rename fails", async () => {
mockDirectoryExists.mockResolvedValue(false)
mockRealpath.mockImplementation(async (p: string) => p)
@@ -4050,6 +4068,39 @@ Instructions`)
})
})
})
+ it("stops creating watchers once the manager is disposed", async () => {
+ const createWatcher = vi.mocked(vscode.workspace.createFileSystemWatcher)
+ const originalNodeEnv = process.env.NODE_ENV
+ process.env.NODE_ENV = "development"
+ try {
+ createWatcher.mockClear()
+ await skillsManager["setupFileWatchers"]()
+ expect(createWatcher).toHaveBeenCalled()
+
+ createWatcher.mockClear()
+ await skillsManager.dispose()
+ await skillsManager["setupFileWatchers"]()
+ expect(createWatcher).not.toHaveBeenCalled()
+ } finally {
+ process.env.NODE_ENV = originalNodeEnv
+ }
+ })
+
+ it("sweeps a staged sidecar left by a previous host", async () => {
+ const staleName = ".SKILL.md.123.abc.tmp"
+ mockReaddir.mockImplementation(async (_dir: string, options?: { withFileTypes?: boolean }) => {
+ if (options?.withFileTypes) {
+ return []
+ }
+ return [staleName]
+ })
+ mockUnlink.mockResolvedValue(undefined)
+
+ await skillsManager["sweepStaleSkillTempFiles"]()
+
+ expect(mockUnlink).toHaveBeenCalledWith(p(globalSkillsDir, staleName))
+ })
+
it("does not start watchers when initialization runs after disposal", async () => {
const createWatcher = vi.mocked(vscode.workspace.createFileSystemWatcher)
await skillsManager.dispose()
From 08b2447db091aef6fcd81808231da95ea38f47b5 Mon Sep 17 00:00:00 2001
From: easonLiangWorldedtech
<136036952+easonLiangWorldedtech@users.noreply.github.com>
Date: Tue, 27 Oct 2026 05:28:15 +0800
Subject: [PATCH 22/26] ci: retrigger platform tests after the vitest worker
teardown flake
The previous run executed all 3934 tests successfully but vitest reported EnvironmentTeardownError (onUserConsoleLog pending) in grace-retry-errors.spec.ts during worker shutdown, failing the job. No source change.
From 1100ade9cf8c6c480fe08ba86873d62fb2d13648 Mon Sep 17 00:00:00 2001
From: easonLiangWorldedtech
<136036952+easonLiangWorldedtech@users.noreply.github.com>
Date: Tue, 27 Oct 2026 05:50:05 +0800
Subject: [PATCH 23/26] fix(skills): narrow the sidecar pattern and serialize
sweep/write per directory
Review follow-ups: the stale-sidecar sweep only matches the writer-generated name (dot, SKILL.md, numeric timestamp, base36 suffix, .tmp), and a static per-directory queue shared by every SkillsManager instance wraps both the sweep and the atomic write create/validate/rename path so cleanup cannot unlink a sidecar another instance is writing. SkillsManager.spec.ts 125 passed.
---
src/services/skills/SkillsManager.ts | 102 +++++++++++++++++----------
1 file changed, 64 insertions(+), 38 deletions(-)
diff --git a/src/services/skills/SkillsManager.ts b/src/services/skills/SkillsManager.ts
index 761466def8..1d9773e2a2 100644
--- a/src/services/skills/SkillsManager.ts
+++ b/src/services/skills/SkillsManager.ts
@@ -113,6 +113,10 @@ export class SkillsManager {
// Atomic SKILL.md writes in flight, tracked so dispose() can remove temp sidecars if the
// extension host stops between the staged write and the rename.
private activeTempFiles: Set = new Set()
+ // Per-directory serialization for SKILL.md sidecar cleanup and writes. The queue is static so
+ // every SkillsManager instance in this process shares it: a sweep cannot unlink a sidecar that
+ // another instance is still writing, and writes cannot interleave their validate/rename steps.
+ private static directoryQueues: Map> = new Map()
constructor(provider: ClineProvider) {
this.providerRef = new WeakRef(provider)
@@ -145,7 +149,7 @@ export class SkillsManager {
* roots are scanned.
*/
private async sweepStaleSkillTempFiles(): Promise {
- const tempPattern = /^\.SKILL\.md\..*\.tmp$/
+ const tempPattern = /^\.SKILL\.md\.\d+\.[a-z0-9]+\.tmp$/
const roots = (await this.getSkillsDirectories()).map((entry) => entry.dir)
for (const root of roots) {
let entries: unknown[]
@@ -161,16 +165,18 @@ export class SkillsManager {
directories.push(path.join(root, String(dirent.name)))
}
for (const directory of directories) {
- let files: string[]
- try {
- files = await fs.readdir(directory)
- } catch {
- continue
- }
- for (const file of files) {
- if (!tempPattern.test(file)) continue
- await fs.unlink(path.join(directory, file)).catch(() => {})
- }
+ await this.withDirectoryLock(directory, async () => {
+ let files: string[]
+ try {
+ files = await fs.readdir(directory)
+ } catch {
+ return
+ }
+ for (const file of files) {
+ if (!tempPattern.test(file)) continue
+ await fs.unlink(path.join(directory, file)).catch(() => {})
+ }
+ })
}
}
}
@@ -724,40 +730,60 @@ export class SkillsManager {
* must not leave a truncated SKILL.md that the loader would silently misparse.
* The temp file is removed when the write or rename fails.
*/
+ private async withDirectoryLock(directory: string, operation: () => Promise): Promise {
+ const key = path.resolve(directory)
+ const previous = SkillsManager.directoryQueues.get(key) ?? Promise.resolve()
+ const run = previous.then(operation)
+ const tracked = run.then(
+ () => undefined,
+ () => undefined,
+ )
+ SkillsManager.directoryQueues.set(key, tracked)
+ try {
+ return await run
+ } finally {
+ if (SkillsManager.directoryQueues.get(key) === tracked) {
+ SkillsManager.directoryQueues.delete(key)
+ }
+ }
+ }
+
private async writeSkillFileAtomically(
filePath: string,
content: string,
validateTarget?: (targetPath: string) => Promise,
): Promise {
const directory = path.dirname(filePath)
- const temporaryPath = path.join(
- directory,
- `.${path.basename(filePath)}.${Date.now()}.${Math.random().toString(36).slice(2)}.tmp`,
- )
- this.activeTempFiles.add(temporaryPath)
- // Only a write that actually created the sidecar through exclusive creation may remove it:
- // when wx fails because the path already exists, that file belongs to someone else.
- let ownsTemporaryPath = false
- try {
- // Exclusive creation: a pre-created symlink at the generated name cannot be followed.
- await fs.writeFile(temporaryPath, content, { encoding: "utf-8", flag: "wx" })
- ownsTemporaryPath = true
- // Re-validate the final target immediately before the rename: the target (or an
- // ancestor) can change between the caller's resolution and this publish.
- await validateTarget?.(filePath)
- await fs.rename(temporaryPath, filePath)
- } catch (error) {
- // Best effort: a partial temp file must not be left behind, but an EEXIST failure means
- // exclusive creation never acquired the path and the existing file belongs to someone
- // else. Any other failure may have created a partial sidecar and is cleaned up; the
- // original failure is the one the caller needs to see.
- if (ownsTemporaryPath || (error as { code?: string })?.code !== "EEXIST") {
- await fs.unlink(temporaryPath).catch(() => {})
+ await this.withDirectoryLock(directory, async () => {
+ const temporaryPath = path.join(
+ directory,
+ `.${path.basename(filePath)}.${Date.now()}.${Math.random().toString(36).slice(2)}.tmp`,
+ )
+ this.activeTempFiles.add(temporaryPath)
+ // Only a write that actually created the sidecar through exclusive creation may remove
+ // it: when wx fails because the path already exists, that file belongs to someone else.
+ let ownsTemporaryPath = false
+ try {
+ // Exclusive creation: a pre-created symlink at the generated name cannot be followed.
+ await fs.writeFile(temporaryPath, content, { encoding: "utf-8", flag: "wx" })
+ ownsTemporaryPath = true
+ // Re-validate the final target immediately before the rename: the target (or an
+ // ancestor) can change between the caller's resolution and this publish.
+ await validateTarget?.(filePath)
+ await fs.rename(temporaryPath, filePath)
+ } catch (error) {
+ // Best effort: a partial temp file must not be left behind, but an EEXIST failure
+ // means exclusive creation never acquired the path and the existing file belongs to
+ // someone else. Any other failure may have created a partial sidecar and is cleaned
+ // up; the original failure is the one the caller needs to see.
+ if (ownsTemporaryPath || (error as { code?: string })?.code !== "EEXIST") {
+ await fs.unlink(temporaryPath).catch(() => {})
+ }
+ throw error
+ } finally {
+ this.activeTempFiles.delete(temporaryPath)
}
- throw error
- } finally {
- this.activeTempFiles.delete(temporaryPath)
- }
+ })
}
async createSkill(
From ca68406ec3becf7e8f4a3615d7c9af4048cc11a4 Mon Sep 17 00:00:00 2001
From: easonLiangWorldedtech
<136036952+easonLiangWorldedtech@users.noreply.github.com>
Date: Tue, 27 Oct 2026 06:13:06 +0800
Subject: [PATCH 24/26] fix(skills): bound the stale sweep by age and key the
directory queue by realpath
Review follow-ups: sweepStaleSkillTempFiles only removes matching sidecars older than ten minutes so a fresh file owned by a live write in another process is left alone; withDirectoryLock resolves the directory through fs.realpath before deriving its queue key (fallback path.resolve) so symlink aliases share one lock. Tests cover the fresh-sidecar survival and alias serialization. SkillsManager.spec.ts 126 passed; tsc clean.
---
src/services/skills/SkillsManager.ts | 14 ++++-
.../skills/__tests__/SkillsManager.spec.ts | 51 ++++++++++++++++++-
2 files changed, 61 insertions(+), 4 deletions(-)
diff --git a/src/services/skills/SkillsManager.ts b/src/services/skills/SkillsManager.ts
index 1d9773e2a2..07d11159c9 100644
--- a/src/services/skills/SkillsManager.ts
+++ b/src/services/skills/SkillsManager.ts
@@ -93,6 +93,10 @@ function isHiddenEntry(name: string): boolean {
return name.startsWith(".")
}
+// Stale-sidecar sweep: a matching temp file younger than this may belong to a live write in
+// another process, so only older sidecars are treated as abandoned by a previous host.
+const SKILL_TEMP_SIDECAR_STALE_MS = 10 * 60 * 1000
+
export class SkillsManager {
private skills: Map = new Map()
private diagnostics: SkillDiagnostic[] = []
@@ -174,7 +178,11 @@ export class SkillsManager {
}
for (const file of files) {
if (!tempPattern.test(file)) continue
- await fs.unlink(path.join(directory, file)).catch(() => {})
+ const stalePath = path.join(directory, file)
+ const stats = await fs.stat(stalePath).catch(() => undefined)
+ // Leave fresh sidecars alone: they may belong to a write in another process.
+ if (!stats || Date.now() - stats.mtimeMs < SKILL_TEMP_SIDECAR_STALE_MS) continue
+ await fs.unlink(stalePath).catch(() => {})
}
})
}
@@ -731,7 +739,9 @@ export class SkillsManager {
* The temp file is removed when the write or rename fails.
*/
private async withDirectoryLock(directory: string, operation: () => Promise): Promise {
- const key = path.resolve(directory)
+ // Normalize through realpath first so a symlinked alias shares the same queue key; fall
+ // back to the resolved path when the directory cannot be resolved (e.g. not created yet).
+ const key = await fs.realpath(directory).catch(() => path.resolve(directory))
const previous = SkillsManager.directoryQueues.get(key) ?? Promise.resolve()
const run = previous.then(operation)
const tracked = run.then(
diff --git a/src/services/skills/__tests__/SkillsManager.spec.ts b/src/services/skills/__tests__/SkillsManager.spec.ts
index d181a7e1f2..c86a4a080b 100644
--- a/src/services/skills/__tests__/SkillsManager.spec.ts
+++ b/src/services/skills/__tests__/SkillsManager.spec.ts
@@ -4086,19 +4086,66 @@ Instructions`)
}
})
- it("sweeps a staged sidecar left by a previous host", async () => {
+ it("sweeps only a staged sidecar left by a previous host", async () => {
const staleName = ".SKILL.md.123.abc.tmp"
+ const freshName = ".SKILL.md.456.def.tmp"
mockReaddir.mockImplementation(async (_dir: string, options?: { withFileTypes?: boolean }) => {
if (options?.withFileTypes) {
return []
}
- return [staleName]
+ return [staleName, freshName]
+ })
+ mockStat.mockImplementation(async (pathArg: string) => {
+ if (pathArg.endsWith(staleName)) {
+ // Older than the stale threshold: abandoned by a previous host.
+ return { mtimeMs: Date.now() - 60 * 60 * 1000 }
+ }
+ if (pathArg.endsWith(freshName)) {
+ return { mtimeMs: Date.now() }
+ }
+ throw enoent()
})
mockUnlink.mockResolvedValue(undefined)
await skillsManager["sweepStaleSkillTempFiles"]()
expect(mockUnlink).toHaveBeenCalledWith(p(globalSkillsDir, staleName))
+ // A fresh matching sidecar may belong to a live write and must survive.
+ expect(mockUnlink).not.toHaveBeenCalledWith(p(globalSkillsDir, freshName))
+ })
+
+ it("shares one directory queue across symlink aliases", async () => {
+ const order: string[] = []
+ let releaseFirst!: () => void
+ const firstGate = new Promise((resolve) => {
+ releaseFirst = resolve
+ })
+ const alias = p("/alias", "skills")
+ mockRealpath.mockImplementation(async (pathArg: string) => (pathArg === alias ? globalSkillsDir : pathArg))
+
+ const first = skillsManager["withDirectoryLock"](alias, async () => {
+ order.push("first:start")
+ await firstGate
+ order.push("first:end")
+ })
+ try {
+ // The lock key is resolved through realpath asynchronously before the operation runs.
+ await vi.waitFor(() => expect(order).toEqual(["first:start"]))
+ const second = skillsManager["withDirectoryLock"](globalSkillsDir, async () => {
+ order.push("second")
+ })
+ await Promise.resolve()
+ // The alias resolves to the same real directory, so the second operation waits.
+ expect(order).toEqual(["first:start"])
+
+ releaseFirst()
+ await Promise.all([first, second])
+ expect(order).toEqual(["first:start", "first:end", "second"])
+ } finally {
+ // Never leave the shared queue holding a gate if an assertion above fails.
+ releaseFirst()
+ await first.catch(() => undefined)
+ }
})
it("does not start watchers when initialization runs after disposal", async () => {
From 741a280a0b2f1b822480e46f90e57c4ae04e726e Mon Sep 17 00:00:00 2001
From: easonLiangWorldedtech
<136036952+easonLiangWorldedtech@users.noreply.github.com>
Date: Tue, 27 Oct 2026 06:28:20 +0800
Subject: [PATCH 25/26] fix(skills): fail closed on pre-rename resolution and
make queued writes disposal-aware
Review follow-ups: the pre-rename realpath has no fallback to the stale path (the error propagates, the owned sidecar is removed, and the rename is skipped), the update rename-rejection test asserts the existing SKILL.md and directory survive, and queued writes reject when the manager is disposed before the operation starts and re-check disposal after acquiring the directory lock and before staging/validation/rename. Descriptor-relative publication is tracked in #2203. SkillsManager.spec.ts 128 passed; tsc clean.
---
src/services/skills/SkillsManager.ts | 13 ++++-
.../skills/__tests__/SkillsManager.spec.ts | 56 +++++++++++++++++++
2 files changed, 67 insertions(+), 2 deletions(-)
diff --git a/src/services/skills/SkillsManager.ts b/src/services/skills/SkillsManager.ts
index 07d11159c9..418a13e42e 100644
--- a/src/services/skills/SkillsManager.ts
+++ b/src/services/skills/SkillsManager.ts
@@ -743,7 +743,11 @@ export class SkillsManager {
// back to the resolved path when the directory cannot be resolved (e.g. not created yet).
const key = await fs.realpath(directory).catch(() => path.resolve(directory))
const previous = SkillsManager.directoryQueues.get(key) ?? Promise.resolve()
- const run = previous.then(operation)
+ const run = previous.then(async () => {
+ // A manager disposed while this operation waited must not run at all.
+ if (this.isDisposed) throw new Error("SkillsManager is disposed")
+ return operation()
+ })
const tracked = run.then(
() => undefined,
() => undefined,
@@ -774,12 +778,15 @@ export class SkillsManager {
// it: when wx fails because the path already exists, that file belongs to someone else.
let ownsTemporaryPath = false
try {
+ if (this.isDisposed) throw new Error("SkillsManager is disposed")
// Exclusive creation: a pre-created symlink at the generated name cannot be followed.
await fs.writeFile(temporaryPath, content, { encoding: "utf-8", flag: "wx" })
ownsTemporaryPath = true
+ if (this.isDisposed) throw new Error("SkillsManager is disposed")
// Re-validate the final target immediately before the rename: the target (or an
// ancestor) can change between the caller's resolution and this publish.
await validateTarget?.(filePath)
+ if (this.isDisposed) throw new Error("SkillsManager is disposed")
await fs.rename(temporaryPath, filePath)
} catch (error) {
// Best effort: a partial temp file must not be left behind, but an EEXIST failure
@@ -1235,7 +1242,9 @@ Add your skill instructions here.
const targetPath = await fs.realpath(skill.path)
await this.assertTargetWithinSkillsRoots(targetPath, source)
await this.writeSkillFileAtomically(targetPath, newContent, async (target) => {
- const freshTarget = await fs.realpath(target).catch(() => target)
+ // Fail closed: a resolution failure must propagate so the staged temp write is removed
+ // and the rename is skipped, rather than publishing through a stale path.
+ const freshTarget = await fs.realpath(target)
await this.assertTargetWithinSkillsRoots(freshTarget, source)
})
diff --git a/src/services/skills/__tests__/SkillsManager.spec.ts b/src/services/skills/__tests__/SkillsManager.spec.ts
index c86a4a080b..ca2f33f4f0 100644
--- a/src/services/skills/__tests__/SkillsManager.spec.ts
+++ b/src/services/skills/__tests__/SkillsManager.spec.ts
@@ -2632,9 +2632,53 @@ Body here.`
// the failed rename never committed.
expect(mockWriteFile).toHaveBeenCalledTimes(1)
expect(mockRename).toHaveBeenCalledWith(temporaryPath, skillMd)
+ // The existing SKILL.md and its directory remain untouched.
+ expect(mockReadFile).toHaveBeenCalledWith(skillMd, "utf-8")
+ expect(mockWriteFile).not.toHaveBeenCalledWith(skillMd, expect.anything(), expect.anything())
+ expect(mockRmdir).not.toHaveBeenCalled()
+ expect(mockRm).not.toHaveBeenCalled()
expect(skillsManager["activeTempFiles"].size).toBe(0)
})
+ it("fails closed when the pre-rename target resolution fails", async () => {
+ const skillDir = p(globalSkillsDir, "resolution-fails")
+ const skillMd = p(skillDir, "SKILL.md")
+ mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir)
+ mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? ["resolution-fails"] : []))
+ mockStat.mockImplementation(async (pathArg: string) => {
+ if (pathArg === skillDir) {
+ return { isDirectory: () => true }
+ }
+ throw enoent()
+ })
+ mockFileExists.mockImplementation(async (file: string) => file === skillMd)
+ mockReadFile.mockResolvedValue("---\nname: resolution-fails\ndescription: A skill\n---\nInstructions")
+ // The first resolution succeeds; the immediate pre-rename resolution fails and must
+ // propagate instead of publishing through the stale path.
+ let resolutions = 0
+ mockRealpath.mockImplementation(async (pathArg: string) => {
+ if (pathArg === skillMd) {
+ resolutions += 1
+ if (resolutions === 1) {
+ return skillMd
+ }
+ throw Object.assign(new Error("EIO: resolution failed"), { code: "EIO" })
+ }
+ return pathArg
+ })
+ mockWriteFile.mockResolvedValue(undefined)
+ mockUnlink.mockResolvedValue(undefined)
+
+ await skillsManager.discoverSkills()
+
+ await expect(skillsManager.updateSkillModes("resolution-fails", "global", ["code"])).rejects.toThrow(
+ "EIO: resolution failed",
+ )
+ const temporaryPath = String(mockWriteFile.mock.calls[0][0])
+ expect(mockUnlink).toHaveBeenCalledWith(temporaryPath)
+ expect(mockRename).not.toHaveBeenCalled()
+ })
+
it("refuses to update a SKILL.md that resolves outside the skills roots", async () => {
const skillDir = p(globalSkillsDir, "escaping")
const skillMd = p(skillDir, "SKILL.md")
@@ -4158,6 +4202,18 @@ Instructions`)
expect(createWatcher).not.toHaveBeenCalled()
})
+ it("rejects a queued write once the manager is disposed", async () => {
+ await skillsManager.dispose()
+ mockWriteFile.mockResolvedValue(undefined)
+ mockUnlink.mockResolvedValue(undefined)
+
+ await expect(
+ skillsManager["writeSkillFileAtomically"](p(globalSkillsDir, "SKILL.md"), "content"),
+ ).rejects.toThrow("SkillsManager is disposed")
+ expect(mockWriteFile).not.toHaveBeenCalled()
+ expect(mockRename).not.toHaveBeenCalled()
+ })
+
it("removes tracked temp sidecars on dispose", async () => {
mockUnlink.mockResolvedValue(undefined)
skillsManager["activeTempFiles"].add("/mock/skills/foo/.SKILL.md.123.abc.tmp")
From 38844c7d1a439c97dfe8249ea0c4f003e160c9c5 Mon Sep 17 00:00:00 2001
From: easonLiangWorldedtech
<136036952+easonLiangWorldedtech@users.noreply.github.com>
Date: Tue, 27 Oct 2026 06:50:38 +0800
Subject: [PATCH 26/26] test(skills): pin the directory-queue disposal guard
directly
Review follow-up: block one withDirectoryLock operation, queue a second, dispose the manager, then release the first; the queued operation rejects with SkillsManager is disposed and its callback never runs. The end-to-end writeSkillFileAtomically disposal test stays. SkillsManager.spec.ts 129 passed.
---
.../skills/__tests__/SkillsManager.spec.ts | 27 +++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/src/services/skills/__tests__/SkillsManager.spec.ts b/src/services/skills/__tests__/SkillsManager.spec.ts
index ca2f33f4f0..15f5d3db47 100644
--- a/src/services/skills/__tests__/SkillsManager.spec.ts
+++ b/src/services/skills/__tests__/SkillsManager.spec.ts
@@ -4214,6 +4214,33 @@ Instructions`)
expect(mockRename).not.toHaveBeenCalled()
})
+ it("never runs a queued directory operation after the manager is disposed", async () => {
+ let releaseFirst!: () => void
+ const firstGate = new Promise((resolve) => {
+ releaseFirst = resolve
+ })
+ let firstStarted = false
+ let secondRan = false
+
+ const first = skillsManager["withDirectoryLock"](globalSkillsDir, async () => {
+ firstStarted = true
+ await firstGate
+ })
+ await vi.waitFor(() => expect(firstStarted).toBe(true))
+
+ // The second operation is queued behind the first and must be rejected after disposal
+ // without running its callback.
+ const second = skillsManager["withDirectoryLock"](globalSkillsDir, async () => {
+ secondRan = true
+ })
+ await skillsManager.dispose()
+ releaseFirst()
+
+ await first
+ await expect(second).rejects.toThrow("SkillsManager is disposed")
+ expect(secondRan).toBe(false)
+ })
+
it("removes tracked temp sidecars on dispose", async () => {
mockUnlink.mockResolvedValue(undefined)
skillsManager["activeTempFiles"].add("/mock/skills/foo/.SKILL.md.123.abc.tmp")