fix: Windows install 404s, skill list duplication, Python frontend false positives - #172
Conversation
… positives - Do not send GITHUB_TOKEN/GH_TOKEN to raw.githubusercontent.com (midudev#124): invalid Bearer tokens make GitHub return 404 for public skill files. - Normalize CRLF→LF when verifying/copying local registry text so Windows autocrlf checkouts still match manifest SHA-256 hashes. - multiSelect: track exact drawn lines + parse batched CSI keys so arrow navigation no longer leaves duplicate skill rows (midudev#126). - Suppress file-based frontend detection when non-JS backends are present (Python/Java/Go/etc.) so HTML/CSS docs/templates do not force a11y/SEO skills (midudev#48). Package-based frontend detection still wins. Closes midudev#124 Closes midudev#126 Closes midudev#48
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSe actualizan el instalador, la detección de frontend y la UI de terminal. El instalador restringe la autorización por host y normaliza archivos de texto; la detección separa señales por origen; la UI admite salida configurable, limpieza por cursor guardado y secuencias CSI agrupadas. ChangesInstalador y registro local
Detección de frontend
UI de terminal
Estimated code review effort: 3 (Moderate) | ~30 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/autoskills/ui.ts (1)
147-193: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCuenta también las filas envueltas al calcular
linesDrawn
newlinessolo suma los saltos “duros”; si una línea se parte por el ancho de la terminal,clearRendered()sube menos filas de las que ocupó y quedan restos del frame anterior, sobre todo con labels/hints largos o el footer. Conviene contar el alto visual real del bloque, no solo los\n.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/autoskills/ui.ts` around lines 147 - 193, Actualiza la función draw para que linesDrawn contabilice la altura visual de cada línea envuelta según el ancho disponible de la terminal, incluyendo labels, hints, grupos y el footer. Sustituye o complementa el contador newlines con el cálculo de filas renderizadas, manteniendo el salto adicional entre grupos y el comportamiento de clearRendered.
🧹 Nitpick comments (1)
packages/autoskills/ui.ts (1)
205-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParsing de secuencias CSI agrupadas: correcto, pero sin cobertura de tests.
La lógica de
onData(normalizar CRLF, detectarESC [+ dígitos/;+ byte final como una sola tecla, y saltar ESC sueltos/incompletos) despacha correctamente varias secuencias de flecha concatenadas en un mismodata, resolviendo el caso descrito en el comentario (#126). El PR indica que la validación manual de navegación con flechas sigue pendiente; dado que ya existe infraestructura de tests paramultiSelectcon stdin simulado (vercollect.test.ts), valdría la pena añadir un test que envíe undatacon múltiples secuencias\x1b[A/\x1b[Bconcatenadas y verifique que el cursor avanza una vez por secuencia (no una vez por todo el chunk).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/autoskills/ui.ts` around lines 205 - 230, Add automated coverage for the onData parsing in the multiSelect stdin test infrastructure, following the existing patterns in collect.test.ts. Send concatenated \x1b[A and \x1b[B sequences through one data chunk and assert that each CSI sequence moves the cursor exactly once, preserving the current handling of CRLF and incomplete or bare ESC input.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/autoskills/lib.ts`:
- Around line 565-592: Update the FILE_FRONTEND_SUPPRESS_TECH_IDS set by
replacing "aspnet-core" with "aspnetcore" and "blazor" with "aspnet-blazor",
matching the IDs returned by detection while preserving all other entries.
---
Outside diff comments:
In `@packages/autoskills/ui.ts`:
- Around line 147-193: Actualiza la función draw para que linesDrawn contabilice
la altura visual de cada línea envuelta según el ancho disponible de la
terminal, incluyendo labels, hints, grupos y el footer. Sustituye o complementa
el contador newlines con el cálculo de filas renderizadas, manteniendo el salto
adicional entre grupos y el comportamiento de clearRendered.
---
Nitpick comments:
In `@packages/autoskills/ui.ts`:
- Around line 205-230: Add automated coverage for the onData parsing in the
multiSelect stdin test infrastructure, following the existing patterns in
collect.test.ts. Send concatenated \x1b[A and \x1b[B sequences through one data
chunk and assert that each CSI sequence moves the cursor exactly once,
preserving the current handling of CRLF and incomplete or bare ESC input.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0122bb16-4052-435d-9c91-22abe619125b
📒 Files selected for processing (5)
packages/autoskills/installer.tspackages/autoskills/lib.tspackages/autoskills/tests/detect.test.tspackages/autoskills/tests/installer.test.tspackages/autoskills/ui.ts
eoinmcgee1993
left a comment
eoinmcgee1993
left a comment
There was a problem hiding this comment.
Approved in CodeRabbit Change Stack
eoinmcgee1993
left a comment
eoinmcgee1993
left a comment
There was a problem hiding this comment.
Approved in CodeRabbit Change Stack
eoinmcgee1993
left a comment
eoinmcgee1993
left a comment
There was a problem hiding this comment.
Approved in CodeRabbit Change Stack
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/autoskills/tests/detect.test.ts (1)
384-392: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVerifica explícitamente la detección de Python.
La aserción
python || fastapipermite que el test pase aunquepythonno se detecte. Como este caso valida específicamente backends Python, comprueba ambas señales o, como mínimo, exigepython.Propuesta
- ok(detected.some((t) => t.id === "python" || t.id === "fastapi")); + ok(detected.some((t) => t.id === "python")); + ok(detected.some((t) => t.id === "fastapi"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/autoskills/tests/detect.test.ts` around lines 384 - 392, Update the test case around detectTechnologies to explicitly require Python detection, replacing the permissive python-or-fastapi assertion with a direct assertion for the python technology signal while preserving the isFrontend false check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/autoskills/ui.ts`:
- Around line 237-242: Update the Ctrl+C branch identified by the key check to
exit with status code 130 instead of 0 after cleanup and cursor restoration,
preserving the existing interruption flow.
---
Outside diff comments:
In `@packages/autoskills/tests/detect.test.ts`:
- Around line 384-392: Update the test case around detectTechnologies to
explicitly require Python detection, replacing the permissive python-or-fastapi
assertion with a direct assertion for the python technology signal while
preserving the isFrontend false check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b0afa8c-536c-4c8b-9fc2-6960527c2524
📒 Files selected for processing (6)
packages/autoskills/installer.tspackages/autoskills/lib.tspackages/autoskills/tests/collect.test.tspackages/autoskills/tests/detect.test.tspackages/autoskills/tests/installer.test.tspackages/autoskills/ui.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/autoskills/lib.ts
- packages/autoskills/tests/installer.test.ts
- packages/autoskills/installer.ts
CodeRabbit: process.exit(0) on interrupt looks like success to shell scripts. Use 130 (128+SIGINT) for standard interrupt status.
|
Addressed CodeRabbit on Ctrl+C: skill-selection abort now uses |
Review follow-upWrap-height / Covered by Ctrl+C exit code: fixed to Happy to adjust further if maintainers prefer hard-wrap-to-columns as well. |
3 participants
Summary
Fixes three high-impact open bugs that block real installs and confuse detection:
download failed … 404for every skillAuthorization: Bearertoraw.githubusercontent.com. Invalid/expiredGITHUB_TOKEN/GH_TOKENvalues make GitHub 404 public raw files (reproduced with a dead fine-grained PAT). Also normalize CRLF→LF when hashing/copying local registry text so Windowscore.autocrlfcheckouts still verify..html/.cssRelated
main— not re-implemented here.Test plan
node --test --experimental-strip-types tests/*.test.tsinpackages/autoskills→ 360 passGITHUB_TOKEN=invalid npx autoskills -yon a sample project installs skillsSummary by CodeRabbit
multiSelectahora permite inyectar una funciónwriteFnpara redirigir la salida.