diff --git a/.apm/skills/backend-configuration/SKILL.md b/.apm/skills/backend-configuration/SKILL.md index 2553ea2..f89eeb7 100644 --- a/.apm/skills/backend-configuration/SKILL.md +++ b/.apm/skills/backend-configuration/SKILL.md @@ -309,10 +309,10 @@ unchecked stack is not persisted, and the fix is the user's. An **environment switch** — "target prod", "clear the environment" — is a write of the `environment` field and nothing else: `odd_config_set {"environment": ""}` (kebab-case, never -`unknown`), or `{"environment": null}` to clear it. It selects which +`unknown` or `local`), or `{"environment": null}` to clear it. It selects which of the configured stack's entries the missions read (`## Check` step -2's effective entry) and changes no `stack_config` value; on the local -stack it is inert. A switch that names both ("switch to cloudwatch in +2's effective entry) and changes no `stack_config` value; the local +stack refuses it. A switch that names both ("switch to cloudwatch in prod") writes both fields in the one call. Like every path it ends at verification (step 5). diff --git a/docs/guide/plugin.md b/docs/guide/plugin.md index 21fde1e..fb3d928 100644 --- a/docs/guide/plugin.md +++ b/docs/guide/plugin.md @@ -564,7 +564,7 @@ than the local otel-lgtm container. | `odd_stack_status` | Probe whether it is up — and get the container's identity too: `image`, `created`/`started` timestamps, and its user-set `env` (credential-named values redacted to `null`; all four `null` when there is no container) — plus `daemon` (`"ok"`, or `"unreachable"` with `daemon_remedy`, the one-line remedy, when the Docker daemon does not answer or no `docker` binary is on PATH) | — | | `odd_stack_reset` | Wipe all stored telemetry and return a fresh, ready stack — the next run starts from a clean slate | `env` (optional) — always applies, the container is recreated; persisted/reapplied like `odd_stack_up` | | `odd_config_get` | Read the global configuration — stack backend, the configured deployment environment (`null` when none), local host ports, targeting values per stack and per environment (`stack_config`, keyed `` or `-`), custom stack declarations, and `effective`: the entry resolved for the configured stack and environment (`stack_config_key` and its `stack_config` — the environment's entry when one is persisted, the stack's plain one otherwise, never a merge) — and the installed `oddyssey-mcp` version | — | -| `odd_config_set` | Update it — a port change resets the stack so the new value applies right away | `config` — partial merge, e.g. `{"local": {"grafana_port": 3300}}`, `{"environment": "prod"}` (`null` clears it; inert on the local stack); `stack_config` keys are `` or `-` (kebab-case environment, never `unknown`; `local` takes no prefix), each accepting the stack's fields; inside `stack_config` and `custom`, `null` deletes a key or an entry; a stack outside the built-in list needs a `custom` declaration, never one that reads as `-` | +| `odd_config_set` | Update it — a port change resets the stack so the new value applies right away | `config` — partial merge, e.g. `{"local": {"grafana_port": 3300}}`, `{"environment": "prod"}` (`null` clears it; refused on the local stack, cleared by a switch to it); `stack_config` keys are `` or `-` (kebab-case environment, never `unknown` or `local`; `local` takes no prefix), each accepting the stack's fields; inside `stack_config` and `custom`, `null` deletes a key or an entry; a stack outside the built-in list needs a `custom` declaration, never one that reads as `-` or that a built-in ends in | The server is instrumented with OpenTelemetry and, by default, exports its own traces and metrics to the local stack (`http://localhost:4318`, OTLP diff --git a/integration-tests/mcp-server/test-config-surface.sh b/integration-tests/mcp-server/test-config-surface.sh index e9faa2e..2ca1204 100644 --- a/integration-tests/mcp-server/test-config-surface.sh +++ b/integration-tests/mcp-server/test-config-surface.sh @@ -6,7 +6,8 @@ # the caller (#228) is accepted, validated and removed, an # environment-prefixed stack_config key merges and deletes next to the # stack's plain one while the environment field selects the effective -# entry (#618), grafana persists its gcx context name and nothing else +# entry (#618) and refuses an environment on the local stack (#656), +# grafana persists its gcx context name and nothing else # (#619), and the tolerant read lists # hand-edited invalid values in invalid_ignored instead of crashing, # and the read carries the installed oddyssey-mcp version (#395). @@ -202,7 +203,7 @@ jq -e '.content[0].text | fromjson | .effective.environment == "dev" and .effect || { echo "ASSERTION FAILED: fallback to the plain entry did not apply" >&2; cat "$workdir/env-dev.json" >&2; exit 1; } step "an invalid prefix is rejected and writes nothing (#618)" -for key in "unknown-cloudwatch" "prod-nagios" "Prod-cloudwatch" "prod-local"; do +for key in "unknown-cloudwatch" "local-cloudwatch" "prod-nagios" "Prod-cloudwatch" "prod-local"; do mcp_call odd_config_set "config={\"stack_config\":{\"$key\":{}}}" > "$workdir/env-bad-key.json" || true grep -q "stack_config keys" "$workdir/env-bad-key.json" \ || { echo "ASSERTION FAILED: key $key was not rejected" >&2; cat "$workdir/env-bad-key.json" >&2; exit 1; } @@ -210,9 +211,11 @@ done mcp_call odd_config_set 'config={"stack_config":{"prod-cloudwatch":{"workspace":"x"}}}' > "$workdir/env-bad-field.json" || true grep -q "accepts only" "$workdir/env-bad-field.json" \ || { echo "ASSERTION FAILED: a field outside the stack's list was not rejected on a prefixed key" >&2; cat "$workdir/env-bad-field.json" >&2; exit 1; } -mcp_call odd_config_set 'config={"environment":"Prod"}' > "$workdir/env-bad-value.json" || true -grep -q "environment must be" "$workdir/env-bad-value.json" \ - || { echo "ASSERTION FAILED: an invalid environment was not rejected" >&2; cat "$workdir/env-bad-value.json" >&2; exit 1; } +for value in "Prod" "local"; do + mcp_call odd_config_set "config={\"environment\":\"$value\"}" > "$workdir/env-bad-value.json" || true + grep -q "environment must be" "$workdir/env-bad-value.json" \ + || { echo "ASSERTION FAILED: environment $value was not rejected" >&2; cat "$workdir/env-bad-value.json" >&2; exit 1; } +done mcp_call odd_config_get > "$workdir/after-env-bad.json" jq -e '.content[0].text | fromjson | .environment == "dev" and (.stack_config | keys) == ["cloudwatch","pre-prod-azure-monitor","prod-cloudwatch"] and (.stack_config["prod-cloudwatch"] | has("workspace") | not)' \ "$workdir/after-env-bad.json" > /dev/null \ @@ -222,25 +225,32 @@ step "a custom name that reads as - is refused (#618)" mcp_call odd_config_set 'config={"custom":{"prod-cloudwatch":{"stack_config_fields":["log_group"]}}}' > "$workdir/env-custom.json" || true grep -q "reads as the" "$workdir/env-custom.json" \ || { echo "ASSERTION FAILED: custom name prod-cloudwatch was not refused" >&2; cat "$workdir/env-custom.json" >&2; exit 1; } +# Nor one a built-in ends in: monitor would make azure-monitor read as azure + monitor (#656). +mcp_call odd_config_set 'config={"custom":{"monitor":{"stack_config_fields":["base_url"]}}}' > "$workdir/env-custom-builtin.json" || true +grep -q "would make the built-in stack 'azure-monitor'" "$workdir/env-custom-builtin.json" \ + || { echo "ASSERTION FAILED: custom name monitor was not refused" >&2; cat "$workdir/env-custom-builtin.json" >&2; exit 1; } -step "null deletes a key and an entry under a prefixed key, and clears the environment (#618)" +step "null deletes a key and an entry under a prefixed key; a switch to local clears the environment (#618, #656)" mcp_call odd_config_set 'config={"stack_config":{"prod-cloudwatch":{"region":null}}}' > "$workdir/env-del-key.json" jq -e '.content[0].text | fromjson | .config.stack_config["prod-cloudwatch"] == {"log_group":"/example/prod-logs"}' \ "$workdir/env-del-key.json" > /dev/null \ || { echo "ASSERTION FAILED: null key deletion on a prefixed entry" >&2; cat "$workdir/env-del-key.json" >&2; exit 1; } mcp_call odd_config_set \ - 'config={"environment":null,"stack":"local","stack_config":{"prod-cloudwatch":null,"pre-prod-azure-monitor":null,"cloudwatch":null}}' \ + 'config={"stack":"local","stack_config":{"prod-cloudwatch":null,"pre-prod-azure-monitor":null,"cloudwatch":null}}' \ > "$workdir/env-clear.json" jq -e '.content[0].text | fromjson | .config.environment == null and .config.stack_config == {} and .config.effective.stack_config_key == "local"' \ "$workdir/env-clear.json" > /dev/null \ || { echo "ASSERTION FAILED: clearing the environment and the prefixed entries" >&2; cat "$workdir/env-clear.json" >&2; exit 1; } -step "on the local stack the environment is inert (#618)" -mcp_call odd_config_set 'config={"environment":"prod"}' > "$workdir/env-local.json" -jq -e '.content[0].text | fromjson | .config.environment == "prod" and .config.effective == {"stack":"local","environment":null,"stack_config_key":"local","stack_config":{}}' \ - "$workdir/env-local.json" > /dev/null \ - || { echo "ASSERTION FAILED: the environment selected something on the local stack" >&2; cat "$workdir/env-local.json" >&2; exit 1; } -mcp_call odd_config_set 'config={"environment":null}' > /dev/null +step "the local stack takes no environment; a remote switch in the same call does (#656)" +mcp_call odd_config_set 'config={"environment":"prod"}' > "$workdir/env-local.json" || true +grep -q "the local stack takes no environment" "$workdir/env-local.json" \ + || { echo "ASSERTION FAILED: an environment was stored on the local stack" >&2; cat "$workdir/env-local.json" >&2; exit 1; } +mcp_call odd_config_set 'config={"stack":"cloudwatch","environment":"prod"}' > "$workdir/env-remote.json" +jq -e '.content[0].text | fromjson | .config.effective.stack == "cloudwatch" and .config.effective.environment == "prod"' \ + "$workdir/env-remote.json" > /dev/null \ + || { echo "ASSERTION FAILED: a remote switch with an environment was not accepted" >&2; cat "$workdir/env-remote.json" >&2; exit 1; } +mcp_call odd_config_set 'config={"stack":"local"}' > /dev/null step "a non-scalar stack_config value is rejected and writes nothing" mcp_call odd_config_set \ diff --git a/src/mcp-server/app/config.py b/src/mcp-server/app/config.py index 97dac36..ed31c33 100644 --- a/src/mcp-server/app/config.py +++ b/src/mcp-server/app/config.py @@ -71,8 +71,9 @@ # a remote backend's targeting values differ per deployment environment, # so one stack may hold one entry per environment next to its plain one. # The environment is kebab-case with no trailing dash - stricter than -# CUSTOM_NAME_RE - and never the sentinel the agents record when the -# telemetry carries no environment. The key is parsed by suffix, the +# CUSTOM_NAME_RE - never the sentinel the agents record when the +# telemetry carries no environment, never "local" - the local stack's +# environment by construction, which takes no other (#656). The key is parsed by suffix, the # longest known stack winning ("dev-azure-monitor" is dev + azure-monitor, # "pre-prod-cloudwatch" is pre-prod + cloudwatch); "local" takes no # prefix - the local stack is the local environment by construction. The @@ -81,10 +82,11 @@ # one: two whole entries, never a merge. ENVIRONMENT_RE = re.compile(r"^[a-z]([a-z0-9-]*[a-z0-9])?$") ENVIRONMENT_SENTINEL = "unknown" +NOT_ENVIRONMENTS = frozenset({ENVIRONMENT_SENTINEL, "local"}) KEY_FORMS = ( " or - - a built-in or a declared custom" " stack, kebab-case (^[a-z]([a-z0-9-]*[a-z0-9])?$) and never" - f" {ENVIRONMENT_SENTINEL!r}; local takes no prefix" + f" {ENVIRONMENT_SENTINEL!r} or 'local'; local takes no prefix" ) DEFAULTS = { @@ -128,7 +130,7 @@ def _valid_environment(value: object) -> bool: return ( isinstance(value, str) and ENVIRONMENT_RE.fullmatch(value) is not None - and value != ENVIRONMENT_SENTINEL + and value not in NOT_ENVIRONMENTS ) @@ -169,15 +171,24 @@ def _stack_config_key_allowed(stack: str, key: str, custom: dict) -> bool: return fields is None or key in fields -def _effective_entry(stack: str, environment: str | None, stack_config: dict) -> dict: +def _effective_entry( + stack: str, environment: str | None, stack_config: dict, custom: dict +) -> dict: """The entry the configured pair resolves to: the environment's when one is persisted for it, the stack's plain one otherwise - whole, never - merged. On the local stack the environment is inert.""" + merged. On the local stack the environment is inert. The composed key + must parse back to the pair: a known stack's own entry (a hand-edited + "monitor" next to azure-monitor, #656) is never another pair's.""" if stack == "local": environment = None key = stack - if environment is not None and f"{environment}-{stack}" in stack_config: - key = f"{environment}-{stack}" + prefixed = f"{environment}-{stack}" + if ( + environment is not None + and prefixed in stack_config + and resolve_stack_config_key(prefixed, custom) == (environment, stack) + ): + key = prefixed return { "stack": stack, "environment": environment, @@ -218,15 +229,15 @@ def load(path: Path | None = None) -> dict: try: stored = json.loads(target.read_text()) except FileNotFoundError: - effective["effective"] = _effective_entry("local", None, {}) + effective["effective"] = _effective_entry("local", None, {}, {}) return effective except (OSError, ValueError): effective["invalid_ignored"] = [""] - effective["effective"] = _effective_entry("local", None, {}) + effective["effective"] = _effective_entry("local", None, {}, {}) return effective if not isinstance(stored, dict): effective["invalid_ignored"] = [""] - effective["effective"] = _effective_entry("local", None, {}) + effective["effective"] = _effective_entry("local", None, {}, {}) return effective # Declarations first: the stack and the stack_config entries below are @@ -306,7 +317,10 @@ def load(path: Path | None = None) -> dict: if invalid: effective["invalid_ignored"] = invalid effective["effective"] = _effective_entry( - effective["stack"], effective["environment"], effective["stack_config"] + effective["stack"], + effective["environment"], + effective["stack_config"], + effective["custom"], ) return effective @@ -340,7 +354,8 @@ def _validate_custom(partial: dict, stored_custom: dict) -> dict: # A name readable as - would make a # stack_config key readable two ways (#618): refused in both # directions - the name itself, and a name that would turn an - # already-declared one into its environment entry. + # already-declared one, or a built-in (#656), into its + # environment entry. known = frozenset(STACKS) | frozenset(effective) - {name} parsed = _split_prefixed(name, known) if parsed is not None: @@ -349,6 +364,15 @@ def _validate_custom(partial: dict, stored_custom: dict) -> dict: f" entry of stack {parsed[1]!r} (stack_config keys are" f" -) - pick a name that ends in no known stack" ) + for builtin in STACKS: + split = _split_prefixed(builtin, known | {name}) + if split is not None and split[1] == name: + raise ValueError( + f"custom.{name}: declaring {name!r} would make the built-in" + f" stack {builtin!r} read as its environment entry" + f" (stack_config keys are -) - pick a" + f" name that no built-in stack ends in" + ) for other in sorted(effective): if other != name and _split_prefixed(other, known | {name}) is not None: raise ValueError( @@ -391,10 +415,18 @@ def save(partial: dict, path: Path | None = None) -> dict: ): raise ValueError( "environment must be a kebab-case name" - f" (^[a-z]([a-z0-9-]*[a-z0-9])?$), never {ENVIRONMENT_SENTINEL!r}," - f" or null to clear it, got {partial['environment']!r}" + f" (^[a-z]([a-z0-9-]*[a-z0-9])?$), never {ENVIRONMENT_SENTINEL!r}" + f" or 'local', or null to clear it, got {partial['environment']!r}" ) effective_stack = partial.get("stack", before["stack"]) + # The local stack is the local environment by construction (#656): an + # environment written there would stay invisible and re-arm on the + # next remote switch - refused, and a switch to local clears it (below). + if effective_stack == "local" and partial.get("environment") is not None: + raise ValueError( + "the local stack takes no environment - switch the stack first," + " or in the same call" + ) removed = {name for name, decl in partial.get("custom", {}).items() if decl is None} if effective_stack in removed: raise ValueError( @@ -482,11 +514,10 @@ def save(partial: dict, path: Path | None = None) -> dict: stored = {} if "stack" in partial: stored["stack"] = partial["stack"] - if "environment" in partial: - if partial["environment"] is None: - stored.pop("environment", None) - else: - stored["environment"] = partial["environment"] + if effective_stack == "local" or partial.get("environment", "") is None: + stored.pop("environment", None) + elif "environment" in partial: + stored["environment"] = partial["environment"] if local_partial: stored_local = stored.get("local") stored["local"] = { diff --git a/src/mcp-server/app/server.py b/src/mcp-server/app/server.py index 6fffce0..7a1dbce 100644 --- a/src/mcp-server/app/server.py +++ b/src/mcp-server/app/server.py @@ -120,7 +120,8 @@ def odd_config_set(config: dict) -> dict: {"stack": "seq", "custom": {"seq": {"stack_config_fields": ["base_url"]}}}. A custom name is kebab-case, never a built-in one and never one that reads as - (prod-cloudwatch, - or prod-seq once seq is declared - refused in both directions); its + or prod-seq once seq is declared - refused in both directions), and + never a built-in one ends in (monitor, for azure-monitor); its declaration lists the stack_config fields the stack's guide names (an empty list when it persists nothing), and a re-declaration replaces the list. The server never reads the stack's guide - the caller derives @@ -153,7 +154,7 @@ def odd_config_set(config: dict) -> dict: deployment environment next to the stack's plain one, e.g. {"stack_config": {"prod-cloudwatch": {"log_group": ""}}}: the environment is kebab-case (^[a-z]([a-z0-9-]*[a-z0-9])?$, never - "unknown"), the key is parsed by suffix (dev-azure-monitor is dev + + "unknown" or "local"), the key is parsed by suffix (dev-azure-monitor is dev + azure-monitor, pre-prod-cloudwatch is pre-prod + cloudwatch - the longest known stack wins), a key whose suffix is no known stack is rejected, and local takes no prefix. stack_config is merged per key @@ -178,11 +179,13 @@ def odd_config_set(config: dict) -> dict: container either. environment selects, for the configured stack, which entry the missions read: {"environment": "prod"} persists it (the same pattern as a key's - prefix, "unknown" refused), {"environment": null} clears it; absent by - default. odd_config_get's effective block resolves the configured pair - - the - entry when one is persisted, else the - plain entry, whole, never merged. On the local stack the field - is inert (the environment is local by construction). + prefix, "unknown" and "local" refused), {"environment": null} clears + it; absent by default. odd_config_get's effective block resolves the + configured pair - the - entry when one is + persisted, else the plain entry, whole, never merged. The local + stack takes no environment (it is local by construction): the write + is refused there - switch the stack first, or in the same call - and a + switch to local clears the stored one. """ ports_before = config_ops.load()["local"] # Read on the RAW partial, before save validates it: a malformed one diff --git a/tests/mcp-server/test_config.py b/tests/mcp-server/test_config.py index 4b1d3c2..d86c853 100644 --- a/tests/mcp-server/test_config.py +++ b/tests/mcp-server/test_config.py @@ -797,17 +797,26 @@ def test_key_parse_is_by_suffix_dashed_environment_and_dashed_stack( def test_key_parse_prefers_the_longest_known_stack(tmp_path): - # A custom stack "monitor" next to the built-in "azure-monitor": the - # key dev-azure-monitor is dev + azure-monitor, never dev-azure + - # monitor - the longest known stack wins. + # A custom stack "unknown-seq" next to "seq" (its prefix is the + # sentinel, no environment, so both are declarable): the key + # dev-unknown-seq is dev + unknown-seq, never dev-unknown + seq - the + # longest known stack wins. path = tmp_path / "config.json" - config.save(_declare("monitor", ("workspace",)), path) + config.save( + { + "custom": { + "seq": {"stack_config_fields": ["base_url"]}, + "unknown-seq": {"stack_config_fields": ["base_url"]}, + } + }, + path, + ) custom = config.load(path)["custom"] - assert config.resolve_stack_config_key("dev-azure-monitor", custom) == ( + assert config.resolve_stack_config_key("dev-unknown-seq", custom) == ( "dev", - "azure-monitor", + "unknown-seq", ) - assert config.resolve_stack_config_key("dev-monitor", custom) == ("dev", "monitor") + assert config.resolve_stack_config_key("dev-seq", custom) == ("dev", "seq") def test_plain_keys_still_resolve_to_the_stack_alone(): @@ -826,6 +835,7 @@ def test_plain_keys_still_resolve_to_the_stack_alone(): "1prod-cloudwatch", # must start with a letter "prod_eu-cloudwatch", # underscore "unknown-cloudwatch", # the sentinel is not an environment + "local-cloudwatch", # local is the local stack's environment (#656) "prod-local", # local takes no prefix "cloudwatch-", # no stack suffix ], @@ -840,7 +850,13 @@ def test_save_rejects_an_unresolvable_key_and_writes_nothing(tmp_path, key): def test_unresolvable_keys_do_not_parse(): - for key in ("prod-nagios", "unknown-cloudwatch", "prod-local", "Prod-cloudwatch"): + for key in ( + "prod-nagios", + "unknown-cloudwatch", + "local-cloudwatch", + "prod-local", + "Prod-cloudwatch", + ): assert config.resolve_stack_config_key(key, {}) is None @@ -968,6 +984,54 @@ def test_save_refuses_a_declaration_that_makes_another_name_prefixed(tmp_path): assert result["custom"] == {"seq": {"stack_config_fields": ["base_url"]}} +def test_save_refuses_a_custom_name_that_makes_a_builtin_prefixed(tmp_path): + # Issue #656: declaring "monitor" would make the built-in azure-monitor + # read as azure + monitor, and the pair (azure, monitor) would resolve + # to the built-in's entry. + path = tmp_path / "config.json" + with pytest.raises(ValueError, match="custom.monitor") as info: + config.save({"stack": "monitor", **_declare("monitor")}, path) + assert "azure-monitor" in str(info.value) + assert not path.exists() + + +def test_a_stored_builtin_suffix_does_not_block_other_declarations(tmp_path): + # A "monitor" stored before #656 is kept by load; it must not make every + # later declaration fail, nor be blamed on the name being declared. + path = tmp_path / "config.json" + path.write_text( + json.dumps({"custom": {"monitor": {"stack_config_fields": ["base_url"]}}}) + ) + result = config.save(_declare("seq"), path) + assert set(result["custom"]) == {"monitor", "seq"} + + +def test_effective_never_reads_a_known_stacks_own_entry(tmp_path): + # A hand-edited file declaring "monitor" (accepted before #656): the + # pair (azure, monitor) never resolves to the built-in azure-monitor's + # entry - it falls back to monitor's plain one. + path = tmp_path / "config.json" + path.write_text( + json.dumps( + { + "stack": "monitor", + "environment": "azure", + "custom": {"monitor": {"stack_config_fields": ["base_url"]}}, + "stack_config": { + "azure-monitor": {"workspace": "00000000-fake-ws"}, + "monitor": {"base_url": "http://monitor.example.test"}, + }, + } + ) + ) + assert config.load(path)["effective"] == { + "stack": "monitor", + "environment": "azure", + "stack_config_key": "monitor", + "stack_config": {"base_url": "http://monitor.example.test"}, + } + + def test_a_custom_name_ending_in_local_is_not_prefixed(): # local takes no prefix, so "seq-local" has one parse: itself. assert config.resolve_stack_config_key("seq-local", {"seq-local": {}}) == ( @@ -1024,7 +1088,7 @@ def test_environment_is_absent_by_default(tmp_path): def test_save_persists_and_clears_the_environment(tmp_path): path = tmp_path / "config.json" - result = config.save({"environment": "prod"}, path) + result = config.save({"stack": "cloudwatch", "environment": "prod"}, path) assert result["environment"] == "prod" assert json.loads(path.read_text())["environment"] == "prod" result = config.save({"environment": "pre-prod"}, path) @@ -1035,18 +1099,19 @@ def test_save_persists_and_clears_the_environment(tmp_path): @pytest.mark.parametrize( - "value", ["Prod", "unknown", "prod-", "-prod", "prod_eu", "1prod", "", 3, ["prod"]] + "value", + ["Prod", "unknown", "local", "prod-", "-prod", "prod_eu", "1prod", "", 3, ["prod"]], ) def test_save_rejects_an_invalid_environment_and_writes_nothing(tmp_path, value): path = tmp_path / "config.json" - with pytest.raises(ValueError, match="environment"): - config.save({"environment": value}, path) + with pytest.raises(ValueError, match="environment must be"): + config.save({"stack": "cloudwatch", "environment": value}, path) assert not path.exists() def test_load_tolerates_an_invalid_stored_environment(tmp_path): path = tmp_path / "config.json" - for bad in ("Prod", "unknown", 3): + for bad in ("Prod", "unknown", "local", 3): path.write_text(json.dumps({"stack": "cloudwatch", "environment": bad})) result = config.load(path) assert result["environment"] is None @@ -1117,19 +1182,52 @@ def test_effective_resolves_a_custom_stacks_pair(tmp_path): } +def test_save_refuses_an_environment_on_the_local_stack(tmp_path): + # Issue #656: the local stack is the local environment by construction; + # a stored environment would stay invisible there and re-arm on the + # next remote switch. + path = tmp_path / "config.json" + with pytest.raises(ValueError, match="local stack takes no environment"): + config.save({"environment": "prod"}, path) + assert not path.exists() + config.save({"stack": "cloudwatch"}, path) + with pytest.raises(ValueError, match="local stack takes no environment"): + config.save({"stack": "local", "environment": "prod"}, path) + assert config.load(path)["stack"] == "cloudwatch" + + +def test_save_accepts_a_remote_switch_and_an_environment_from_local(tmp_path): + path = tmp_path / "config.json" + result = config.save({"stack": "cloudwatch", "environment": "prod"}, path) + assert result["effective"]["environment"] == "prod" + + +def test_a_switch_to_local_clears_the_environment(tmp_path): + # The reverse of the refusal: an environment kept through a switch to + # local would be invisible there and re-armed by the next remote switch. + path = tmp_path / "config.json" + config.save({"stack": "cloudwatch", "environment": "prod"}, path) + result = config.save({"stack": "local"}, path) + assert result["environment"] is None + assert "environment" not in json.loads(path.read_text()) + result = config.save({"stack": "cloudwatch"}, path) + assert result["effective"]["environment"] is None + + def test_environment_is_inert_on_the_local_stack(tmp_path): - # The local stack is the local environment by construction: the field - # persists (it applies again after a switch) but selects nothing. + # A file stored before #656 may still hold one: it selects nothing, and + # the next write on the local stack drops it. path = tmp_path / "config.json" - config.save({"environment": "prod"}, path) - result = _cw(path, local={"GF_LOG_LEVEL": "debug"}) - assert result["environment"] == "prod" - assert result["effective"] == { + path.write_text(json.dumps({"environment": "prod"})) + assert config.load(path)["effective"] == { "stack": "local", "environment": None, "stack_config_key": "local", - "stack_config": {"GF_LOG_LEVEL": "debug"}, + "stack_config": {}, } + result = _cw(path, local={"GF_LOG_LEVEL": "debug"}) + assert result["environment"] is None + assert result["effective"]["stack_config"] == {"GF_LOG_LEVEL": "debug"} def test_effective_reads_the_loaded_entry_not_the_file(tmp_path): diff --git a/tests/mcp-server/test_server.py b/tests/mcp-server/test_server.py index 49c1575..34abe66 100644 --- a/tests/mcp-server/test_server.py +++ b/tests/mcp-server/test_server.py @@ -437,3 +437,7 @@ def test_config_descriptions_state_the_environment_and_the_effective_entry(): assert "stack_config_key" in get_description assert "-" in set_description assert '"environment": null' in set_description + # Issue #656: the refusals are stated where the write is described. + flat = " ".join(set_description.split()) + assert "local stack takes no environment" in flat + assert "never a built-in one ends in" in flat