mcp: surface specific TOKEN_FILE errors + link follow-ups (#2934)
Self-review of #2935 turned up two real defects: 1. Stale README issue references — the build_runtime_package.py README template said "(issue #2934 follow-up)" twice, but the marketplace-plugin and `doctor` items now have dedicated tracking issues. Updated to point at #2936 and #2937 respectively. 2. Silent fallthrough on broken MOLECULE_WORKSPACE_TOKEN_FILE — when an operator EXPLICITLY pointed TOKEN_FILE at a path that didn't exist / wasn't readable / was blank / contained internal whitespace, the resolver silently returned the generic "set one of these three vars" error. That's exactly the silent failure mode #2934 flagged ("a new user has no chance"). Refactor `_read_token_from_file_env` to return `(token, error)`; surface the SPECIFIC failure when the operator's intent was clearly the file path. Skip the CONFIGS_DIR fallback in that case so the operator's config bug isn't masked by a different source happening to work. Adds 2 renames + 2 new tests in test_mcp_cli_split.py: - test_missing_file_returns_specific_error (asserts "does not exist") - test_empty_file_returns_specific_error (asserts "is empty") - test_multi_line_file_rejected (asserts "internal whitespace") - test_token_file_error_skips_configs_dir_fallback (asserts a valid CONFIGS_DIR/.auth_token does NOT silently rescue a broken TOKEN_FILE) All 81 mcp_cli + mcp_cli_multi_workspace + mcp_cli_split tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
1ad107cc15
commit
da9061c131
@ -373,8 +373,9 @@ hold:
|
|||||||
non-plugin-sourced server, which Claude Code rejects with
|
non-plugin-sourced server, which Claude Code rejects with
|
||||||
`channel_enable requires a marketplace plugin`. Until the
|
`channel_enable requires a marketplace plugin`. Until the
|
||||||
official `moleculesai/claude-code-plugin` marketplace lands
|
official `moleculesai/claude-code-plugin` marketplace lands
|
||||||
(issue #2934 follow-up), operators who want push must scaffold
|
(tracking [#2936](https://github.com/Molecule-AI/molecule-core/issues/2936)),
|
||||||
their own local marketplace under
|
operators who want push must scaffold their own local marketplace
|
||||||
|
under
|
||||||
`~/.claude/marketplaces/molecule-local/` containing a
|
`~/.claude/marketplaces/molecule-local/` containing a
|
||||||
`marketplace.json` + `plugin.json` that points at this wheel.
|
`marketplace.json` + `plugin.json` that points at this wheel.
|
||||||
3. **Claude Code is launched with the dev-channels flag** — pass
|
3. **Claude Code is launched with the dev-channels flag** — pass
|
||||||
@ -384,8 +385,9 @@ hold:
|
|||||||
|
|
||||||
Symptom of any condition failing: messages arrive but only via the
|
Symptom of any condition failing: messages arrive but only via the
|
||||||
poll path (every ~1–60s), not real-time. There's currently no
|
poll path (every ~1–60s), not real-time. There's currently no
|
||||||
diagnostic surfaced — `molecule-mcp doctor` (issue #2934 follow-up)
|
diagnostic surfaced — `molecule-mcp doctor` (tracking
|
||||||
is planned.
|
[#2937](https://github.com/Molecule-AI/molecule-core/issues/2937)) is
|
||||||
|
planned.
|
||||||
|
|
||||||
If you don't need real-time push, the default poll path works
|
If you don't need real-time push, the default poll path works
|
||||||
universally with no extra setup; both modes converge on the same
|
universally with no extra setup; both modes converge on the same
|
||||||
|
|||||||
@ -112,7 +112,18 @@ def resolve_workspaces() -> tuple[list[tuple[str, str]], list[str]]:
|
|||||||
# fallback predates this and stays for in-container runtimes.
|
# fallback predates this and stays for in-container runtimes.
|
||||||
tok = os.environ.get("MOLECULE_WORKSPACE_TOKEN", "").strip()
|
tok = os.environ.get("MOLECULE_WORKSPACE_TOKEN", "").strip()
|
||||||
if not tok:
|
if not tok:
|
||||||
tok = _read_token_from_file_env()
|
tok, tf_err = _read_token_from_file_env()
|
||||||
|
if tf_err:
|
||||||
|
# Operator explicitly pointed TOKEN_FILE somewhere — surface
|
||||||
|
# the SPECIFIC failure (path doesn't exist, isn't readable,
|
||||||
|
# or holds a blank file) instead of falling through to the
|
||||||
|
# generic "set one of these three vars" message. Otherwise
|
||||||
|
# they get exactly the silent failure mode #2934 flagged
|
||||||
|
# ("a new user has no chance"). Skip the CONFIGS_DIR
|
||||||
|
# fallback in this case — the operator's intent is clearly
|
||||||
|
# to use the file path; deferring to a different source
|
||||||
|
# would mask their config error.
|
||||||
|
return [], [tf_err]
|
||||||
if not tok:
|
if not tok:
|
||||||
tok = read_token_file()
|
tok = read_token_file()
|
||||||
if not tok:
|
if not tok:
|
||||||
@ -123,26 +134,62 @@ def resolve_workspaces() -> tuple[list[tuple[str, str]], list[str]]:
|
|||||||
return [(wsid, tok)], []
|
return [(wsid, tok)], []
|
||||||
|
|
||||||
|
|
||||||
def _read_token_from_file_env() -> str:
|
def _read_token_from_file_env() -> tuple[str, str]:
|
||||||
"""Read the token from the file path in MOLECULE_WORKSPACE_TOKEN_FILE.
|
"""Read the token from the file path in MOLECULE_WORKSPACE_TOKEN_FILE.
|
||||||
|
|
||||||
Returns "" on:
|
Returns ``(token, error)``:
|
||||||
- env var unset / blank
|
* env var unset/blank → ``("", "")`` — caller falls through silently
|
||||||
- file not found, unreadable, or empty
|
to the next source; the operator didn't ask for this path.
|
||||||
- any OSError on read
|
* file open/read fails (missing, permission denied, decode error)
|
||||||
|
→ ``("", "<specific error>")`` — caller surfaces it directly.
|
||||||
Empty-on-failure (rather than raising) lets the resolver fall through
|
The operator EXPLICITLY pointed at this path, so a generic
|
||||||
to the CONFIGS_DIR fallback. The caller surfaces the combined "no
|
fallthrough error would mask their config bug (#2934).
|
||||||
token" error if every source is empty.
|
* file is blank → ``("", "<blank file error>")`` — same reasoning.
|
||||||
|
* file read returns junk with internal whitespace/newlines (e.g.
|
||||||
|
a CSV cell, accidental multi-token paste) → ``("", "<error>")``
|
||||||
|
rather than concatenating into a malformed bearer that 401s
|
||||||
|
against the platform with no context.
|
||||||
|
* happy path → ``("<token>", "")``.
|
||||||
"""
|
"""
|
||||||
path = os.environ.get("MOLECULE_WORKSPACE_TOKEN_FILE", "").strip()
|
path = os.environ.get("MOLECULE_WORKSPACE_TOKEN_FILE", "").strip()
|
||||||
if not path:
|
if not path:
|
||||||
return ""
|
return "", ""
|
||||||
try:
|
try:
|
||||||
with open(path, encoding="utf-8") as fh:
|
with open(path, encoding="utf-8") as fh:
|
||||||
return fh.read().strip()
|
raw = fh.read()
|
||||||
except OSError:
|
except FileNotFoundError:
|
||||||
return ""
|
return "", (
|
||||||
|
f"MOLECULE_WORKSPACE_TOKEN_FILE points to {path!r} which "
|
||||||
|
f"does not exist"
|
||||||
|
)
|
||||||
|
except PermissionError:
|
||||||
|
return "", (
|
||||||
|
f"MOLECULE_WORKSPACE_TOKEN_FILE={path!r} is not readable "
|
||||||
|
f"(permission denied)"
|
||||||
|
)
|
||||||
|
except OSError as exc:
|
||||||
|
return "", (
|
||||||
|
f"MOLECULE_WORKSPACE_TOKEN_FILE={path!r} could not be read: "
|
||||||
|
f"{exc}"
|
||||||
|
)
|
||||||
|
except UnicodeDecodeError:
|
||||||
|
return "", (
|
||||||
|
f"MOLECULE_WORKSPACE_TOKEN_FILE={path!r} is not valid UTF-8"
|
||||||
|
)
|
||||||
|
tok = raw.strip()
|
||||||
|
if not tok:
|
||||||
|
return "", (
|
||||||
|
f"MOLECULE_WORKSPACE_TOKEN_FILE={path!r} is empty"
|
||||||
|
)
|
||||||
|
# Reject tokens with internal whitespace — a CSV cell or accidental
|
||||||
|
# multi-token paste would otherwise become a malformed bearer that
|
||||||
|
# 401s against the platform with no diagnostic.
|
||||||
|
if any(ch.isspace() for ch in tok):
|
||||||
|
return "", (
|
||||||
|
f"MOLECULE_WORKSPACE_TOKEN_FILE={path!r} contains internal "
|
||||||
|
f"whitespace — expected a single token"
|
||||||
|
)
|
||||||
|
return tok, ""
|
||||||
|
|
||||||
|
|
||||||
def print_missing_env_help(missing: list[str], have_token_file: bool) -> None:
|
def print_missing_env_help(missing: list[str], have_token_file: bool) -> None:
|
||||||
|
|||||||
@ -275,26 +275,68 @@ class TestTokenFileEnv:
|
|||||||
out, _ = mcp_workspace_resolver.resolve_workspaces()
|
out, _ = mcp_workspace_resolver.resolve_workspaces()
|
||||||
assert out == [("ws-1", "inline-tok")]
|
assert out == [("ws-1", "inline-tok")]
|
||||||
|
|
||||||
def test_missing_file_falls_through_to_error(self, monkeypatch, tmp_path):
|
def test_missing_file_returns_specific_error(self, monkeypatch, tmp_path):
|
||||||
# Pointed at a non-existent path — resolver should return the
|
# Operator EXPLICITLY pointed TOKEN_FILE at a non-existent path —
|
||||||
# combined "no token" error, NOT crash.
|
# surface the SPECIFIC failure (not the generic "set one of these
|
||||||
|
# three vars" message). Otherwise they hit the silent failure mode
|
||||||
|
# #2934 flagged ("a new user has no chance").
|
||||||
|
bad_path = tmp_path / "does-not-exist"
|
||||||
monkeypatch.setenv("WORKSPACE_ID", "ws-1")
|
monkeypatch.setenv("WORKSPACE_ID", "ws-1")
|
||||||
monkeypatch.setenv(
|
monkeypatch.setenv("MOLECULE_WORKSPACE_TOKEN_FILE", str(bad_path))
|
||||||
"MOLECULE_WORKSPACE_TOKEN_FILE", str(tmp_path / "does-not-exist")
|
|
||||||
)
|
|
||||||
out, errors = mcp_workspace_resolver.resolve_workspaces()
|
out, errors = mcp_workspace_resolver.resolve_workspaces()
|
||||||
assert out == []
|
assert out == []
|
||||||
assert any("MOLECULE_WORKSPACE_TOKEN_FILE" in e for e in errors)
|
assert len(errors) == 1
|
||||||
|
assert "MOLECULE_WORKSPACE_TOKEN_FILE" in errors[0]
|
||||||
|
assert "does not exist" in errors[0]
|
||||||
|
assert str(bad_path) in errors[0]
|
||||||
|
|
||||||
def test_empty_file_falls_through_to_error(self, monkeypatch, tmp_path):
|
def test_empty_file_returns_specific_error(self, monkeypatch, tmp_path):
|
||||||
# File exists but is blank — same shape as no token at all.
|
# Blank file — operator's intent was clearly the file path, so a
|
||||||
|
# generic "no token" error would mask their config bug.
|
||||||
token_path = tmp_path / "empty.txt"
|
token_path = tmp_path / "empty.txt"
|
||||||
token_path.write_text("")
|
token_path.write_text("")
|
||||||
monkeypatch.setenv("WORKSPACE_ID", "ws-1")
|
monkeypatch.setenv("WORKSPACE_ID", "ws-1")
|
||||||
monkeypatch.setenv("MOLECULE_WORKSPACE_TOKEN_FILE", str(token_path))
|
monkeypatch.setenv("MOLECULE_WORKSPACE_TOKEN_FILE", str(token_path))
|
||||||
out, errors = mcp_workspace_resolver.resolve_workspaces()
|
out, errors = mcp_workspace_resolver.resolve_workspaces()
|
||||||
assert out == []
|
assert out == []
|
||||||
assert errors # at least one combined error message
|
assert len(errors) == 1
|
||||||
|
assert "MOLECULE_WORKSPACE_TOKEN_FILE" in errors[0]
|
||||||
|
assert "is empty" in errors[0]
|
||||||
|
|
||||||
|
def test_multi_line_file_rejected(self, monkeypatch, tmp_path):
|
||||||
|
# CSV cell or accidental multi-token paste — would otherwise become
|
||||||
|
# a malformed bearer that 401s against the platform with no
|
||||||
|
# diagnostic. Reject upfront with a specific error.
|
||||||
|
token_path = tmp_path / "junk.txt"
|
||||||
|
token_path.write_text("tok-a tok-b\n")
|
||||||
|
monkeypatch.setenv("WORKSPACE_ID", "ws-1")
|
||||||
|
monkeypatch.setenv("MOLECULE_WORKSPACE_TOKEN_FILE", str(token_path))
|
||||||
|
out, errors = mcp_workspace_resolver.resolve_workspaces()
|
||||||
|
assert out == []
|
||||||
|
assert len(errors) == 1
|
||||||
|
assert "internal whitespace" in errors[0]
|
||||||
|
|
||||||
|
def test_token_file_error_skips_configs_dir_fallback(
|
||||||
|
self, monkeypatch, tmp_path
|
||||||
|
):
|
||||||
|
# When TOKEN_FILE is explicitly set but broken, do NOT fall through
|
||||||
|
# to a valid CONFIGS_DIR/.auth_token — the operator's intent is
|
||||||
|
# clearly to use the file path; deferring to a different source
|
||||||
|
# would mask their config error.
|
||||||
|
configs_dir = tmp_path / "configs"
|
||||||
|
configs_dir.mkdir()
|
||||||
|
(configs_dir / ".auth_token").write_text("configs-tok")
|
||||||
|
monkeypatch.setenv("CONFIGS_DIR", str(configs_dir))
|
||||||
|
monkeypatch.setenv("WORKSPACE_ID", "ws-1")
|
||||||
|
monkeypatch.setenv(
|
||||||
|
"MOLECULE_WORKSPACE_TOKEN_FILE", str(tmp_path / "missing")
|
||||||
|
)
|
||||||
|
out, errors = mcp_workspace_resolver.resolve_workspaces()
|
||||||
|
assert out == []
|
||||||
|
# Specific TOKEN_FILE error — not the generic "no token" fallback
|
||||||
|
# and crucially not the silent success of using configs-tok.
|
||||||
|
assert len(errors) == 1
|
||||||
|
assert "does not exist" in errors[0]
|
||||||
|
|
||||||
def test_blank_env_var_treated_as_unset(self, monkeypatch):
|
def test_blank_env_var_treated_as_unset(self, monkeypatch):
|
||||||
# Empty string is treated as "not set" — common pitfall when
|
# Empty string is treated as "not set" — common pitfall when
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user