diff --git a/.github/workflows/changelog.yml b/.github/workflows/changelog.yml index e648c10..f0886c4 100644 --- a/.github/workflows/changelog.yml +++ b/.github/workflows/changelog.yml @@ -18,6 +18,14 @@ jobs: with: python-version: "3.12" + # The checker parses each fragment with markdown-it-py and refuses to fall + # back to text scanning when it is absent -- it reports `skipped` and exits + # non-zero rather than claiming anything. So the job has to install it; the + # gate was added without this step and its first real run failed on the + # parser rather than on a fragment. + - name: Install the fragment parser + run: python3 -m pip install --disable-pip-version-check markdown-it-py + # Paths are passed explicitly. The script derives its default root from its # own location, which assumes it lives in .github/scripts/ -- it does not # here, and it will not in any repo this plugin is pointed at either. diff --git a/bin/oss-workspace b/bin/oss-workspace index d296ef2..d4c8f47 100755 --- a/bin/oss-workspace +++ b/bin/oss-workspace @@ -11,15 +11,23 @@ # # ln -sf "$PWD/bin/oss-workspace" ~/.local/bin/oss-workspace # -# Two things about the session that are not the default: +# Three things about the session are not the default, and each was measured +# against claude 2.1.219 rather than assumed: # # * Without the development-channel flag, watch pollers still spawn and still -# emit and nothing reads them — a board that looks armed and delivers nothing. -# Check `supertool 'channel:health'` before trusting a green board. -# * `claude` reads only its FIRST positional as the prompt and silently ignores -# later ones, and a variadic option swallows whatever follows it. So the -# prompt is appended ONLY when there is nothing else to pass, and when it is -# not, that is said rather than dropped. +# emit and nothing reads them — a board that looks armed and delivers +# nothing. Check `supertool 'channel:health'` before trusting a green board. +# * That flag resolves CONFIGURED MCP servers only, so the consumer is +# registered at local scope here. Naming a server that is not registered +# refuses the launch outright: +# server:oss-channel - no MCP server configured with that name +# So the flag is passed ONLY when the registration held. A session with no +# channel is worth more than no session. +# * `claude` reads only its FIRST positional as the prompt, and a variadic +# option swallows whatever follows it. The prompt therefore goes FIRST and +# the flag LAST; putting the prompt after the flag ends the launch with +# --dangerously-load-development-channels entries must be tagged: /oss:tick +# which is exactly how this script failed on its first real use. set -eu # A user's CDPATH can make `cd relative` land somewhere else entirely and print @@ -28,7 +36,11 @@ set -eu CDPATH= CHANNEL_FLAG="--dangerously-load-development-channels" -CHANNEL_SERVER="server:claude-channel" + +# Deliberately not `claude-channel`: the supertool plugin's own entry keeps that +# name and keeps failing on an unset CLAUDE_PLUGIN_ROOT, and two servers sharing +# one name is a collision this script cannot decide the winner of. +CHANNEL_SERVER="oss-channel" # `dirname "$0"` on a symlink gives the link's directory, not the checkout. # `readlink -f` is not portable to macOS's stock readlink, so walk it. @@ -43,8 +55,7 @@ done # `dirname` splits on `/` only. Under Git Bash `$0` arrives as # `D:\a\repo\bin\oss-workspace`, so dirname answers `.` and the root resolves # against the caller's directory instead — confidently, and wrongly. Strip either -# separator ourselves. The same shape failed the Windows leg in scripts/doctor.sh; -# here nothing downstream reads the value yet, which is why it was silent. +# separator ourselves. The same shape failed the Windows leg in scripts/doctor.sh. self_dir=${self%/*} [ "$self_dir" = "$self" ] && self_dir=${self%\\*} [ "$self_dir" = "$self" ] && self_dir=. @@ -93,8 +104,167 @@ elif ! grep -q radar "$repo_root/.supertool.json" 2>/dev/null; then echo "oss-workspace: .supertool.json declares no radar tiers, so the channel is open and nothing publishes to it. Check delivery with supertool 'channel:health' before trusting a green board." >&2 fi +# `python3` by name is a POSIX assumption: Windows ships `python` and no +# `python3`, and its App Execution Alias makes a `python` that EXISTS and opens a +# store page instead of running. So each candidate is proved by running it and +# comparing a sentinel, the same route scripts/doctor.sh already takes. +python_bin= +for candidate in python3 python; do + if [ "$("$candidate" -c "print(42)" 2>/dev/null)" = "42" ]; then + python_bin=$candidate + break + fi +done +if [ -z "$python_bin" ]; then + echo "oss-workspace: no working python was found (tried python3, python), so" \ + "neither the channel name nor the consumer path could be read. This" \ + "session opens without a channel." >&2 +fi + +# The channel NAME is this repo's state and is read from its own .supertool.json, +# then exported — a stdio MCP server the harness spawns inherits this process's +# environment, which is what carries the name to the consumer. Exactly one +# distinct name is exported; disagreeing op blocks are a state this script cannot +# resolve, so it says so and exports nothing. +# The guard is an `if` and not `[ -n "$x" ] && ...` inside the substitution: a +# heredoc opened by the right-hand side of `&&` and then followed by `|| true` is +# a syntax error under bash -- `syntax error near unexpected token ||` -- while +# macOS's /bin/sh parsed it and every local run stayed green. +watch_name="" +if [ -n "$python_bin" ]; then +watch_name=$("$python_bin" - "$repo_root/.supertool.json" <<'READ_NAME' || true +import json, sys + +try: + with open(sys.argv[1], encoding="utf-8") as handle: + doc = json.load(handle) +except (OSError, ValueError) as err: + sys.stderr.write( + "oss-workspace: %s could not be read (%s), so no channel name was exported " + "and this session is on the default channel\n" % (sys.argv[1], type(err).__name__)) + raise SystemExit(0) + +ops = doc.get("ops") if isinstance(doc, dict) else None +names = sorted({ + block["watch_name"] + for block in (ops or {}).values() + if isinstance(block, dict) and isinstance(block.get("watch_name"), str) + and block["watch_name"] +}) +if len(names) == 1: + print(names[0]) +elif names: + sys.stderr.write( + "oss-workspace: op blocks disagree about watch_name (%s) — none exported. " + "The ops themselves are on different channels; fix that file, then run " + "supertool 'channel:health'\n" % ", ".join(names)) +READ_NAME +) +fi + +# An exported value is the one a running poller already captured, and moving the +# paths underneath a live fleet is a documented failure. So an export wins — and +# both values are named, because a name losing quietly is the other half of it. +if [ -n "${SUPERTOOL_WATCH_NAME:-}" ]; then + if [ -n "$watch_name" ] && [ "$watch_name" != "$SUPERTOOL_WATCH_NAME" ]; then + echo "oss-workspace: SUPERTOOL_WATCH_NAME is already exported as" \ + "${SUPERTOOL_WATCH_NAME} and wins; $repo_root/.supertool.json declares" \ + "$watch_name. The pollers this session spawns follow the export; ops" \ + "already running elsewhere do not. supertool 'channel:health'" >&2 + fi +elif [ -n "$watch_name" ]; then + export SUPERTOOL_WATCH_NAME="$watch_name" +fi + +# The consumer ships with the SUPERTOOL plugin, not with this one, and its path +# is resolved from installed_plugins.json rather than by globbing the cache: the +# cache keeps every version ever installed, so a glob answers with whichever +# sorts last and reports a version this session is not running. That exact bug +# shipped once in scripts/doctor.py. +channel_script="" +if [ -n "$python_bin" ]; then +channel_script=$("$python_bin" - <<'FIND_CONSUMER' || true +import json, os, sys + +# Each dead end names itself. "Not found" with no reason is the shape that +# reports a confident wrong answer -- an absent registry, a supertool that is not +# installed and an install path that no longer holds the consumer are three +# different problems with three different remedies, and one message for all three +# sends everybody to the wrong one. +registry = os.path.expanduser("~/.claude/plugins/installed_plugins.json") +try: + with open(registry, encoding="utf-8") as handle: + doc = json.load(handle) +except (OSError, ValueError) as err: + sys.stderr.write("oss-workspace: %s could not be read (%s)\n" % (registry, type(err).__name__)) + raise SystemExit(0) + +installs = [ + entry.get("installPath") + for key, entries in (doc.get("plugins") or {}).items() + if key.split("@")[0] == "supertool" + for entry in entries or [] + if entry.get("installPath") +] +if not installs: + sys.stderr.write( + "oss-workspace: %s lists no supertool install, so the channel consumer it " + "ships has nowhere to be read from\n" % registry) + raise SystemExit(0) + +for path in installs: + candidate = os.path.join(path, "notifiers", "claude-channel", "channel.ts") + if os.path.isfile(candidate): + print(candidate) + raise SystemExit(0) + +sys.stderr.write( + "oss-workspace: supertool is installed at %s but holds no " + "notifiers/claude-channel/channel.ts\n" % ", ".join(installs)) +FIND_CONSUMER +) +fi + +channel_ready=0 +if [ -z "$channel_script" ]; then + echo "oss-workspace: the supertool plugin's channel consumer was not found in" \ + "installed_plugins.json, so this session opens WITHOUT the channel flag." \ + "Naming an unregistered server refuses the launch outright, which is worse" \ + "than a session with no board." >&2 +elif ! command -v bun >/dev/null 2>&1; then + echo "oss-workspace: bun is not on PATH, so the channel consumer cannot start." \ + "Opening without the channel flag." >&2 +else + # Registered at LOCAL scope rather than passed as `--mcp-config`: a server + # loaded from --mcp-config does start and does bind the socket, but the + # channel resolver reads CONFIGURED servers only and refuses it by name. Local + # scope is per-project and private, so it does not ship, and the path is + # absolute so it cannot depend on CLAUDE_PLUGIN_ROOT being set. + # + # Idempotent: `claude mcp get` is the test, and a re-add on every launch would + # churn the user's config for nothing. + if claude mcp get "$CHANNEL_SERVER" >/dev/null 2>&1; then + channel_ready=1 + elif claude mcp add -s local "$CHANNEL_SERVER" bun "$channel_script" >/dev/null 2>&1; then + channel_ready=1 + echo "oss-workspace: registered MCP server $CHANNEL_SERVER at local scope" \ + "(private to $repo_root) pointing at $channel_script. Remove it with" \ + "claude mcp remove $CHANNEL_SERVER -s local" >&2 + else + echo "oss-workspace: could not register the MCP server $CHANNEL_SERVER, so" \ + "this session has no channel consumer. Opening without the flag;" \ + "supertool 'channel:health' says the same from inside." >&2 + fi +fi + if [ "$#" -eq 0 ]; then - exec claude "$CHANNEL_FLAG" "$CHANNEL_SERVER" "$prompt" + # The prompt goes FIRST and the variadic flag LAST. A positional after the + # flag is read as one of its values, and claude refuses the launch rather than + # dropping it. + if [ "$channel_ready" -eq 1 ]; then + exec claude "$prompt" "$CHANNEL_FLAG" "server:$CHANNEL_SERVER" + fi + exec claude "$prompt" fi # The third state. Placing our prompt after the caller's arguments hands `claude` @@ -103,4 +273,10 @@ fi echo "oss-workspace: arguments were passed through, so $prompt was NOT appended —" \ "claude reads only its first positional as the prompt. Run $prompt inside the" \ "session, or start it with no arguments." >&2 -exec claude "$CHANNEL_FLAG" "$CHANNEL_SERVER" "$@" + +# The flag trails for the same reason it does above, and it terminates any +# variadic option the caller passed, being `-`-prefixed. +if [ "$channel_ready" -eq 1 ]; then + exec claude "$@" "$CHANNEL_FLAG" "server:$CHANNEL_SERVER" +fi +exec claude "$@" diff --git a/changelog.d/13.added.md b/changelog.d/13.added.md new file mode 100644 index 0000000..623293c --- /dev/null +++ b/changelog.d/13.added.md @@ -0,0 +1,5 @@ +- `bin/oss-workspace` registers the channel consumer itself, at local MCP scope, resolving its path + from `installed_plugins.json` rather than by globbing the plugin cache — a glob answers with + whichever version sorts last, which is a version the session is not running. The channel flag is + passed only when that registration held: naming an unregistered server refuses the launch + outright, and a session with no board beats no session (#13). diff --git a/changelog.d/13.fixed.md b/changelog.d/13.fixed.md new file mode 100644 index 0000000..7d42042 --- /dev/null +++ b/changelog.d/13.fixed.md @@ -0,0 +1,10 @@ +- `bin/oss-workspace` put its prompt after `--dangerously-load-development-channels`, which is + variadic and swallowed it, so the first real use of the launcher died on + `entries must be tagged: /oss:tick` instead of opening anything. The prompt goes first now and the + flag last (#13). +- The launcher looked for `python3` by name, which Windows does not ship, so the channel name and + the consumer path were both silently unreadable there. Each candidate is now proved by running it + (#13). +- `verify_test_command` read only 127 as "command not found", which is the POSIX shell's code. + cmd.exe answers 9009, so every missing runner on Windows reported as a failing suite — sending + someone to debug a suite that was never installed (#13). diff --git a/scripts/oss_config.py b/scripts/oss_config.py index 14d4c11..b1a6258 100644 --- a/scripts/oss_config.py +++ b/scripts/oss_config.py @@ -14,6 +14,8 @@ import json import os import re +import shlex +import shutil import subprocess import sys from pathlib import Path @@ -329,6 +331,24 @@ def verify_test_command(command, cwd, timeout=120): if not command: return {"state": "none", "detail": "no test command detected; nothing to verify"} + # The runner is resolved before anything runs, because the shell's own + # "command not found" code is not portable: POSIX shells answer 127, cmd.exe + # answers 9009, and on a GitHub Windows runner it answered neither -- so a + # runner that was never installed reported as a suite that ran and failed, + # which is the one confusion these states exist to prevent. Only for a plain + # command: with an operator in it the first word is not the whole story, and a + # shell builtin resolves to no file at all. + if not any(token in command for token in ("&&", "||", "|", ";", ">", "<", "$(", "`")): + try: + words = shlex.split(command, posix=os.name != "nt") + except ValueError: + words = [] + if words and shutil.which(words[0]) is None: + return { + "state": "not-found", + "detail": "{!r}: {!r} is not on PATH".format(command, words[0]), + } + try: done = subprocess.run( command, @@ -352,9 +372,11 @@ def verify_test_command(command, cwd, timeout=120): return {"state": "ok", "detail": "{!r} ran and passed".format(command)} tail = (done.stdout or "").strip().splitlines()[-1:] or [""] - # 127 is the shell's own "command not found", which is a different problem from a - # suite that ran and failed. - if done.returncode == 127: + # 127 is the POSIX shell's own "command not found", and 9009 is cmd.exe's, which + # is a different problem from a suite that ran and failed. Reading only 127 makes + # every missing runner on Windows report as a failing suite -- the exact confusion + # between "install this" and "fix this" the states exist to prevent. + if done.returncode in (127, 9009): return { "state": "not-found", "detail": "{!r}: command not found ({})".format(command, tail[0]), diff --git a/tests/test_oss_config.py b/tests/test_oss_config.py index da2c73e..24cc9c3 100644 --- a/tests/test_oss_config.py +++ b/tests/test_oss_config.py @@ -6,6 +6,7 @@ """ import json +import subprocess import sys from pathlib import Path @@ -171,9 +172,15 @@ def test_probe_output_validates(): # --------------------------------------------------------------- test verification -PASSES = "python3 -c pass" -FAILS = "python3 -c 'raise SystemExit(3)'" -SLEEPS = "python3 -c 'import time; time.sleep(5)'" +# The interpreter running the suite, not the name `python3`: Windows ships +# `python` and no `python3`, so the hardcoded name was not a slow suite or a +# broken one but a command that does not exist -- which made the timeout and +# not-found cases both report `failed` and hid the states they exist to tell +# apart. Quoting matters too: cmd.exe does not strip single quotes. +PY = subprocess.list2cmdline([sys.executable]) +PASSES = PY + " -c pass" +FAILS = PY + ' -c "raise SystemExit(3)"' +SLEEPS = PY + ' -c "import time; time.sleep(5)"' def test_a_working_command_verifies_ok(tmp_path): diff --git a/tests/test_workspace_launcher.py b/tests/test_workspace_launcher.py index 85663ea..734868b 100644 --- a/tests/test_workspace_launcher.py +++ b/tests/test_workspace_launcher.py @@ -9,10 +9,13 @@ launcher tested by reading it is a launcher nobody has run. """ +import json import os +import re import shutil import stat import subprocess +import sys from pathlib import Path import pytest @@ -28,30 +31,93 @@ ) +def _executable(path, text): + path.write_text(text, encoding="utf-8") + path.chmod(path.stat().st_mode | stat.S_IEXEC | stat.S_IXGRP | stat.S_IXOTH) + return path + + def _stub_claude(bindir, argv_log): - """A `claude` that records argv and exits 0, so exec is observable.""" - path = bindir / "claude" - path.write_text( - '#!/bin/sh\nfor a in "$@"; do printf "%s\\n" "$a" >> "{}"; done\nexit 0\n'.format(argv_log), + """A `claude` that records argv and exits 0, so exec is observable. + + `claude mcp ...` is answered rather than recorded: those calls are the script + probing and configuring, not the session it opens, and mixing them into the + argv log makes every assertion about the launch read the wrong list. The probe + reports "not registered", so the registration path is the one under test. + """ + return _executable( + bindir / "claude", + '#!/bin/sh\n' + 'if [ "${1:-}" = "mcp" ]; then\n' + ' [ "${2:-}" = "add" ] && exit 0\n' + ' exit 1\n' + 'fi\n' + 'for a in "$@"; do printf "%s\\n" "$a" >> "' + str(argv_log) + '"; done\n' + 'exit 0\n', + ) + + +def _with_channel_consumer(home, bindir): + """Plant what the script needs to register a channel: a `bun`, and a supertool + plugin whose install path holds the consumer. + + The path is read from installed_plugins.json rather than globbed out of the + cache, so the fixture writes that registry -- a glob answers with whichever + version sorts last, and that is a version the session is not running. + """ + _executable(bindir / "bun", "#!/bin/sh\nexit 0\n") + install = home / ".claude" / "plugins" / "cache" / "dpt-plugins" / "supertool" / "9.9.9" + consumer = install / "notifiers" / "claude-channel" / "channel.ts" + consumer.parent.mkdir(parents=True) + consumer.write_text("// stub\n", encoding="utf-8") + registry = home / ".claude" / "plugins" / "installed_plugins.json" + registry.write_text( + json.dumps({ + "version": 2, + "plugins": { + "supertool@dpt-plugins": [ + {"scope": "user", "installPath": str(install), "version": "9.9.9"} + ] + }, + }), encoding="utf-8", ) - path.chmod(path.stat().st_mode | stat.S_IEXEC | stat.S_IXGRP | stat.S_IXOTH) - return path + return consumer -def run(cwd, args=(), with_claude=True): +def run(cwd, args=(), with_claude=True, with_channel=False): bindir = Path(cwd) / "_stubbin" bindir.mkdir(exist_ok=True) argv_log = Path(cwd) / "argv.txt" if with_claude: _stub_claude(bindir, argv_log) + # HOME is pinned for the same reason PATH is: the consumer is looked up under + # the user's real ~/.claude, so an unpinned HOME decides the channel assertions + # by whether the developer running the suite happens to have supertool + # installed -- green on the author's machine, red on a contributor's. + home = Path(cwd) / "_home" + (home / ".claude" / "plugins").mkdir(parents=True, exist_ok=True) + if with_channel: + _with_channel_consumer(home, bindir) + env = dict(os.environ) + env["HOME"] = str(home) + env["USERPROFILE"] = str(home) + env.pop("SUPERTOOL_WATCH_NAME", None) # Minimal PATH, deliberately: with the real claude reachable, the "missing # claude" case found it and EXECUTED it -- a test suite that launches a live - # agent session in a temp directory. Only the stub and the system utilities - # the script needs are on PATH here. - env["PATH"] = os.pathsep.join([str(bindir), "/usr/bin", "/bin"]) + # agent session in a temp directory. Only the stub, the interpreter and the + # system utilities the script needs are on PATH here. + # + # The interpreter's directory is on it because the launcher needs a python to + # read the channel name and find the consumer. `/usr/bin` and `/bin` are Git + # Bash's on Windows and hold no python at all, so pinning to those alone + # starved the launcher of one -- it then said so correctly, and the channel + # assertions failed against a fixture problem wearing a product bug's clothes. + env["PATH"] = os.pathsep.join( + [str(bindir), str(Path(sys.executable).parent), "/usr/bin", "/bin"] + ) done = subprocess.run( [BASH, str(LAUNCHER), *args], cwd=str(cwd), @@ -121,12 +187,46 @@ def test_not_appending_the_prompt_is_said_out_loud(tmp_path): assert "not appended" in done.stderr.lower() -def test_the_watch_channel_flag_is_passed(tmp_path): +def test_the_watch_channel_flag_is_passed_once_the_consumer_is_registered(tmp_path): """Without it the pollers still spawn and still emit and nothing reads them: a board that looks armed and delivers nothing. """ - _, argv = run(_repo(tmp_path)) + _, argv = run(_repo(tmp_path), with_channel=True) assert any("development-channels" in a for a in argv) + assert "server:oss-channel" in argv + + +def test_the_prompt_precedes_the_channel_flag(tmp_path): + """The flag is variadic, so a positional written after it is read as one of its + values and the launch is REFUSED, not degraded: + + --dangerously-load-development-channels entries must be tagged: /oss:tick + + which is how this script failed the first time it was run for real. + """ + _, argv = run(_repo(tmp_path), with_channel=True) + assert argv.index("/oss:tick") < argv.index("--dangerously-load-development-channels") + + +def test_no_consumer_means_no_flag_rather_than_no_session(tmp_path): + """`--dangerously-load-development-channels server:NAME` resolves CONFIGURED + servers only and refuses the launch outright when the name is not one. So a + session that cannot have a channel opens without one and is told which half is + missing; the alternative is no session at all. + """ + done, argv = run(_repo(tmp_path)) + assert argv, done.stderr + assert not any("development-channels" in a for a in argv) + assert "channel consumer was not found" in done.stderr + + +def test_registering_the_consumer_is_said_out_loud(tmp_path): + """It writes to the user's MCP config. Something that edits your config without + saying so is something you cannot undo, so the removal command is named. + """ + done, _ = run(_repo(tmp_path), with_channel=True) + assert "registered MCP server oss-channel" in done.stderr + assert "claude mcp remove oss-channel -s local" in done.stderr def test_it_survives_being_run_through_a_symlink(tmp_path): @@ -141,7 +241,9 @@ def test_it_survives_being_run_through_a_symlink(tmp_path): bindir.mkdir(exist_ok=True) _stub_claude(bindir, repo / "argv.txt") env = dict(os.environ) - env["PATH"] = os.pathsep.join([str(bindir), "/usr/bin", "/bin"]) + env["PATH"] = os.pathsep.join( + [str(bindir), str(Path(sys.executable).parent), "/usr/bin", "/bin"] + ) done = subprocess.run( [BASH, str(link)], @@ -191,5 +293,11 @@ def test_the_script_is_posix_sh_not_bash(tmp_path): """It runs on whatever the user has, including Git Bash and a stock macOS shell.""" text = LAUNCHER.read_text(encoding="utf-8") assert text.startswith("#!/bin/sh") - for bashism in ("[[", "declare ", "local ", "function "): - assert bashism not in text, bashism + # Matched at statement position rather than as a substring. `local ` occurs + # inside `claude mcp add -s local ...` and inside prose about local scope, and + # a substring check calls both a bashism. A test that is wrong in the direction + # of stopping you is still wrong, and it gets edited around rather than heeded. + for bashism in ("declare", "local", "function"): + offender = re.search(r"^\s*%s\s" % bashism, text, re.MULTILINE) + assert offender is None, "%s: %r" % (bashism, offender.group(0)) + assert re.search(r"\[\[", text) is None, "[[ is bash test syntax"