feat(cli): remediation hints on scan/test failures#283
Conversation
|
@albatrossflyon-coder great work on these! #281 and #284 are merged. #283 conflicts with #281 since both modify the same files. Could you rebase #283 on top of the current main? The --quiet changes from #281 are already in — your remediation hints PR just needs to rebase cleanly on top. Thanks for the incredible pace! |
|
@albatrossflyon-coder hey — #281 is merged now. There are minor conflicts in and that need resolving. Happy to fix them for you and merge, or you can rebase whenever you have time. Your call — great work as always. |
|
Coverage gap in src/tools/ — filed as #282 Hey William, Following up after digging into a few issues on mcp-observatory (just shipped #280 and #281) — found something worth flagging separately: your coverage CI check has been failing on main for a few days now, not tied to any specific PR. Root cause is src/tools/* sitting at 10-15% coverage against a 90% global threshold. Filed the details as #282, didn't attempt a fix since it's real scope, not a quick pass. Appreciate the follow — been enjoying working in this codebase. |
Failures told the user what broke but not how to fix it -- worst case, `test` showed nothing about a failure at all (no fatalError display whatsoever, unlike `scan`). Add suggestFix() (helpers.ts): a lightweight one-line remediation lookup for the failure classes outside the rich connection-diagnosis path (thrown exceptions, config parsing errors). Connection failures during MCP session startup already get a much better diagnosis via formatConnectionFailureDiagnosis (diagnosis/likely causes/next steps, stored in RunArtifact.fatalError) -- that system just wasn't being surfaced anywhere close to fully. Added firstNextStep() (failure-diagnosis.ts) to pull the first "Next steps:" bullet out of that existing diagnosis for callers that only have room for one line. Wired into: - scan.ts's exception catch block -- suggestFix() after the existing friendlyMsg (ENOENT/ECONNREFUSED/timeout) messaging - scan.ts's fatalError display -- was showing only the diagnosis header line with no remediation; now also shows firstNextStep() - test.ts -- was showing nothing for a failed target beyond a bare red X and the tool/prompt/resource counts. Now shows the fatalError header plus firstNextStep()/suggestFix(), matching scan.ts. Fixes KryptosAI#245
59a2883 to
48ae2a5
Compare
|
Rebased onto current main — conflicts with #281 are resolved, PR shows mergeable now. Thanks for the flag! |
|
Merged — this is excellent work. The test.ts fix alone is worth the PR (showing nothing for failures was a real papercut). Clean integration of the existing rich diagnosis system into the inline output, solid test coverage, and thanks for catching the setup-ci-conversion prompt format drift too. Really appreciate the thoroughness and the rebase turnaround! |
…y-peer-deps - Downgrade TypeScript from 7.0.2 to 5.8.0 (compatible with typescript-eslint 8.64.0) - Remove --legacy-peer-deps from all 4 CI workflows - Fix CI prompt test: restore text format from #283 (was reverted by rebase) - Fix privacy test: exclude safety-index artifacts from forbidden-pattern scan - Exclude WIP permission-delta scripts from tsc - Fix build: remove missing processIdentifyFlag import from init-ci.ts
Summary
Fixes #245.
Note: this branch is built on top of #281 (
feat/quiet-flag-wiring) — GitHub won't let me set that as the PR base cross-repo (only exists on my fork), so this diff includes #281's commit too until that one merges and I can rebase. The remediation-hints-specific commit is the second one in the PR — happy to rebase ontomaindirectly right now instead if you'd rather review this standalone.What I found
The issue's ask was "error messages should include a remediation hint" — but digging in,
testdoesn't even show the error message today, let alone a hint. Reproduced live:Also found this codebase already has a much richer diagnosis system than what the issue's suggested fix implies:
formatConnectionFailureDiagnosis(src/utils/failure-diagnosis.ts) produces a full diagnosis/likely-causes/next-steps block, stored inRunArtifact.fatalError— but it's only rendered byrenderWatchFirstRun/renderRunTerminal(used bywatchand single-target renderers), not byscanortest's own inline printing.scanshows just the diagnosis header line;testshows nothing at all.Fix
suggestFix()(helpers.ts) — the lightweight one-line lookup the issue asked for (timeout / ECONNREFUSED / ENOENT-spawn / invalid-JSON), for the failure classes outside the rich diagnosis path (thrown exceptions before a connection is even attempted, config parsing errors)firstNextStep()(failure-diagnosis.ts) — pulls the first "Next steps:" bullet out of the existing rich diagnosis, forscan/test's one-line-of-room displayscan.ts— exception catch block getssuggestFix()after the existing friendly message; thefatalErrordisplay (previously header-only) now also showsfirstNextStep()test.ts— was showing nothing on failure; now shows thefatalErrorheader plusfirstNextStep()??suggestFix(), matchingscan.ts's patternVerified live
Same improvement confirmed on
scanagainst a config pointing at a broken server.Testing
suggestFix(), 3 forfirstNextStep()(extracts first bullet, handles no-Next-steps text, handles Next-steps-with-nothing-after)npx tsc --noEmit: cleannpx eslinton all changed files: cleanmain(environment-specific, unrelated — see feat(scan): add passed/failed/warnings summary line #280/fix(cli): wire up --quiet to actually suppress logo and CTAs #281 for the same confirmation)