[chloe41427]

What

Blocks SDKMAN installation on Cygwin, as requested in #75.

Cygwin is unsupported: its path handling is incompatible with the native SDKMAN components, which surfaces as confusing failures for users — empty sdk list java, broken sdk selfupdate, and platform detection falling back to exotic.

How

• app/views/includes/sanity.scala.txt: added a guard at the top of the sanity checks that bails out early when Cygwin is detected, with a clear message pointing users to WSL2. This reuses the existing $cygwin flag, which os.scala.txt already sets from uname (CYGWIN*). @includes.os() runs before @includes.sanity() in the install template, so the flag is available.
• features/installation.feature: added a scenario asserting the rendered install script contains the Cygwin guard and message.

Acceptance criteria (from #75)

• Installer hook detects Cygwin (CYGWIN* via the existing uname-based $cygwin flag)
• Exits with a clear message directing users to a supported alternative (WSL2)
• No change to behaviour on supported platforms (guard only fires when $cygwin == true)

Notes

• No Java/sbt toolchain on the dev box, so the Cucumber suite was not run locally — CI covers it.
• Deploy: this is a Scala hooks service released via sdkman-ansible, so it needs an ansible deploy after merge.

Closes #75

[marc0der]

@claude please review

[claude]

Claude finished @marc0der's task in 19s —— View job

---

Review of PR #99: block installation on Cygwin

• Read the diff and the surrounding templates
• Check how the install templates are assembled
• Post the review

Summary

I don't think this PR is needed, and I'd recommend against merging it as is. Master already blocks Cygwin installs.

Main finding: duplicate of an existing guard

app/views/includes/cygwin_check.scala.txt is already on origin/master and is included at the top of both install_stable.scala.txt and install_beta.scala.txt (line 6 in each). It does the same job:

• It refuses to install when uname -s matches CYGWIN_NT*.
• It prints a message pointing to WSL2.
• It exits with status 1.

With this PR, the install script would contain two Cygwin guards:

1. The existing @includes.cygwin_check(), which runs first.
2. The new if [[ "$cygwin" == true ]] block in sanity.scala.txt.

The new block can never be reached on Cygwin, because the earlier check exits first. It's dead code, and it adds a second message to keep in sync with the first.

Please check whether #75 is already satisfied by the existing include. If it is, close this PR. If #75 is about something the existing guard misses, say what in the issue and fix cygwin_check instead of adding a second check.

Minor points

• Test doesn't prove the guard works. The new scenario in features/installation.feature:18 only checks that the rendered script contains a string. It doesn't run the script under a Cygwin-like uname. The existing guard also has no feature coverage, so a test for the existing include would be more useful than one for the duplicate.
• Detection mismatch. The existing check matches CYGWIN_NT* via uname -s. The new check uses the $cygwin flag from os.scala.txt, which matches CYGWIN*. If there's a real gap (for example a uname that doesn't start with CYGWIN_NT), the right fix is to widen the pattern in one place.
• Not run. I didn't run the Cucumber suite, which needs MongoDB and a JDK. I only read the code. The PR description says the suite wasn't run locally either, so CI is the only check.

Verdict

Request changes. Drop the duplicate guard, or explain what gap in cygwin_check it fills.
· branch feature/block-cygwin-install

[chloe41427]

Closing this - Claude's review is right: app/views/includes/cygwin_check.scala.txt already exists on master and is included at the top of both install_stable.scala.txt and install_beta.scala.txt. It already refuses Cygwin installs, points users to WSL2, and exits 1, so #75 is already satisfied. The guard added here runs after that check and would be dead code.

If a real gap in the existing cygwin_check surfaces, we'll widen the uname pattern in that one include rather than adding a second check.