nofeat: implement diagnostic error messages in findConfig#15
nofeat: implement diagnostic error messages in findConfig#15theDakshJaitly merged 3 commits intotheDakshJaitly:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates findConfig to emit more specific, actionable diagnostics when the mex scaffold/config can’t be located, aiming to improve developer experience over the prior generic “No scaffold found” message.
Changes:
- Added targeted error messages for common misconfigurations (running inside
.mex/, missing git repo, missing scaffold). - Updated the fallback scaffold-not-found message to include concrete setup instructions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Added specific checks for .mex/ directory, git init, and scaffold completeness. - Note: Existing tests in test/config.test.ts are currently failing due to the message change. Waiting for confirmation to update test expectations.
799d59b to
ebba57f
Compare
|
"Differentiates between missing Git initialization, missing scaffold, and incomplete scaffold." |
|
looks good. gonna merge soon. Thanks Carlos! |
yeah i realised that was an issue, now fixed thanks to @c4rl0s04 ! |
|
Thank you for your fast reply. FYI: I'm the guy from Reddit with the tests with openclaw. |
theDakshJaitly
left a comment
There was a problem hiding this comment.
Hey @c4rl0s04, thanks for the updates -- the logic restructuring is much better now (moving the .mex/ path check up front, extracting gitRoot, cross-platform split). But there are still test failures to sort out, and one of them points to a real bug.
Category A: Expected test failures (tests 1 & 2)
These just need updated error message strings. The behavior change is intentional -- that's the whole point of this PR.
- "throws when no scaffold found (no .git)" -- old:
"No scaffold found...", new:"No git repository found...". Just update thetoThrow()string. - "throws when no scaffold found (with .git)" -- old:
"No scaffold found...", new:"No .mex/ scaffold found...". Same deal.
No issues here, just update the expectations.
Category B: The setup.sh check has unintended side effects (tests 3 & 5)
This is the real problem. To fix the dead code issue from the first version, findScaffoldRoot was changed from:
if (existsSync(mexDir)) return mexDir;to:
if (existsSync(mexDir) && existsSync(resolve(mexDir, "setup.sh"))) return mexDir;This fixes the dead code, but it changes what counts as a valid scaffold everywhere, not just in the error path. That causes two unintended regressions:
Test 3: "works without .git if scaffold exists"
Creates a .mex/ directory. Previously that was a valid scaffold. Now it's "incomplete" because there's no setup.sh inside, so it throws instead of returning a config. A real user who has .mex/ cloned but hasn't run setup.sh yet would hit this too.
Test 5: "prefers .mex/ over context/"
Has both .mex/ and context/. Previously .mex/ was preferred. Now .mex/ silently gets skipped (no setup.sh) and context/ is used instead. The user gets no warning that their .mex/ is being ignored. And the "incomplete scaffold" diagnostic in findConfig never fires either, because findScaffoldRoot returned a non-null value via the context/ fallback.
Root cause and suggested fix
The setup.sh check is in the wrong place. It was added to findScaffoldRoot (which determines scaffold validity globally) when it should only affect the error diagnostics in findConfig.
The cleanest fix: keep findScaffoldRoot unchanged (the original simple existsSync check). Instead, add a separate completeness check in findConfig before calling findScaffoldRoot. Something like:
const mexDir = resolve(projectRoot, ".mex");
if (existsSync(mexDir) && !existsSync(resolve(mexDir, "setup.sh"))) {
throw new Error("Scaffold directory exists but looks incomplete. Run: bash .mex/setup.sh");
}
const scaffoldRoot = findScaffoldRoot(projectRoot);
if (!scaffoldRoot) {
// now .mex/ truly doesn't exist, so the remaining diagnostics work naturally
...
}This way:
- The "incomplete scaffold" diagnostic works correctly
findScaffoldRootstill treats any existing.mex/as valid (no regressions)- The
.mex/preference overcontext/is preserved
Summary
- Tests 1 & 2: expected failures, just update the
toThrow()strings - Tests 3 & 5: real bug from the
setup.shcheck being infindScaffoldRoot-- move that logic intofindConfiginstead - Please also add new test cases for each diagnostic path (inside
.mex/, no git, incomplete scaffold, no scaffold)
Once those are sorted this is good to go!
|
Done! Thanks for catching those edge cases. I've reverted I also updated the existing tests and added the new test cases covering every diagnostic path you mentioned. Ready for another look! |
theDakshJaitly
left a comment
There was a problem hiding this comment.
All feedback from the previous review has been addressed:
findScaffoldRootreverted to the simpleexistsSyncchecksetup.shcompleteness check moved intofindConfigwhere it belongs- All 4 diagnostic paths covered with tests
- Existing tests updated correctly
All 85 tests pass against current main, no conflicts. Good to merge.
findConfig..mex/directory.test/config.test.tsare currently failing due to the change in error message strings. I am waiting for confirmation to update the test expectations to match the new diagnostic messages.Fixes #13
What
This change replaces the generic "No scaffold found" error with specific, actionable messages that guide the user on how to fix the environment (e.g., suggesting
git initor providing thegit clonecommand for the scaffold).Why
To improve Developer Experience (DX) by providing clear instructions when the tool is incorrectly configured, instead of leaving the user to guess why the scaffold wasn't found.
Type of change
How to test
.gitfolder and run any mex command to see the "No git repository found" error..mexfolder and run a command to see the "No .mex/ scaffold found" with the clone instructions..mex/directory to see the "You're inside the .mex/ directory" warning.Checklist
npm test)