diff --git a/.env.example b/.env.example index 67f1bd48..6036514c 100644 --- a/.env.example +++ b/.env.example @@ -20,6 +20,31 @@ # each process mints its own token, which only works single-process-per-host. # APOLLO_INTERNAL_TOKEN= +# Which characters a workflow step name may contain. Apollo sanitises step names +# on the way out and Lightning validates them on the way in, so the two rules +# have to agree. +# +# false (the default) ASCII only: letters, digits, spaces, hyphens and +# underscores. Accents are folded (Café -> Cafe) and +# anything else is dropped. Matches the rule Lightning +# enforces today. +# true Anything except control characters: letters and marks +# from any script, all punctuation and symbols, emoji, +# / : > & and quotes. Vérifier l'état and 患者確認 +# survive exactly as typed. +# +# Leave this off until Lightning ships its Unicode step names (Lightning#4577), +# then turn it on. Turning it on first means Apollo emits names Lightning +# rejects; leaving it off afterwards means Apollo renames steps people typed +# deliberately. +# +# Both modes reject the same control set and nothing else: C0 (U+0000-U+001F, +# NUL included), DEL (U+007F), C1 (U+0080-U+009F), U+FFFE / U+FFFF, the +# surrogates U+D800-U+DFFF, and the separators U+2028 / U+2029. Names +# are NFC-normalised and capped at 100 graphemes in both modes. +# See services/name_rules.py. +APOLLO_UNICODE_STEP_NAMES=false + ANTHROPIC_API_KEY=sk-YOUR-API-KEY-HERE OPENAI_API_KEY=sk-YOUR-API-KEY-HERE diff --git a/.github/workflows/unit-tests.yaml b/.github/workflows/unit-tests.yaml index 5fa2788f..53d5456c 100644 --- a/.github/workflows/unit-tests.yaml +++ b/.github/workflows/unit-tests.yaml @@ -28,6 +28,43 @@ jobs: - name: Run unit tests run: poetry run pytest services/*/tests/unit + unicode-parity: + name: Unicode parity with Elixir + runs-on: ubuntu-latest + timeout-minutes: 15 + + # `services/name_rules.py` carries tables generated from the Elixir that + # Lightning runs: grapheme break classes, Extended_Pictographic, combining + # classes, the trim set and OTP's NFC. If Elixir's or Python's Unicode + # version moves and nobody re-runs the harness, the tables silently stop + # matching and Apollo starts emitting step names Lightning rejects. This + # job is the only thing that would notice. + steps: + - uses: actions/checkout@v7 + + - name: Set up Elixir + uses: erlef/setup-beam@v1 + with: + elixir-version: "1.18.3" + otp-version: "27" + + - name: Set up Python 3.11 + uses: actions/setup-python@v7 + with: + python-version: "3.11" + + - name: Generate range edges from the tables + working-directory: tools/unicode_parity + run: python3 edges.py + + - name: Probe Elixir + working-directory: tools/unicode_parity + run: elixir probe.exs + + - name: Compare against name_rules + working-directory: tools/unicode_parity + run: python3 check.py + bun: name: Bun unit tests runs-on: ubuntu-latest diff --git a/README.md b/README.md index 2e4d7c3c..a08e04d8 100644 --- a/README.md +++ b/README.md @@ -147,6 +147,73 @@ full list of keys and env vars Apollo reads. Also note that `tmp` dirs are untracked, so if you do want to store credentials in your json, keep it inside a tmp dir and it'll remain safe and secret. +### `APOLLO_UNICODE_STEP_NAMES` + +Controls which characters a workflow step name may contain. Apollo sanitises +step names on the way out and Lightning validates them on the way in, so the two +rules have to agree. The default is `false`. + +| Value | Rule | +| --- | --- | +| `false` (default) | ASCII only. Letters, digits, spaces, hyphens, underscores. Accents are folded (`Café` becomes `Cafe`) and anything else is dropped. This is the rule Lightning enforces today. | +| `true` | Anything except control characters. Letters and marks from any script, all punctuation and symbols, emoji, `/`, `:`, `>`, `&`, quotes and apostrophes. `Vérifier l'état` and `患者確認` survive exactly as typed. | + +Leave it off until Lightning ships Unicode step names (Lightning#4577), then +turn it on. Turning it on first means Apollo emits names Lightning rejects. +Leaving it off afterwards means Apollo renames steps people typed deliberately, +across the whole workflow, on every turn that returns YAML. + +The permissive rule is deliberately maximal. Apollo being stricter than +Lightning is the worse of the two failures: Lightning rejecting a name is loud +and recoverable, whereas Apollo quietly renaming a valid name is the silent +vandalism this flag exists to prevent. + +The rejected control set is the same in both modes: C0 (`U+0000`-`U+001F`, +NUL included), DEL (`U+007F`), C1 (`U+0080`-`U+009F`), the noncharacters +`U+FFFE` and `U+FFFF`, the surrogates `U+D800`-`U+DFFF`, and the line and +paragraph separators `U+2028` and `U+2029`. A NUL byte in a name crashes the Postgres insert on +Lightning's side. Names are NFC-normalised in both modes so that Apollo and +Lightning agree on how to spell an accent, which is what step lookup matches +on, and capped at 100 graphemes because that is what Ecto's `validate_length` +counts. + +The rule lives in `services/name_rules.py`, and everything that states or +enforces it is derived from there: the sanitiser, the workflow-generation +prompt (`describe_rule_for_prompt`), the acceptance-test judges +(`describe_rule_for_judge`, substituted into the rubric markdown by +`judges.load_judge`), and the `assert_no_special_chars` test assertion. Change +the rule in that one file and all four follow. + +The 100-character cap is counted in graphemes, because that is what Ecto's +`validate_length` counts on Lightning's side. The clustering is hand-written in +`name_rules`, with no third-party dependency, and it targets Elixir's +`String.length/1` rather than UAX #29 — Elixir deviates from the spec in two +places (it does not implement the Unicode 15.1 Indic conjunct rule, and it ends +an emoji ZWJ run at the joiner unless a pictograph follows) and the whole point +is to agree with Elixir, not with the spec. + +`tools/unicode_parity` is the harness that checks it. Run `python3 edges.py`, +then `elixir probe.exs`, then `python3 check.py` with the Elixir version Lightning runs; `--tables` +prints the literals to paste back into `name_rules`. It checks five things: +every codepoint's break class, the `Extended_Pictographic` set, the trim set, +what a GB11 emoji run may be separated from its joiner by, and cluster +boundaries over a generated corpus. Normalisation is not among them: +`normalize_nfc` is the standard library's, so there is no table of ours to +check against Elixir. + +`Extended_Pictographic` needs its own check because it is *not* a break class, +so a per-codepoint sweep cannot see it — an over-broad set there silently +changes clustering either side of a ZWJ and nothing else notices. That is +exactly how a hand-written table with 531 wrong codepoints survived two rounds +of review. + +Re-run the harness whenever Python's or Elixir's Unicode version moves. +`name_rules.PARITY_SOURCE` records what the committed tables were generated +from, and a unit test pins it, so a silent regeneration fails loudly. Python +moving ahead only makes Apollo overcount, which truncates early; Elixir moving +ahead is the direction that reintroduces undercounting, and an undercount ships +a name Lightning rejects. + ## Debugging The server defaults to port 3000. You can test any service directly with curl to diff --git a/pyproject.toml b/pyproject.toml index f67ccac0..203f7ebc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -45,6 +45,7 @@ pythonpath = ["services"] # Discovery roots. pytest walks these for test_*.py files. testpaths = [ + "services/echo/tests", "services/global_chat/tests", "services/workflow_chat/tests", "services/job_chat/tests", @@ -59,6 +60,9 @@ python_functions = ["test_*"] markers = [ "unit: fast, isolated, no I/O. Runs on every PR push.", + # Declared but currently unused: nothing carries it, so `-m \"unit or service\"` + # is really `-m unit`. Kept because the tiers are referenced in the testing + # README; apply it when the first mocked-client suite lands. "service: mocks HTTP/LLM clients; exercises service handlers. Runs on merge.", "integration: hits real external services (LLM, Pinecone, Postgres). Manual/nightly.", "acceptance: end-to-end acceptance criteria. Manual/nightly.", diff --git a/services/global_chat/PAYLOAD_SPEC.md b/services/global_chat/PAYLOAD_SPEC.md index f8cde2e0..4116fffc 100644 --- a/services/global_chat/PAYLOAD_SPEC.md +++ b/services/global_chat/PAYLOAD_SPEC.md @@ -172,7 +172,9 @@ The `page` field is a simplified path/breadcrumb representing where the user is workflows// ``` -The step name should match a job key in the workflow YAML (exact match or normalized — lowercase, non-alphanumeric chars replaced with hyphens). The backend parses the URL by splitting on `/` and reading the 3rd segment as the step name. +The step name should match a job key in the workflow YAML, either exactly or after normalization — NFC-normalized, lowercased, with every character that is not a letter, mark or digit replaced by a hyphen. Normalization is Unicode-aware, so `患者確認` normalizes to itself rather than to the empty string; a name that normalizes to nothing is never fuzzy-matched. + +The backend parses the URL by splitting on `/` and taking everything after the workflow segment as the step name, so a step name containing a `/` survives. A workflow name containing a `/` still makes the split ambiguous, so the parsed step name is validated against the workflow YAML rather than trusted. | Page URL | Router signal | What happens | |---|---|---| diff --git a/services/global_chat/tests/test_workflow_chat_pass_fail.py b/services/global_chat/tests/test_workflow_chat_pass_fail.py index b5545bd1..f61bff65 100644 --- a/services/global_chat/tests/test_workflow_chat_pass_fail.py +++ b/services/global_chat/tests/test_workflow_chat_pass_fail.py @@ -222,9 +222,9 @@ def test_rename_two_jobs_commcare(): def test_special_characters(): print("==================TEST==================") - print("Description: Ask for a workflow that uses platforms with special characters in their names. " - "Verify that diacritics and punctuation removed/normalised correctly (e.g. é->e) in job names " - "in the generated YAML.") + print("Description: Ask for a workflow that uses platforms with accents and punctuation in their " + "names. Verify the job names in the generated YAML obey whichever step-name rule is active " + "(see name_rules): folded to ASCII by default, kept as typed with APOLLO_UNICODE_STEP_NAMES on.") existing_yaml = """""" history = [ {"role": "user", "content": "Create a workflow that retrieves data from mwater, google sheets, netsuite, ferntech.io and processed it and sends it to frappé"}, diff --git a/services/global_chat/tests/unit/test_error_logging.py b/services/global_chat/tests/unit/test_error_logging.py index ebc1d113..07d1a7a2 100644 --- a/services/global_chat/tests/unit/test_error_logging.py +++ b/services/global_chat/tests/unit/test_error_logging.py @@ -369,18 +369,42 @@ def _leak_patterns(names: set[str]) -> list: # Job and edge names before and after sanitising, the adaptor a job # declares, and the `__ID_JOB_x__` placeholders this service invented # itself. Names and ids, never a body. + # The naming work replaced per-key logging with one line naming the whole + # renamed set, so the individual key expressions the leak branch vets are + # gone from this module here. "workflow_chat/workflow_chat.py": frozenset({ "adaptor", "job_key", - "edge_key", - "sanitized_edge_key", - "original_name", - "sanitized_name", - "original_source", - "original_target", - "edge_data['source_job']", - "edge_data['target_job']", "current_id", + # Job names and edge endpoints, resolved or unresolved. `unclaimed` + # reads as bodies but holds the `__CODE_BLOCK___` tokens, so it is + # keys too. Same category as the names above, and the reason a name is + # loggable where a body is not: the user typed it into a form as a + # label, and a log line is unreadable without it. + "', '.join(sorted(matches))", + "', '.join(duplicated)", + "', '.join(unclaimed)", + "', '.join(renamed)", + "by_name", + "owner", + "', '.join(sorted(dangling))", + # Literals chosen at the call site, a parameter the callers pass a + # literal to, and a count. `msg` is built but only from `len()`. + "how", + "label", + "msg", + # More names: the reference as written, and what it sanitises to. + "reference", + "str(reference)", + "resolved", + }), + + # Job names again, on the shared walkers. + "yaml_utils.py": frozenset({ + "', '.join(sorted((str(match) for match in matches)))", + "job_key", + "how", + "step_name", }), } @@ -978,7 +1002,7 @@ def test_a_vetted_expression_is_scoped_to_its_module() -> None: #: Every expression cleared by hand in `VETTED_INTERPOLATIONS`. Pinned for the #: same reason as `EXPECTED_MARKERS`: an opt-out nobody counts is an opt-out #: that spreads. -EXPECTED_VETTED_INTERPOLATIONS = 51 +EXPECTED_VETTED_INTERPOLATIONS = 60 def test_the_vetted_interpolations_are_inventoried() -> None: diff --git a/services/global_chat/tests/unit/test_name_rules.py b/services/global_chat/tests/unit/test_name_rules.py new file mode 100644 index 00000000..10e11aec --- /dev/null +++ b/services/global_chat/tests/unit/test_name_rules.py @@ -0,0 +1,637 @@ +"""Unit tests for the shared step-name rule (`services/name_rules.py`). + +`yaml_utils` lives next to it and is tested from here too, so the shared +modules keep their tests in one place. +""" + +import ast +import inspect +import unicodedata + +import name_rules +import pytest +import yaml +from name_rules import ( + _TRIM_CHARS, + MAX_NAME_LENGTH, + PARITY_SOURCE, + UNICODE_FLAG_ENV, + _is_ext_pict, + describe_rule, + describe_rule_for_prompt, + first_invalid_char, + grapheme_clusters, + grapheme_length, + is_valid_name, + normalize_for_lookup, + normalize_nfc, + sanitize_name, + unicode_names_enabled, +) + + +@pytest.fixture +def ascii_mode(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv(UNICODE_FLAG_ENV, "false") + + +@pytest.fixture +def unicode_mode(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv(UNICODE_FLAG_ENV, "true") + + +# --- the flag --------------------------------------------------------------- + + +def test_unicode_is_off_when_the_flag_is_unset(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv(UNICODE_FLAG_ENV, raising=False) + assert unicode_names_enabled() is False + + +@pytest.mark.parametrize("value", ["true", "TRUE", "1", "yes", "on", " True "]) +def test_flag_accepts_the_usual_truthy_spellings(monkeypatch: pytest.MonkeyPatch, value: str) -> None: + monkeypatch.setenv(UNICODE_FLAG_ENV, value) + assert unicode_names_enabled() is True + + +@pytest.mark.parametrize("value", ["false", "0", "no", "off", "", "maybe"]) +def test_flag_treats_anything_else_as_off(monkeypatch: pytest.MonkeyPatch, value: str) -> None: + monkeypatch.setenv(UNICODE_FLAG_ENV, value) + assert unicode_names_enabled() is False + + +# --- the ASCII rule --------------------------------------------------------- + + +@pytest.mark.usefixtures("ascii_mode") +@pytest.mark.parametrize( + ("raw", "expected"), + [ + ("Vérifier l'état", "Verifier letat"), + ("O'Brien's Step", "OBriens Step"), + ("Café München", "Cafe Munchen"), + ("Fetch Data", "Fetch Data"), + ("Valid Job-Name_123", "Valid Job-Name_123"), + ("患者確認", ""), + ("Проверка данных", ""), + ], +) +def test_ascii_rule(raw: str, expected: str) -> None: + assert sanitize_name(raw) == expected + + +@pytest.mark.usefixtures("ascii_mode") +def test_ascii_rule_no_longer_leaves_a_name_of_only_spaces() -> None: + """`Проверка данных` used to sanitize to a single space, which is not a name.""" + assert sanitize_name("Проверка данных") == "" + assert sanitize_name("ß straße") == "ss strasse" + + +# --- the Unicode rule ------------------------------------------------------- + + +@pytest.mark.usefixtures("unicode_mode") +@pytest.mark.parametrize( + "raw", + [ + "Vérifier l'état", + "O'Brien's Step", + "患者確認", + "Проверка данных", + "ß straße", + "رعاية المرضى", + "Étape 1 (données)", + "Étape 1: charger", + ], +) +def test_unicode_rule_keeps_names_as_typed(raw: str) -> None: + assert sanitize_name(raw) == raw + + +@pytest.mark.usefixtures("unicode_mode") +@pytest.mark.parametrize( + "raw", + [ + "a->b", # nothing splits an edge key on "->"; it is a label + "Import A/B", # the page breadcrumb takes everything after the workflow + "a|b", + "ac", + "a#b", + "Done ✅", + "Ship it 🚢🇫🇷", + "Étape « une »", + 'He said "go"', + "50% & rising", + "@mention", + ], +) +def test_unicode_rule_allows_everything_that_is_not_a_control(raw: str) -> None: + """The permissive rule is deliberately maximal. + + Apollo being stricter than Lightning is the silent-vandalism failure that + issue #446 exists to prevent, so nothing but control characters is stripped. + """ + assert first_invalid_char(raw) is None + assert sanitize_name(raw) == raw + assert is_valid_name(raw) is True + + +@pytest.mark.usefixtures("unicode_mode") +def test_unicode_rule_keeps_zero_width_joiner_sequences() -> None: + """ZWJ is a format character — category C, but Lightning accepts it, so we must.""" + family = "Team \U0001f469\u200d\U0001f4bb" + assert sanitize_name(family) == family + + +# --- control characters, both modes ----------------------------------------- + + +@pytest.mark.parametrize("mode", ["false", "true"]) +@pytest.mark.parametrize("control", ["\x00", "\x01", "\x1b", "\x7f", "\x85", "\x9b"]) +def test_control_characters_never_survive(monkeypatch: pytest.MonkeyPatch, mode: str, control: str) -> None: + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + assert control not in sanitize_name(f"Fetch{control}Data") + + +@pytest.mark.parametrize("mode", ["false", "true"]) +def test_nul_is_rejected_even_alone(monkeypatch: pytest.MonkeyPatch, mode: str) -> None: + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + assert sanitize_name("\x00") == "" + + +# --- NFC --------------------------------------------------------------------- + + +@pytest.mark.usefixtures("unicode_mode") +def test_decomposed_and_composed_forms_agree() -> None: + """The same name typed two ways must come out identical, or lookups miss.""" + composed = "Vérifier" # U+00E9 + decomposed = "Vérifier" # e + combining acute + + assert composed != decomposed + assert sanitize_name(composed) == sanitize_name(decomposed) == composed + assert normalize_for_lookup(composed) == normalize_for_lookup(decomposed) + + +# --- validity helpers -------------------------------------------------------- + + +@pytest.mark.usefixtures("ascii_mode") +def test_is_valid_name_tracks_the_active_rule(monkeypatch: pytest.MonkeyPatch) -> None: + assert is_valid_name("Fetch Data") is True + assert is_valid_name("Vérifier l'état") is False + + monkeypatch.setenv(UNICODE_FLAG_ENV, "true") + assert is_valid_name("Vérifier l'état") is True + + +@pytest.mark.usefixtures("ascii_mode") +def test_first_invalid_char_names_the_offender() -> None: + assert first_invalid_char("Fetch Data") is None + assert first_invalid_char("Fetch@Data") == "@" + + +# --- the prompt text --------------------------------------------------------- + + +def test_the_prompt_text_changes_with_the_mode() -> None: + """The prompt and the sanitizer are built from the same rule, so it must move.""" + ascii_text = describe_rule(unicode_mode=False) + unicode_text = describe_rule(unicode_mode=True) + + assert ascii_text != unicode_text + assert "only unaccented English letters" in ascii_text + assert "any script" in unicode_text + assert "100" in describe_rule_for_prompt(unicode_mode=False) + assert "unique" in describe_rule_for_prompt(unicode_mode=True) + + +# --- lookup normalization ---------------------------------------------------- + + +def test_normalize_for_lookup_is_unicode_aware() -> None: + """Non-Latin names used to fold to the empty string, which cross-matched everything.""" + assert normalize_for_lookup("患者確認") == "患者確認" + assert normalize_for_lookup("Проверка данных") == "проверка-данных" + assert normalize_for_lookup("患者確認") != normalize_for_lookup("データ送信") + + +def test_normalize_for_lookup_keeps_the_old_latin_behaviour() -> None: + assert normalize_for_lookup("Fetch Patients") == "fetch-patients" + assert normalize_for_lookup("--Fetch/Patients--") == "fetch-patients" + assert normalize_for_lookup("") == "" + + +def test_normalize_for_lookup_keeps_combining_marks() -> None: + """Devanagari matras are Unicode marks, not letters — they must not become hyphens.""" + assert normalize_for_lookup("रोगी की जाँच") == "रोगी-की-जाँच" + + +# --- length cap --------------------------------------------------------------- + + +@pytest.mark.parametrize("mode", ["false", "true"]) +def test_cap_counts_graphemes(monkeypatch: pytest.MonkeyPatch, mode: str) -> None: + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + assert grapheme_length(sanitize_name("a" * 200)) == MAX_NAME_LENGTH + + +@pytest.mark.usefixtures("unicode_mode") +def test_grapheme_length_counts_user_perceived_characters() -> None: + assert grapheme_length("abc") == len("abc") + assert grapheme_length("e\u0301") == 1 # e + combining acute + assert grapheme_length("\U0001f469\u200d\U0001f4bb") == 1 # ZWJ sequence + assert grapheme_length("\U0001f1eb\U0001f1f7") == 1 # flag, two regional indicators + assert grapheme_length("\U0001f44d\U0001f3fd") == 1 # emoji + skin tone + # Devanagari conjuncts are covered by the Elixir parity table below + # instead — they sit on the GB9c divergence, so a hand-written expectation + # here would just duplicate that table and drift from it. + + +#: A grapheme NFC cannot collapse into a single codepoint. `e` + combining +#: acute is no use for testing truncation: NFC composes it to `é` before the +#: cut ever happens, so it never exercises a multi-codepoint cluster. +SCOTLAND_FLAG = "\U0001F3F4\U000E0067\U000E0062\U000E0073\U000E0063\U000E0074\U000E007F" +WOMAN_TECHNOLOGIST = "\U0001F469\u200d\U0001F4BB" + + +@pytest.mark.usefixtures("unicode_mode") +@pytest.mark.parametrize("cluster", [SCOTLAND_FLAG, WOMAN_TECHNOLOGIST, "\U0001F44D\U0001F3FD"]) +def test_cap_does_not_split_a_grapheme(cluster: str) -> None: + """Cutting inside a cluster corrupts it — an orphaned tag or a bare joiner. + + Each of these stays multi-codepoint through NFC, so the cut is real. + """ + assert len(cluster) > 1, "NFC collapsed the fixture; it no longer tests anything" + + capped = sanitize_name(cluster * 200) + + assert grapheme_length(capped) == MAX_NAME_LENGTH + assert capped == cluster * MAX_NAME_LENGTH + # Nothing left dangling at the cut. + assert grapheme_clusters(capped)[-1] == cluster + + +@pytest.mark.usefixtures("unicode_mode") +def test_nfc_composition_before_the_cap() -> None: + """The old fixture, kept to pin down why it was the wrong test.""" + assert sanitize_name("e\u0301" * 200) == "\u00e9" * MAX_NAME_LENGTH + + +@pytest.mark.usefixtures("unicode_mode") +def test_cap_counts_emoji_as_one_each() -> None: + capped = sanitize_name("\U0001f469\u200d\U0001f4bb" * 200) + assert grapheme_length(capped) == MAX_NAME_LENGTH + + +# --- parity with Elixir's String.length/1 ------------------------------------- +# +# Ecto's `validate_length` counts graphemes with `String.length/1`, so that is +# the authority — not UAX #29, which Elixir deviates from in two places we +# deliberately copy. Every number below was generated by running Elixir 1.18.3 +# over the same codepoint sequences. +# +# The standing check is `tools/unicode_parity`, not a figure quoted here: it +# puts every codepoint in 0x0..0x10FFFF in the same break class as Elixir and +# compares cluster boundaries over the corpus `probe.exs` generates. There is +# no known clustering divergence; if one appears, add the shape here rather +# than widening the assertion. + +ELIXIR_PARITY = [ + ("ascii", [0x0061, 0x0062, 0x0063], 3), + ("e+combining acute", [0x0065, 0x0301], 1), + ("ExtPict ZWJ ExtPict", [0x1F469, 0x200D, 0x1F4BB], 1), + ("a ZWJ b (GB11 must NOT join)", [0x0061, 0x200D, 0x0062], 2), + ("a ZWJ combining mark (plain lead: mark attaches)", [0x0061, 0x200D, 0x0301], 1), + ("ExtPict ZWJ combining mark (emoji run ends at the joiner)", [0x00A9, 0x200D, 0x0301], 2), + ("ExtPict ZWJ combining mark x200", [0x00A9, 0x200D, 0x0301] * 200, 400), + ("woman ZWJ combining mark", [0x1F469, 0x200D, 0x0301], 2), + ("ExtPict Extend ZWJ combining mark", [0x00A9, 0x0301, 0x200D, 0x0301], 2), + ("ExtPict ZWJ VS16", [0x00A9, 0x200D, 0xFE0F], 2), + ("ExtPict ZWJ skintone", [0x00A9, 0x200D, 0x1F3FD], 2), + ("ExtPict ZWJ ZWJ", [0x00A9, 0x200D, 0x200D], 2), + ("scotland flag tag seq", [0x1F3F4, 0xE0067, 0xE0062, 0xE0073, 0xE0063, 0xE0074, 0xE007F], 1), + ("15x scotland flag", [0x1F3F4, 0xE0067, 0xE0062, 0xE0073, 0xE0063, 0xE0074, 0xE007F] * 15, 15), + ("FR flag (2 RI)", [0x1F1EB, 0x1F1F7], 1), + ("3 RI", [0x1F1EB, 0x1F1F7, 0x1F1EB], 2), + ("4 RI", [0x1F1EB, 0x1F1F7, 0x1F1EB, 0x1F1F7], 2), + ("thumbsup + skintone", [0x1F44D, 0x1F3FD], 1), + ("devanagari namaste", [0x0928, 0x092E, 0x0938, 0x094D, 0x0924, 0x0947], 4), + ("indic conjunct ka virama ssa", [0x0915, 0x094D, 0x0937], 2), + ("CRLF", [0x000D, 0x000A], 1), + ("hangul L V T", [0x1100, 0x1161, 0x11A8], 1), + ("hangul LV + T", [0xAC00, 0x11A8], 1), + ("keycap 1", [0x0031, 0xFE0F, 0x20E3], 1), + ("arabic number sign prepend", [0x0600, 0x0661], 1), + ("zanabazar prepend 11A3A x150", [0x11A3A, 0x0061] * 150, 150), + ("masaram prepend 11D46 x150", [0x11D46, 0x0061] * 150, 150), + ("kawi prepend 11F02 x150", [0x11F02, 0x0061] * 150, 150), + ("tamil ka virama", [0x0B95, 0x0BCD], 1), + ("family ZWJ", [0x1F468, 0x200D, 0x1F469, 0x200D, 0x1F467], 1), + ("heart + VS16", [0x2764, 0xFE0F], 1), + ("trailing lone ZWJ", [0x0061, 0x200D], 1), + ("leading ZWJ", [0x200D, 0x0061], 2), + ("ExtPict ZWJ non-ExtPict", [0x1F469, 0x200D, 0x0062], 2), + ("spacingmark devanagari aa", [0x0915, 0x093E], 1), + ("thai sara i", [0x0E01, 0x0E31], 1), + ("myanmar non-spacingmark Mc (1063)", [0x1000, 0x1063], 2), + ("myanmar non-spacingmark Mc (109C)", [0x1000, 0x109C], 2), + ("kawi vowel (post-Unicode-14 mark)", [0x11F00, 0x11F01], 1), + ("kawi sign 11F41 attaches", [0x11F04, 0x11F41], 1), + ("nag mundari 1E4EC attaches", [0x1E4D0, 0x1E4EC], 1), + ("egyptian hieroglyph control 13439", [0x13000, 0x13439, 0x0301], 3), + ("RI + extend", [0x1F1EB, 0xFE0F, 0x1F1F7], 2), + ("emoji + VS + ZWJ + emoji", [0x1F468, 0xFE0F, 0x200D, 0x1F469], 1), + ("digit + tag", [0x0031, 0xE0031], 1), + ("100x a-ZWJ-b", [0x0061, 0x200D, 0x0062] * 100, 200), +] + + +@pytest.mark.parametrize(("name", "codepoints", "expected"), ELIXIR_PARITY) +def test_grapheme_length_matches_elixir(name: str, codepoints: list, expected: int) -> None: + assert grapheme_length("".join(map(chr, codepoints))) == expected, name + + +@pytest.mark.parametrize(("name", "codepoints", "expected"), ELIXIR_PARITY) +def test_cap_never_exceeds_what_elixir_would_count( + name: str, codepoints: list, expected: int, +) -> None: + """Undercounting is the failure that matters: it emits a name Ecto rejects.""" + del expected + capped = sanitize_name("".join(map(chr, codepoints)) * 40, unicode_mode=True) + assert grapheme_length(capped) <= MAX_NAME_LENGTH, name + + +def test_the_regressions_the_reviews_found() -> None: + """`ab` must not join, a flag tag sequence must not be seven, and an + emoji ZWJ run must end at the joiner when a plain mark follows.""" + # GB11 joins across a ZWJ only when both sides are pictographic. Two plain + # letters are not, so this is two graphemes, not one. + assert grapheme_clusters("a\u200db") == ["a\u200d", "b"] + assert grapheme_clusters("a\u200db" * 100) == ["a\u200d", "b"] * 100 + + # The tag characters are Extend, so the whole flag sequence is one grapheme. + assert grapheme_clusters(SCOTLAND_FLAG) == [SCOTLAND_FLAG] + assert grapheme_clusters(SCOTLAND_FLAG * 15) == [SCOTLAND_FLAG] * 15 + + # Elixir ends an emoji ZWJ run at the joiner unless a pictograph follows, + # so this is two graphemes per copy, not one. Counting it as one meant a + # 200-grapheme name went out under a 100-grapheme cap. + assert grapheme_clusters("\u00a9\u200d\u0301") == ["\u00a9\u200d", "\u0301"] + copies = 200 + assert grapheme_length("\u00a9\u200d\u0301" * copies) == copies * len(["\u00a9\u200d", "\u0301"]) + assert grapheme_length( + sanitize_name("\u00a9\u200d\u0301" * copies, unicode_mode=True), + ) == MAX_NAME_LENGTH + + # ...but with a plain lead the mark still attaches, per GB9. + assert grapheme_clusters("a\u200d\u0301") == ["a\u200d\u0301"] + + +def test_prepend_families_are_recognised() -> None: + """`regex` misses these, which made it truncate at half the real limit.""" + for prepend in ("\U00011A3A", "\U00011A84", "\U00011D46", "\U00011F02", "\u0600"): + assert grapheme_clusters(prepend + "a") == [prepend + "a"] + copies = 150 + assert grapheme_length((prepend + "a") * copies) == copies + assert grapheme_length( + sanitize_name((prepend + "a") * copies, unicode_mode=True), + ) == MAX_NAME_LENGTH + + +def test_post_unicode_14_characters_are_classified() -> None: + """Python 3.11 ships Unicode 14, so these look unassigned without the lag table.""" + assert unicodedata.category("\U00011F41") == "Cn", "python caught up; the lag table can shrink" + assert grapheme_clusters("\U00011F04\U00011F41") == ["\U00011F04\U00011F41"] + assert grapheme_clusters("\U0001E4D0\U0001E4EC") == ["\U0001E4D0\U0001E4EC"] + # Egyptian hieroglyph format controls break on both sides. + assert grapheme_clusters("\U00013000\U00013439\u0301") == [ + "\U00013000", "\U00013439", "\u0301", + ] + + +def test_there_is_no_regex_dependency() -> None: + """Which algorithm runs must not depend on transitive resolution. + + `regex` is spec-correct, which is why it is wrong here — see `name_rules`. + """ + tree = ast.parse(inspect.getsource(name_rules)) + imported = { + alias.name.split(".")[0] + for node in ast.walk(tree) + if isinstance(node, ast.Import) + for alias in node.names + } | { + node.module.split(".")[0] + for node in ast.walk(tree) + if isinstance(node, ast.ImportFrom) and node.module + } + + assert imported == {"os", "unicodedata"}, imported + + +# --- trimming ----------------------------------------------------------------- + + +def test_trim_set_is_exactly_what_elixir_strips() -> None: + """Brute-forced against Elixir 1.18.3 over the whole codepoint space. + + Python's bare `.strip()` also eats U+001C-U+001F, which are not Unicode + White_Space. Trimming a different set from Lightning would mean the two + disagree about a name's identity, and step lookup would silently miss. + """ + elixir_trims = { + 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x20, 0x85, 0xA0, 0x1680, + 0x2000, 0x2001, 0x2002, 0x2003, 0x2004, 0x2005, 0x2006, + 0x2007, 0x2008, 0x2009, 0x200A, 0x2028, 0x2029, 0x202F, + 0x205F, 0x3000, + } + assert {ord(c) for c in _TRIM_CHARS} == elixir_trims + + python_only = {c for c in range(0x110000) if chr(c).strip() == "" and c != 0} - elixir_trims + assert python_only == {0x1C, 0x1D, 0x1E, 0x1F} + assert not (python_only & {ord(c) for c in _TRIM_CHARS}) + + +@pytest.mark.usefixtures("unicode_mode") +@pytest.mark.parametrize("space", ["\xa0", "\u2003", "\u3000", "\u205f", " "]) +def test_unicode_whitespace_is_trimmed(space: str) -> None: + assert sanitize_name(f"{space}Fetch Data{space}") == "Fetch Data" + + +# --- surrogates --------------------------------------------------------------- + + +@pytest.mark.parametrize("mode", ["false", "true"]) +def test_lone_surrogates_are_rejected(monkeypatch: pytest.MonkeyPatch, mode: str) -> None: + """A lone surrogate cannot be encoded as UTF-8, so it must never reach Lightning.""" + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + name = "Fetch\ud800Data\udfff" + + cleaned = sanitize_name(name) + + assert cleaned == "FetchData" + cleaned.encode("utf-8") # would raise if a surrogate survived + + +# --- case folding ------------------------------------------------------------- + + +def test_lookup_folds_case_rather_than_lowercasing() -> None: + """`.lower()` leaves these pairs distinct, so the lookups used to miss.""" + assert normalize_for_lookup("ΣΙΣ") == normalize_for_lookup("σις") + assert normalize_for_lookup("ΟΔΟΣ") == normalize_for_lookup("οδος") + assert normalize_for_lookup("STRASSE") == normalize_for_lookup("straße") + + +# --- Extended_Pictographic ---------------------------------------------------- + +#: What tools/unicode_parity produced against PARITY_SOURCE. These move only +#: when the tables are regenerated against a different Elixir or Python. +EXT_PICT_CODEPOINTS = 3537 +LAG_EXTEND_RANGES = 12 +LAG_CONTROL_RANGES = 7 + + +def test_extpict_is_not_over_broad() -> None: + """The bug a codepoint-bucket sweep structurally cannot find. + + ExtPict is not a break class, so classifying every codepoint into A/P/C/O + passes regardless of how wrong this set is. It only shows up either side of + a ZWJ. The hand-written ranges claimed hundreds of codepoints too many by + collapsing sparse sets into solid blocks. + """ + # U+2713 sits inside the old (0x2600, 0x27BF) block and is not pictographic. + assert not _is_ext_pict(0x2713), "CHECK MARK is not Extended_Pictographic" + assert not _is_ext_pict(0x219A), "arrows in the 0x2190 block are not pictographic" + assert _is_ext_pict(0x2764), "HEAVY BLACK HEART is" + assert _is_ext_pict(0x1F600) + + +def test_a_non_pictograph_does_not_join_across_a_zwj() -> None: + """`✓` is two graphemes to Elixir; calling it one shipped a + 200-grapheme name under a 100-grapheme cap.""" + assert grapheme_clusters("✓‍\U0001F600") == ["✓‍", "\U0001F600"] + copies = 100 + assert grapheme_length("✓‍\U0001F600" * copies) == copies * 2 + + # And the other direction: a mark after a non-pictograph's ZWJ still + # attaches under GB9, so this is one grapheme, and truncating it early + # would have cut a name Lightning accepts. + assert grapheme_clusters("✓‍́") == ["✓‍́"] + + +def test_the_extpict_set_matches_the_recorded_probe() -> None: + """Canary for Elixir moving forward. + + Regenerating the tables against a newer Elixir changes these sizes, which + fails here until PARITY_SOURCE is updated too. + """ + assert PARITY_SOURCE == {"elixir": "1.18.3", "otp": "27", "python_unicodedata": "14.0.0"} + assert unicodedata.unidata_version == PARITY_SOURCE["python_unicodedata"], ( + "Python's Unicode version moved; re-run tools/unicode_parity and update PARITY_SOURCE" + ) + # Pinned to what tools/unicode_parity produced against PARITY_SOURCE. + assert sum(b - a + 1 for a, b in name_rules._EXT_PICT_RANGES) == EXT_PICT_CODEPOINTS + assert len(name_rules._LAG_EXTEND) == LAG_EXTEND_RANGES + assert len(name_rules._LAG_CONTROL) == LAG_CONTROL_RANGES + + +# --- GB11 lookback ------------------------------------------------------------ + + +def test_the_emoji_run_lookback_crosses_a_spacing_mark() -> None: + """UAX #29 says Extend only; Elixir also crosses SpacingMark.""" + heart, zwj, emoji = "❤", "‍", "\U0001F600" + + assert grapheme_clusters(f"{heart}\u0903{zwj}{emoji}") == [f"{heart}\u0903{zwj}{emoji}"] + assert grapheme_clusters(f"{heart}́{zwj}{emoji}") == [f"{heart}́{zwj}{emoji}"] + assert grapheme_clusters(f"{heart}́\u0903{zwj}{emoji}") == [ + f"{heart}́\u0903{zwj}{emoji}", + ] + + # ...but not an ordinary character, and not a non-SpacingMark Mc. + assert grapheme_clusters(f"{heart}a{zwj}{emoji}") == [heart, f"a{zwj}", emoji] + assert grapheme_clusters(f"{heart}ၣ{zwj}{emoji}") == [heart, f"ၣ{zwj}", emoji] + + +# --- line and paragraph separators -------------------------------------------- + + +@pytest.mark.parametrize("mode", ["false", "true"]) +@pytest.mark.parametrize("separator", ["\u2028", "\u2029"]) +def test_line_separators_are_rejected( + monkeypatch: pytest.MonkeyPatch, mode: str, separator: str, +) -> None: + """PyYAML writes them literally and indents the continuation; yamerl, which + is what Lightning parses with, does not fold that back, so the stored name + grows YAML indentation inside it.""" + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + + cleaned = sanitize_name(f"Fetch{separator}Data") + + assert separator not in cleaned + assert cleaned == "Fetch Data" + assert not is_valid_name(f"a{separator}b") + + +def test_a_name_with_a_line_separator_survives_the_yaml_round_trip() -> None: + """The check PyYAML-only tests are blind to: it reads its own output back + correctly, so the damage is invisible from this side.""" + name = sanitize_name("Fetch\u2028Data", unicode_mode=True) + dumped = yaml.dump({"name": name}, allow_unicode=True) + + assert "\u2028" not in dumped + + # `str.split("\n")` does not split on U+2028 but `splitlines` does, which is + # the whole point: PyYAML writes the separator literally and indents the + # continuation, and a parser that treats it as a line break (yamerl, which + # is what Lightning uses) then reads that indentation back into the name. + # Splitting on "\n" made this assertion always pass, and it also fired on + # any name long enough for PyYAML to wrap. + assert len(dumped.splitlines()) == len(dumped.rstrip("\n").split("\n")) + + +def test_the_line_separator_assertion_would_catch_the_real_damage() -> None: + """Guards the test above: show the check fails on an unsanitised name.""" + dumped = yaml.dump({"name": "Fetch\u2028Data"}, allow_unicode=True) + + assert "\u2028" in dumped + assert len(dumped.splitlines()) != len(dumped.rstrip("\n").split("\n")) + + +# --- normalisation ------------------------------------------------------------ + + +@pytest.mark.parametrize("mode", ["false", "true"]) +def test_sanitized_names_are_fixed_points_of_normalisation( + monkeypatch: pytest.MonkeyPatch, mode: str, +) -> None: + """A name that has been through `sanitize_name` normalises to itself, which + is what `is_valid_name` stakes its answer on.""" + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + + for text in ( + "Vérifier l'état", + "\u0995\u09c7\u09be", + "A\u0302\u200c\u0323", + "患者確認", + # Leading and trailing whitespace matter here rather than being noise: + # trimming is what can uncover a mark that had nothing to compose onto. + " \u09cb", + "\t\u09cb ", + " \u0995\u094b", + " Vérifier l'état ", + ): + cleaned = sanitize_name(text) + assert sanitize_name(cleaned) == cleaned + assert normalize_nfc(cleaned) == cleaned + + +@pytest.mark.usefixtures("unicode_mode") +def test_trimming_does_not_leave_a_mark_uncomposed() -> None: + """Trimming happens after normalising, so it can leave behind a boundary + that has not been normalised. Repeat until it settles, or the sanitiser + returns a name it would then call invalid.""" + assert sanitize_name(" \u09cb") == "\u09cb" + assert is_valid_name(sanitize_name(" \u09cb")) diff --git a/services/global_chat/tests/unit/test_yaml_assertions.py b/services/global_chat/tests/unit/test_yaml_assertions.py new file mode 100644 index 00000000..09a7448b --- /dev/null +++ b/services/global_chat/tests/unit/test_yaml_assertions.py @@ -0,0 +1,276 @@ +"""Unit tests for `assert_no_special_chars`. + +The assertion is what the live acceptance suites lean on to catch the +sanitizer misbehaving, so it needs to fail on the things the sanitizer can get +wrong — not just on a job name with an `@` in it. +""" + +from pathlib import Path + +import pytest +from name_rules import MAX_NAME_LENGTH, UNICODE_FLAG_ENV, describe_rule_for_judge +from testing import judges +from testing.judges import load_judge +from testing.yaml_assertions import assert_no_special_chars + + +@pytest.fixture +def ascii_mode(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv(UNICODE_FLAG_ENV, "false") + + +@pytest.fixture +def unicode_mode(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv(UNICODE_FLAG_ENV, "true") + + +def _workflow(**overrides: object) -> dict: + data = { + "jobs": {"fetch": {"name": "Fetch"}, "send": {"name": "Send"}}, + "triggers": {"webhook": {"type": "webhook"}}, + "edges": { + "webhook->fetch": {"source_trigger": "webhook", "target_job": "fetch"}, + "fetch->send": {"source_job": "fetch", "target_job": "send"}, + }, + } + data.update(overrides) + return data + + +@pytest.mark.usefixtures("ascii_mode") +def test_accepts_a_clean_workflow() -> None: + assert_no_special_chars(_workflow()) + + +@pytest.mark.usefixtures("ascii_mode") +def test_rejects_a_bad_job_name() -> None: + workflow = _workflow(jobs={"fetch": {"name": "Vérifier l'état"}}) + workflow["edges"] = {} + + with pytest.raises(AssertionError, match="Job 'fetch' name"): + assert_no_special_chars(workflow) + + +@pytest.mark.usefixtures("ascii_mode") +def test_rejects_a_bad_job_key() -> None: + """Job keys were never checked, which is how the key/name asymmetry hid.""" + workflow = _workflow(jobs={"患者確認": {"name": "Check"}}, edges={}) + + with pytest.raises(AssertionError, match="Job key"): + assert_no_special_chars(workflow) + + +@pytest.mark.usefixtures("ascii_mode") +def test_rejects_a_bad_trigger_key() -> None: + """Triggers were not checked at all, which is how the unsanitized ones sailed past.""" + workflow = _workflow(triggers={"ウェブ": {"type": "webhook"}}, edges={}) + + with pytest.raises(AssertionError, match="Trigger key"): + assert_no_special_chars(workflow) + + +@pytest.mark.usefixtures("ascii_mode") +def test_rejects_a_name_over_the_length_cap() -> None: + """Uses is_valid_name, so the cap is checked; a character-set regex missed this.""" + workflow = _workflow(jobs={"fetch": {"name": "x" * (MAX_NAME_LENGTH + 1)}}, edges={}) + + with pytest.raises(AssertionError, match="does not obey the step-name rule"): + assert_no_special_chars(workflow) + + +@pytest.mark.usefixtures("ascii_mode") +def test_rejects_an_edge_whose_key_contradicts_its_endpoints() -> None: + workflow = _workflow( + edges={"fetch->nowhere": {"source_job": "fetch", "target_job": "send"}}, + ) + + with pytest.raises(AssertionError, match="does not match its own endpoints"): + assert_no_special_chars(workflow) + + +@pytest.mark.usefixtures("ascii_mode") +def test_accepts_the_collision_suffix_the_sanitizer_adds() -> None: + """Two edges between the same pair are legitimate, and the second gets a -N.""" + workflow = _workflow( + edges={ + "fetch->send": {"source_job": "fetch", "target_job": "send"}, + "fetch->send-2": {"source_job": "fetch", "target_job": "send"}, + }, + ) + + assert_no_special_chars(workflow) + + +@pytest.mark.usefixtures("unicode_mode") +def test_does_not_split_an_edge_key_on_an_arrow_inside_a_name() -> None: + """Splitting on the first "->" is exactly the ambiguity the sanitizer avoids.""" + workflow = { + "jobs": {"a->b": {"name": "a->b"}, "c": {"name": "C"}}, + "edges": {"a->b->c": {"source_job": "a->b", "target_job": "c"}}, + } + + assert_no_special_chars(workflow) + + +@pytest.mark.usefixtures("unicode_mode") +def test_permissive_mode_accepts_what_lightning_accepts() -> None: + workflow = { + "jobs": { + "Vérifier l'état": {"name": "Vérifier l'état"}, + "患者確認": {"name": "患者確認 ✅"}, + }, + "edges": { + "Vérifier l'état->患者確認": { + "source_job": "Vérifier l'état", + "target_job": "患者確認", + }, + }, + } + + assert_no_special_chars(workflow) + + +@pytest.mark.usefixtures("unicode_mode") +def test_permissive_mode_still_rejects_a_control_character() -> None: + workflow = _workflow(jobs={"fetch": {"name": "Fetch\x00Data"}}, edges={}) + + with pytest.raises(AssertionError, match="does not obey the step-name rule"): + assert_no_special_chars(workflow) + + +# --- the judges are generated from the same rule ------------------------------ + + +def test_judges_state_the_active_rule(monkeypatch: pytest.MonkeyPatch) -> None: + """The rubrics used to restate the rule as static prose, a third copy. + + With the ASCII rule active a hardcoded permissive rubric would pass a name + the sanitizer is in fact folding, so the judge could no longer catch Apollo + misbehaving. + """ + monkeypatch.setenv(UNICODE_FLAG_ENV, "false") + ascii_rules = load_judge("general").rules + assert describe_rule_for_judge() in ascii_rules + assert "unaccented English letters" in ascii_rules + + monkeypatch.setenv(UNICODE_FLAG_ENV, "true") + unicode_rules = load_judge("general").rules + assert describe_rule_for_judge() in unicode_rules + assert "no control characters" in unicode_rules + + assert ascii_rules != unicode_rules + + +#: Every judge on disk, not a hardcoded pair — a new rubric that restates the +#: rule by hand would otherwise never be checked. +ALL_JUDGES = sorted(p.stem for p in (Path(judges.__file__).parent / "judges").glob("*.md")) + + +def test_the_judge_list_is_not_empty() -> None: + """Guards the glob: an empty list would make every test below vacuous.""" + assert ALL_JUDGES + + +@pytest.mark.parametrize("judge", ALL_JUDGES) +def test_no_judge_leaves_the_placeholder_unsubstituted(judge: str) -> None: + config = load_judge(judge) + assert "{name_rule}" not in config.rules + assert "{name_rule}" not in config.role + + +@pytest.mark.parametrize("judge", ALL_JUDGES) +def test_a_judge_that_uses_the_token_gets_the_active_rule(judge: str) -> None: + """Not every judge needs it — the code-quality one grades job bodies, not names.""" + raw = (Path(judges.__file__).parent / "judges" / f"{judge}.md").read_text() + if "{name_rule}" not in raw: + pytest.skip(f"{judge} does not grade names") + + assert describe_rule_for_judge() in load_judge(judge).rules + + +@pytest.mark.parametrize("judge", ALL_JUDGES) +def test_no_judge_hardcodes_a_naming_rule(judge: str) -> None: + """A hand-written charset is the drift this whole indirection exists to stop.""" + raw = (Path(judges.__file__).parent / "judges" / f"{judge}.md").read_text().lower() + + for phrase in ( + "letters, numbers, spaces", + "letters, digits, spaces", + "hyphens, and underscores", + "no special characters", + ): + assert phrase not in raw, f"{judge} states the naming rule itself; use {{name_rule}}" + + +def test_a_mangled_placeholder_is_rejected_loudly(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """`str.replace` is a silent no-op on a misspelled token, so it must raise.""" + monkeypatch.setattr(judges, "_JUDGES_DIR", tmp_path) + (tmp_path / "typo.md").write_text("# role\nA judge\n\n# rules\n- {name_rules}\n") + + with pytest.raises(ValueError, match="unsubstituted placeholders"): + load_judge("typo") + + +def test_prose_braces_are_not_mistaken_for_placeholders( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + """The code-quality judge is full of JS snippets; none of them may trip it.""" + monkeypatch.setattr(judges, "_JUDGES_DIR", tmp_path) + (tmp_path / "code.md").write_text( + "# role\nA judge\n\n# rules\n- `create({ name: $.patient.name })` and `() => {}`\n", + ) + + assert load_judge("code").rules + + +# --- null sections ------------------------------------------------------------ + + +@pytest.mark.parametrize( + "workflow", + [ + {"jobs": None}, + {"edges": None}, + {"triggers": None}, + {"jobs": None, "edges": None, "triggers": None}, + {}, + ], +) +def test_tolerates_an_empty_section(workflow: dict) -> None: + """`edges:` with nothing under it is valid YAML and parses as None.""" + assert_no_special_chars(workflow) + + +# --- referential integrity ---------------------------------------------------- + + +@pytest.mark.usefixtures("ascii_mode") +def test_rejects_an_edge_pointing_at_a_job_that_does_not_exist() -> None: + """A well-formed name that names nothing is what a broken mapping produces.""" + workflow = _workflow(edges={"fetch->ghost": {"source_job": "fetch", "target_job": "ghost"}}) + + with pytest.raises(AssertionError, match="is not a job in this workflow"): + assert_no_special_chars(workflow) + + +@pytest.mark.usefixtures("ascii_mode") +def test_rejects_a_trigger_reference_that_names_a_job() -> None: + """The exact shape a shared job/trigger key mapping produced.""" + workflow = _workflow( + edges={"fetch->send": {"source_trigger": "fetch", "target_job": "send"}}, + ) + + with pytest.raises(AssertionError, match="is not a trigger in this workflow"): + assert_no_special_chars(workflow) + + +@pytest.mark.usefixtures("ascii_mode") +def test_rejects_an_over_long_edge_key() -> None: + long_name = "a" * MAX_NAME_LENGTH + workflow = { + "jobs": {long_name: {"name": "A"}, "b": {"name": "B"}}, + "edges": {f"{long_name}->{long_name}->b": {"source_job": long_name, "target_job": "b"}}, + } + + with pytest.raises(AssertionError): + assert_no_special_chars(workflow) diff --git a/services/global_chat/tests/unit/test_yaml_utils.py b/services/global_chat/tests/unit/test_yaml_utils.py index 9d24f86b..4680ee3f 100644 --- a/services/global_chat/tests/unit/test_yaml_utils.py +++ b/services/global_chat/tests/unit/test_yaml_utils.py @@ -7,11 +7,15 @@ from yaml_utils import ( REDACTED_BODY, WITHHELD_NOTICE, + find_job_in_yaml, + get_page_view, + get_step_name_from_page, has_unredacted_body, inspect_job_code, iter_body_holders, redact_job_bodies, remove_ids, + stitch_job_code, ) WORKFLOW_YAML = """\ @@ -80,6 +84,77 @@ def test_inspect_handles_missing_yaml_and_keys() -> None: assert inspect_job_code(WORKFLOW_YAML, []) == "ERROR: No job keys provided." +# --- step lookup by name ---------------------------------------------------- + +NON_LATIN_WORKFLOW_YAML = """\ +name: wf +jobs: + patient-check: + name: 患者確認 + body: get('/patients'); + send-data: + name: データ送信 + body: post('/data', $.data); + verify: + name: Проверка данных + body: check(); +""" + + +def test_find_job_matches_the_right_non_latin_name() -> None: + key, job = find_job_in_yaml(NON_LATIN_WORKFLOW_YAML, "データ送信") + assert key == "send-data" + assert job["name"] == "データ送信" + + +def test_find_job_does_not_cross_match_non_latin_names() -> None: + """Every non-Latin name used to normalize to "", so any non-Latin lookup + matched the first non-Latin job in the workflow.""" + key, job = find_job_in_yaml(NON_LATIN_WORKFLOW_YAML, "Проверка") + assert key is None + assert job is None + + key, _ = find_job_in_yaml(NON_LATIN_WORKFLOW_YAML, "Проверка данных") + assert key == "verify" + + +def test_find_job_lookup_is_still_fuzzy_for_latin_names() -> None: + assert find_job_in_yaml(WORKFLOW_YAML, "Fetch Patients")[0] == "fetch-patients" + assert find_job_in_yaml(WORKFLOW_YAML, "FETCH-PATIENTS")[0] == "fetch-patients" + + +def test_find_job_ignores_a_lookup_key_with_nothing_to_match_on() -> None: + assert find_job_in_yaml(NON_LATIN_WORKFLOW_YAML, "!!!") == (None, None) + assert find_job_in_yaml(NON_LATIN_WORKFLOW_YAML, "") == (None, None) + + +def test_find_job_matches_a_decomposed_name() -> None: + yaml_str = "jobs:\n verify:\n name: V\u00e9rifier\n" + assert find_job_in_yaml(yaml_str, "Ve\u0301rifier")[0] == "verify" + + +# --- page breadcrumb parsing ------------------------------------------------ + + +def test_page_view_classifies_the_three_shapes() -> None: + assert get_page_view("workflows/wf") == ("overview", None) + assert get_page_view("workflows/wf/settings") == (None, None) + assert get_page_view("workflows/wf/fetch-patients") == ("step", "fetch-patients") + assert get_page_view("projects/p") == (None, None) + assert get_page_view(None) == (None, None) + assert get_page_view("workflows") == (None, None) + + +def test_page_view_keeps_a_slash_inside_a_step_name() -> None: + """A step name containing "/" used to silently lose the step focus.""" + assert get_page_view("workflows/wf/Import A/B") == ("step", "Import A/B") + assert get_step_name_from_page("workflows/wf/Import A/B") == "Import A/B" + + +def test_page_view_keeps_a_non_latin_step_name() -> None: + assert get_step_name_from_page("workflows/wf/患者確認") == "患者確認" + + # --- redaction must never fall back to the unredacted document ---------------- WORKFLOW_WITH_NULL_JOB = """\ @@ -168,6 +243,22 @@ def test_withholding_tells_the_model_what_happened() -> None: assert "Do not conclude that it is empty" in withheld +def test_stitching_a_missing_job_is_reported(caplog: pytest.LogCaptureFixture) -> None: + """It returns the original either way; the planner logs success regardless, + so silence here meant the generated code vanished without trace.""" + with caplog.at_level("ERROR"): + stitch_job_code(WORKFLOW_YAML, "no-such-job", "get('/x');") + + assert any("discarded" in record.message for record in caplog.records) + + +def test_stitch_tolerates_a_null_job_entry() -> None: + stitched = stitch_job_code(WORKFLOW_WITH_NULL_JOB, "fetch", "post('/x');") + + assert "post('/x');" in stitched + assert stitch_job_code(WORKFLOW_WITH_NULL_JOB, "half-written", "x();") is not None + + # --- one walker, every shape --------------------------------------------------- @@ -295,3 +386,39 @@ def test_remove_ids_still_walks_tuples() -> None: remove_ids(data) assert "id" not in data["jobs"][0][1] + + +# --- the fuzzy lookup writes, so it must not guess ----------------------------- + +AMBIGUOUS_WORKFLOW = """\ +name: wf +jobs: + upload-data: + name: Legacy uploader + body: legacy(); + upload-data-2: + name: Upload Data + body: current(); +""" + + +def test_an_exact_name_beats_an_earlier_key_fold() -> None: + """The result goes to `stitch_job_code`, which replaces that step's body. + Taking the first fold hit let `upload-data`'s key fold beat + `upload-data-2`'s exact name, so the model's code overwrote the legacy + step.""" + assert find_job_in_yaml(AMBIGUOUS_WORKFLOW, "Upload Data")[0] == "upload-data-2" + + +def test_an_exact_key_still_wins_outright() -> None: + assert find_job_in_yaml(AMBIGUOUS_WORKFLOW, "upload-data")[0] == "upload-data" + + +def test_an_ambiguous_fold_is_refused_rather_than_guessed() -> None: + workflow = "jobs:\n a:\n name: Fetch Data\n b:\n name: fetch-data\n" + + assert find_job_in_yaml(workflow, "FETCH DATA") == (None, None) + + +def test_an_unambiguous_fold_still_resolves() -> None: + assert find_job_in_yaml(AMBIGUOUS_WORKFLOW, "upload data 2")[0] == "upload-data-2" diff --git a/services/job_chat/prompt_online.py b/services/job_chat/prompt_online.py deleted file mode 100644 index 72206dd0..00000000 --- a/services/job_chat/prompt_online.py +++ /dev/null @@ -1,457 +0,0 @@ -import json -import time -import sentry_sdk -from langfuse import observe -from util import create_logger, ApolloError, AdaptorSpecifier, get_db_connection -from .retrieve_docs import retrieve_knowledge -from search_adaptor_docs.search_adaptor_docs import fetch_signatures - -logger = create_logger("job_chat.prompt") - -system_role = """ -You are a software engineer helping a non-expert user write a job for our platform. -We are OpenFn (Open Function Group) the world's leading digital public good for workflow automation. - -Where reasonable, assume questions are related to workflow automation, -professional platforms or programming. You may provide general information around these topics, -e.g. general programming assistance unrelated to job writing. -If a question is entirely irrelevant, do not answer it. - -Keep your responses concise and lead with the answer. Explain only as much as -the user's question needs. When generating code, always use the simplest -possible code to achieve the task. - -Do not thank the user or be obsequious. Address the user directly. - -You are embedded in our app for building workflows. Our app will provide the -history of each chat session to you. Our app will send you the user's code and -tell you which adaptor (library) is being used. -Chat sessions are saved to each job, so any user who can see the workflow can see the chat. - -Your chat panel is embedded in a web based IDE, which lets users build a Workflow with a number -of steps (or jobs). There is a code editor next to you, which users can copy and paste code into. -Users must set or select an input in the Input tab, and can then run the current job. - -You ONLY help with job code. Do NOT help with overall workflow structure. -If the user wants to add/remove/edit workflow steps, tell them to navigate to the workflow overview. - -Users can Flag any answers that are not helpful, which will help us build a better prompt for you. - - -The system will provide you with various pieces of context about the user's job using XML tags: - -- : The current job code the user is working on. This is the code they want help with. -- : Documentation for the adaptor (library) the user is using. Reference this when suggesting functions. -- : Sample input data the user is testing with. Shows what data structure enters the job. -- : The output data from a previous run. Shows what the job produced. -- : Execution logs from when the user ran their job. These contain console.log output, - error messages, and system logs. Use these logs to diagnose errors and understand what happened - during execution. When logs are present, you should analyze them carefully to identify the root - cause of any issues. - -When the user asks you to check logs or debug an error, the tag will contain the -relevant execution information. Pay close attention to error messages, stack traces, and the -sequence of log statements to understand what went wrong. - -Earlier turns may have pertained to different context (workflow structure or other job steps) that is no longer -attached. Any previously generated code has been redacted from history. Some turns may have a [pg:...] -prefix showing the user's page context at that time. - -""" - -job_writing_summary = """ - -When writing jobs, users will use their own credentials to access different -backend systems. The OpenFn app handles all credential management for them -in a secure way. - -For more help direct them to https://docs.openfn.org/documentation/build/credentials - -Users must never add credentials into job code directly. If a user gives you an -API key, password, access token, or other credential, you must reject it. - - -An OpenFn Job is written in a DSL which is very similar to Javascript. - -Job code does not use import statements or async/await. - -Job code must only contain function calls at the top level. - -If the user is talking about collections, suggest this: "For working with collections, refer to the official documentation here: https://docs.openfn.org/adaptors/packages/collections-docs.". -Avoid suggesting code to a user enquiring about collections or a single collection. - -Each job is associated with an adaptor, which provides functions for the job. -All jobs have the fn() and each() function, which are very important. - -DO NOT use the `alterState()` function. Use `fn()` instead. - -The adaptor API may be attached. - -The functions provided by an adaptor are called Operations. -Know that technically an Operation is a factory function which returns a function that takes state and returns state, like this: -```js -const myOperation = (arg) => (state) => { /* do something with arg and state */ return state; } -``` -But the DSL presents these operations like simple functions. Users don't know it's a factory, they think it's a regular function. - - -Here's how we issue a GET request with the http adaptor: -``` -get('/patients'); -``` -The first argument to get is the path to request from (the configuration will tell -the adaptor what base url to use). In this case we're passing a static string, -but we can also pass a value from state: -``` -get(state => state.endpoint); -``` - - -Example job code with the HTTP adaptor: -``` -get('/patients'); -fn(state => { - const patients = state.data.map(p => { - return { ...p, enrolled: true } - }); - - return { ...state, data: { patients } }; -}) -post('/patients', dataValue('patients')); - - -``` -Example job code with the Salesforce adaptor: -``` -each( - '$.form.participants[*]', - upsert('Person__c', 'Participant_PID__c', state => ({ - Participant_PID__c: state.pid, - First_Name__c: state.participant_first_name, - Surname__c: state.participant_surname, - })) -); -``` - - -Example job code with the ODK adaptor: -``` -create( - 'ODK_Submission__c', - fields( - field('Site_School_ID_Number__c', dataValue('school')), - field('Date_Completed__c', dataValue('date')), - field('comments__c', dataValue('comments')), - field('ODK_Key__c', dataValue('*meta-instance-id*')) - ) -); -``` - - - - -A job is just one step in a workflow (or pipeline). Workflows are used -to automate processes and migrate data from system to system. - -In OpenFn, each step works with a single backend system, or adaptor. Data is shared -between steps through the state object. - -To build a successful workflow, we have to take the user's problem and break it down -step by step. Focus on one bit at a time. For example, when uploading from CommCare to Salesforce, we have to: -1. Download our data from CommCare in one step -2. Transform/map data into salesforce format in another step (with the common adaptor) -3. Upload the transformed data into salesforce in the final step - - - -You must respond in JSON format with two fields: - -{ - "code_edits": [], - "text_answer": "Your conversational response here" -} - -"code_edits" are applied directly to the user's job code — a "rewrite", or a "replace" of the whole body, will overwrite whatever they currently have. So reach for code_edits when the user actually wants their job changed. When you're explaining, teaching, or showing an illustrative example that shouldn't disturb their current work, put the code inline in "text_answer" as a markdown code block instead. Judge from what the user is asking which they want — and if they clearly want the example in their job, edit it; if they just want to understand or see it, keep it inline. -Use "text_answer" for all explanations, guidance, and conversation. -The user will see these code edits as suggestions in their separate code panel, so avoid ending on a colon. - -Code edit actions: -{ - "action": "replace", - "old_code": "exact code to find and replace", - "new_code": "replacement code" -} - -{ - "action": "rewrite", - "new_code": "complete new code" -} - - -- The old_code must match exactly, including all whitespace and indentation -- Apply edits sequentially - later edits work on the already-modified code -- If old_code is not found exactly, the edit will fail safely rather than corrupt the file - -To insert new code using replace: -- Find a suitable insertion point and replace it with itself plus the new code -- Example: To insert after "get('/patients');", replace it with "get('/patients');\n[new code here]" - -**IMPORTANT: INCLUDE CONTEXT TO AVOID DUPLICATE MATCHES** -We will use literal string replacement to apply your changes. To avoid duplicate matches, you MUST: -1. Include ample surrounding context in the old_code to replace (comments, variable declarations, both similar passages etc.) -2. If in doubt, use "rewrite" action instead to rewrite the whole code - -**Output valid JSON strings** -Your answer MUST be parsable with json.loads() -This means that all string values in your JSON (including "old_code", "new_code", and "text_answer") must be valid JSON strings. -- Escape all newlines as \\n (one backslash followed by n) -- Escape all double quotes as \\" (one backslash followed by double quotation mark) -- Do not include unescaped control characters in any string value. -- When you include code in a string, ensure it is a single line with \\n for line breaks. - -Example: -{ - "code_edits": [{ - "action": "replace", - "old_code": "get('/patients');", - "new_code": "get('/patients');\\nfn(state => {\\n if (!state.data) {\\n throw new Error(\\\"No data received\\\");\\n }\\n return state;\\n});" - }], - "text_answer": "I'll add error handling after your GET request" -} - -ALWAYS use \\n instead of actual newlines: -THIS IS WRONG: -"new_code": "function() { - return true; -}" - -THIS IS CORRECT: -"new_code": "function() {\\n return true;\\n}" - - -""" - -error_correction_system_prompt = """ -You are a code edit correction assistant. A code edit failed because the string replacement system couldn't find a unique match. - -CRITICAL: You are working with a LITERAL STRING REPLACEMENT system, not a semantic code editor. - -The system has tried to look for old_code in the full_original_code, and substitute it with new_code. -Your task is to understand the intended change from the given context and attempted replacement, to output a corrected attempt for the string replacement system. -The correction system will look for your corrected_old_code in full_original_code and substitute it with your corrected_new_code. - -Context to use: -You will be given relevant context under "Original edit details" below. -This may include an explanation of attempted changes. Note that this may describe a broader change/series of changes but you will only be shown a specific edit to fix. - -Common issues: -1. "old_code not found" - the old_code doesn't exactly match what's in the file - --> Look at the full code and find the closest matching section -2. "old_code matches multiple locations" - the old_code appears multiple times - --> Add more surrounding context to make the old_code unique for string replacement. - **CRITICAL**: Take care to include the intended context in the corrected_new_code so that the substitution does not result in deletions or duplications. -3. "Replace action requires old_code and new_code" - missing required fields - --> Either/both fields missing. Use the given context and full code to fill these. - -It is important to: -- Preserve the intended change from the original new_code -- Maintain exact whitespace and formatting -- Include enough context in old_code to make it unique - -Output JSON format: -{ - "explanation": "1-sentence explanation of the correction", - "corrected_old_code": "corrected old code with proper context", - "corrected_new_code": "corrected new code" -} - -**Output valid JSON strings** -Your answer MUST be parsable with json.loads() -- Escape all newlines as \\n (one backslash followed by n) -- Escape all double quotes as \\" (one backslash followed by double quotation mark) -- Do not include unescaped control characters in any string value. -- When you include code in a string, ensure it is a single line with \\n for line breaks. - -ALWAYS use \\n instead of actual newlines: -THIS IS WRONG: -"corrected_new_code": "function() { - return true; -}" - -THIS IS CORRECT: -"corrected_new_code": "function() {\\n return true;\\n}" -""" - - -class Context: - def __init__(self, **kwargs): - self.__dict__.update(kwargs) - - def has(self, key): - return hasattr(self, key) and getattr(self, key) is not None - - -def generate_system_message(context_dict, search_results, download_adaptor_docs=True, stream_manager=None): - context = context_dict if isinstance(context_dict, Context) else Context(**(context_dict or {})) - - message = [system_role] - message.append(f"{job_writing_summary}") - message.append({"type": "text", "text": ".", "cache_control": {"type": "ephemeral"}}) - - if search_results: - search_results = format_search_results(search_results) - message.append(f"General OpenFn documentation search results. These cover platform concepts only — not adaptor-specific APIs, which are included separately. Treat with caution if not relevant to the user's situation.\n\n{search_results}") - message.append({"type": "text", "text": ".", "cache_control": {"type": "ephemeral"}}) - - if context.has("adaptor"): - adaptor_string = ( - f"The user is using the OpenFn {context.adaptor} adaptor. Use functions provided by its API.\n\n" - ) - - try: - conn = get_db_connection() - - try: - try: - adaptor = AdaptorSpecifier(context.adaptor) - - signatures = fetch_signatures(adaptor, conn, auto_load=download_adaptor_docs) - - if signatures: - adaptor_string += "These are the available functions in the adaptor:\n\n" - for func_name, signature in signatures.items(): - adaptor_string += f"{signature}\n" - else: - msg = f"No adaptor signatures returned from search_adaptor_docs for {adaptor.specifier}" - logger.warning(msg) - sentry_sdk.capture_message(msg, level="warning") - sentry_sdk.set_context("adaptor_context", { - "adaptor_name": adaptor.name, - "version": adaptor.version, - "parsed_from": context.adaptor - }) - adaptor_string += "The user is using an OpenFn Adaptor to write the job." - except Exception as parse_error: - msg = f"Failed to parse adaptor string '{context.adaptor}': {parse_error}" - logger.warning(msg) - sentry_sdk.capture_message(msg, level="warning") - sentry_sdk.set_context("adaptor_context", { - "parsed_from": context.adaptor, - "error": str(parse_error) - }) - adaptor_string += "The user is using an OpenFn Adaptor to write the job." - finally: - conn.close() - except ApolloError as e: - logger.warning(f"Database not available: {e.message}") - adaptor_string += "The user is using an OpenFn Adaptor to write the job." - except Exception as e: - logger.warning(f"Could not fetch adaptor docs for {context.adaptor}: {e}") - adaptor_string += "The user is using an OpenFn Adaptor to write the job." - - if len(adaptor_string) >= 40000: - adaptor_string = adaptor_string[:40000] - adaptor_string += "(...)" - - adaptor_string += "" - - message.append(adaptor_string) - else: - message.append("The user is using an OpenFn Adaptor to write the job.") - - message.append({"type": "text", "text": ".", "cache_control": {"type": "ephemeral"}}) - - if context.has("expression"): - message.append(f"{context.expression}") - - if context.has("input"): - message.append(f"The user's input data is :\n\n```{context.input}```") - - if context.has("output"): - message.append(f"The user's last output data was :\n\n```{context.output}```") - - if context.has("log"): - message.append(f""" -IMPORTANT: The user has included execution logs from their last workflow run below. -These logs contain the actual runtime output including console.log statements, error messages, -and system information. When debugging, analyze these logs carefully to identify: -- Error messages and their root causes -- The sequence of operations that executed -- Any unexpected behavior or missing output -- Stack traces if errors occurred - -```{context.log}``` -""") - - return list(map(lambda text: text if isinstance(text, dict) else {"type": "text", "text": text}, message)) - -def format_search_results(search_results): - return '\n'.join([ - f'search result: "{result.get("text")}", source: "{result.get("metadata", {}).get("doc_title", "")} {result.get("medatada", {}).get("docs_type", "")}"' - for result in search_results - ]) - -@observe(name="job_chat_build_prompt") -def build_prompt(content, history, context, rag=None, api_key=None, stream_manager=None, download_adaptor_docs=True, refresh_rag=False): - retrieved_knowledge = { - "search_results": [], - "search_results_sections": [], - "search_queries": [], - "config_version": "", - "prompts_version": "", - "usage": { - "needs_docs": {}, - "generate_queries": {} - } - } - - # Run RAG if: (a) no RAG data provided, OR (b) refresh_rag flag is True - if rag and not refresh_rag: - retrieved_knowledge = rag - else: - try: - retrieved_knowledge = retrieve_knowledge( - content=content, - history=history, - code=context.get("expression", ""), - adaptor=context.get("adaptor", ""), - api_key=api_key, - stream_manager=stream_manager, - ) - except Exception as e: - logger.error(f"Error retrieving knowledge: {str(e)}") - - system_message = generate_system_message( - context_dict=context, - search_results=retrieved_knowledge.get("search_results") if retrieved_knowledge is not None else None, - download_adaptor_docs=download_adaptor_docs, - stream_manager=stream_manager) - - prompt = [] - prompt.extend(history) - prompt.append({"role": "user", "content": content}) - - return (system_message, prompt, retrieved_knowledge) - -def build_error_correction_prompt(content: str, error_message: str, old_code: str, new_code: str, full_code: str, text_explanation: str): - """Build a prompt for correcting code edit errors.""" - - system_message = [{"type": "text", "text": error_correction_system_prompt}] - - user_content = f"""A code edit failed with this error: "{error_message}" - -Original edit details: -- old_code:\n{json.dumps(old_code)} -- attempted to replace the above with new_code:\n{json.dumps(new_code)} -- the user's original message:\n{content} -- explanation of (all) attempted changes:\n{text_explanation} -- full_original_code: -``` -{full_code} -``` - -Please provide corrected old_code and new_code that will successfully apply the intended change with string replacement.""" - - prompt = [{"role": "user", "content": user_content}] - logger.info(f"prompt in full:\n{prompt}") - return (system_message, prompt) \ No newline at end of file diff --git a/services/langfuse_util.py b/services/langfuse_util.py index 4772b2a1..0a99926c 100644 --- a/services/langfuse_util.py +++ b/services/langfuse_util.py @@ -181,7 +181,7 @@ def _normalize_yaml(yaml_str: str) -> str: return yaml_str if not isinstance(data, dict): return yaml_str - return yaml.dump(data, Dumper=_BlockScalarDumper, sort_keys=False) + return yaml.dump(data, Dumper=_BlockScalarDumper, sort_keys=False, allow_unicode=True) def build_generation_diff( diff --git a/services/name_rules.py b/services/name_rules.py new file mode 100644 index 00000000..de2c571e --- /dev/null +++ b/services/name_rules.py @@ -0,0 +1,620 @@ +r"""Single source of truth for which characters a workflow step name may contain. + +Lightning validates step names on its side and Apollo sanitises them on this +side. The two rules have to agree: if Apollo strips a character Lightning would +have accepted, Apollo silently renames a step the user deliberately named; if +Apollo emits a character Lightning rejects, the workflow fails to save. Of the +two, Apollo being *stricter* is the worse failure -- that is the silent +vandalism issue #446 exists to stop -- so the permissive rule below is +deliberately maximal. + +Lightning is lifting its restriction (Lightning#4577) from ASCII-only to +"anything except control characters". The two releases cannot ship at the same +instant, so the rule here is switchable at runtime: + + APOLLO_UNICODE_STEP_NAMES=false (default) -- today's ASCII-only behaviour, + matching Lightning's current ``~r/^[a-zA-Z0-9_\- ]*$/``. + APOLLO_UNICODE_STEP_NAMES=true -- anything except control + characters. Letters and marks from any script, all punctuation and + symbols, emoji, ``/``, ``:``, ``>``, ``&``, quotes and apostrophes. + +Deploy Apollo first with the default, flip the flag once Lightning ships. + +Both modes reject the same set, and nothing else is ever rejected in permissive +mode: + + C0 U+0000-U+001F (NUL included) + DEL U+007F + C1 U+0080-U+009F + noncharacters U+FFFE, U+FFFF + surrogates U+D800-U+DFFF (cannot be encoded as UTF-8 at all) + separators U+2028, U+2029 (do not survive the YAML round trip) + +A NUL byte in a name crashes the Postgres insert on Lightning's side +(Lightning#4893), so it is never permitted regardless of which rule is active. + +Both modes normalise to NFC and cap the name at 100 *graphemes*, counted the +way Elixir counts them (see the grapheme section below). Ecto's +``validate_length`` counts graphemes, so counting codepoints here would let a +name through that Lightning then rejects. + +On normalisation: Lightning's ``main`` still carries the ASCII-only regex at +``job.ex`` and does not normalise. The NFC normalisation this module is +matching is on Lightning's ``4577-unicode-step-names`` branch, unmerged at the +time of writing -- so treat "Lightning normalises to NFC" as the agreed plan, +not as shipped behaviour, and re-check the branch before flipping the flag. +Step lookup matches names as text, so if the two sides ever disagree on the +normal form, a lookup for a name containing an accent silently misses. +""" + +import os +import unicodedata + +UNICODE_FLAG_ENV = "APOLLO_UNICODE_STEP_NAMES" + +_TRUTHY = frozenset({"1", "true", "t", "yes", "y", "on"}) + +#: Permitted in both modes, alongside the letters and digits. +BASE_PUNCTUATION = " -_" + +_ASCII_ALNUM = frozenset( + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789", +) + +_ASCII_ALLOWED = _ASCII_ALNUM | frozenset(BASE_PUNCTUATION) + +#: Used only by the lookup normalizer, not by the name rule. +_LETTER_MARK_DIGIT = ("L", "M", "N") + +#: Longest name Lightning will store, counted in graphemes because Ecto's +#: ``validate_length`` counts graphemes. +MAX_NAME_LENGTH = 100 + +# How many times sanitize_name may re-trim and re-normalise before giving up. +# Two passes settle every case found so far; the rest is margin. +_SANITIZE_PASSES = 4 + +#: Longest edge key. An edge label is ``source->target``, so two names at the +#: limit would otherwise make a key over twice the length of anything else in +#: the document. +MAX_EDGE_KEY_LENGTH = MAX_NAME_LENGTH * 2 + len("->") + +#: Letters NFKD cannot decompose, so under the ASCII rule they would vanish and +#: take the word with them (``straße`` -> ``strae``). Spelled out instead. +_ASCII_TRANSLITERATIONS = str.maketrans({ + "ß": "ss", "ẞ": "SS", + "æ": "ae", "Æ": "AE", + "œ": "oe", "Œ": "OE", + "ø": "o", "Ø": "O", + "đ": "d", "Đ": "D", + "ð": "d", "Ð": "D", + "þ": "th", "Þ": "TH", + "ł": "l", "Ł": "L", + "ı": "i", "ŋ": "n", "Ŋ": "N", # noqa: RUF001 - dotless i is the character being mapped +}) + +_C0 = range(0x20) # NUL through US +_DEL = 0x7F +_C1 = range(0x80, 0xA0) +_NONCHARACTERS = frozenset({0xFFFE, 0xFFFF}) + +#: Lone surrogates. Python will hold one in a str (a YAML or JSON payload can +#: carry a bare \ud800), but it cannot be encoded as UTF-8, so letting one +#: through would hand Lightning a name it cannot store. +_SURROGATES = range(0xD800, 0xE000) + +#: LINE SEPARATOR and PARAGRAPH SEPARATOR. Not control characters by category, +#: but they cannot survive the round trip: PyYAML with ``allow_unicode=True`` +#: writes U+2028 literally and then indents the continuation, and ``yamerl``, +#: which is what Lightning parses with, does not fold that back. A name goes in +#: at 11 graphemes and comes out of Lightning at 17 with six spaces of YAML +#: indentation inside it. PyYAML reads its own output back correctly, so this +#: is invisible from this side of the wire. Lightning rejects them too. +_LINE_SEPARATORS = frozenset({0x2028, 0x2029}) + +#: Exactly what Elixir's `String.trim/1` strips, verified by brute-forcing the +#: whole codepoint space against Elixir 1.18.3: the 25 Unicode White_Space +#: characters. Python's bare `str.strip()` also eats U+001C-U+001F, which are +#: not White_Space, so trimming with an explicit set is what keeps Apollo and +#: Lightning agreeing on a name's identity. +_TRIM_CHARS = "\u0009\u000a\u000b\u000c\u000d\u0020\u0085\u00a0\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200a\u2028\u2029\u202f\u205f\u3000" + +#: What the generated tables below were probed from. `tools/unicode_parity` +#: regenerates them; if you re-run it against a different Elixir, update this +#: too -- a unit test pins it, so a silent regeneration fails loudly. +#: +#: Python moving forward only makes this module overcount, which truncates +#: early. Elixir moving forward is the dangerous direction: it undercounts, and +#: an undercount ships a name Ecto rejects. +PARITY_SOURCE = {"elixir": "1.18.3", "otp": "27", "python_unicodedata": "14.0.0"} + + +def unicode_names_enabled() -> bool: + """Return True when the Unicode-permissive rule is active. + + Read at call time rather than import time so tests (and a redeploy that + only changes the environment) do not need the module reloaded. + """ + return os.getenv(UNICODE_FLAG_ENV, "false").strip().lower() in _TRUTHY + + +def _is_forbidden(char: str) -> bool: + """True for the characters rejected in every mode (see the module docstring). + + A codepoint test, not a general-category test. Category ``C`` also covers + format characters such as ZWJ (U+200D), which emoji sequences need, and + private-use and unassigned codepoints, all of which Lightning accepts. + """ + code = ord(char) + return ( + code in _C0 + or code == _DEL + or code in _C1 + or code in _NONCHARACTERS + or code in _SURROGATES + or code in _LINE_SEPARATORS + ) + + +def is_control_char(char: str) -> bool: + """True for a character rejected in every mode. See `_is_forbidden`.""" + return _is_forbidden(char) + + +def is_allowed_char(char: str, unicode_mode: bool | None = None) -> bool: + """Return True if `char` may appear verbatim in a step name under the active rule.""" + if unicode_mode is None: + unicode_mode = unicode_names_enabled() + if _is_forbidden(char): + return False + return unicode_mode or char in _ASCII_ALLOWED + + +# Grapheme clustering. The authority is Elixir's `String.length/1`, because +# that is what Ecto calls. So the target is not "correct per UAX #29" but +# "identical to Elixir", and the two are not the same thing. Elixir deviates +# from the spec in two places that matter here, and this implementation +# deliberately copies both: +# +# * It does not implement GB9c, the Unicode 15.1 Indic conjunct rule, so +# `क` + virama + `ष` is two graphemes to Elixir and one to the spec. +# * It ends an emoji ZWJ run *at* the joiner unless another pictograph +# follows, so `©` is two graphemes to Elixir and one +# to the spec (which would attach the mark under GB9). +# +# Hand-written rather than the `regex` module's `\X`, which is spec-correct and +# therefore disagrees with OTP in both directions: it undercounts on the two +# deviations above, which ships a name over Ecto's cap, and it overcounts on +# U+11A3A, which truncates a name Lightning would have accepted. `regex` was +# also never a declared dependency -- it arrived transitively through nltk. +# +# Re-derive with `python3 edges.py && elixir probe.exs && python3 check.py`. + +_ZWJ = "\u200d" +_CR = "\r" +_LF = "\n" + +_REGIONAL_INDICATOR = range(0x1F1E6, 0x1F200) + +#: GCB=Extend characters that are not Mn/Me by general category. +_OTHER_GRAPHEME_EXTEND = frozenset( + { + 0x09BE, 0x09D7, 0x0B3E, 0x0B57, 0x0BBE, 0x0BD7, 0x0CC2, 0x0CD5, 0x0CD6, + 0x0D3E, 0x0D57, 0x0DCF, 0x0DDF, 0x1B35, 0x200C, 0x302E, 0x302F, 0xFF9E, + 0xFF9F, 0x1133E, 0x11357, 0x114B0, 0x115AF, 0x11930, 0x1D165, 0x1D16E, + 0x1D16F, 0x1D170, 0x1D171, 0x1D172, + }, +) + +#: Tag characters (GCB=Extend despite being format characters). These are what +#: make the Scotland/Wales/England flag sequences a single grapheme. +_TAGS = range(0xE0020, 0xE0080) + +#: Emoji skin-tone modifiers. General category Sk, but GCB=Extend. +_SKIN_TONES = range(0x1F3FB, 0x1F400) + +#: GCB=Prepend. +_PREPEND = frozenset( + { + 0x0600, 0x0601, 0x0602, 0x0603, 0x0604, 0x0605, 0x06DD, 0x070F, 0x0890, + 0x0891, 0x08E2, 0x0D4E, 0x110BD, 0x110CD, 0x111C2, 0x111C3, 0x1193F, + 0x11941, 0x11A3A, 0x11A84, 0x11A85, 0x11A86, 0x11A87, 0x11A88, 0x11A89, + 0x11D46, 0x11F02, + }, +) + +#: Mc characters that are NOT GCB=SpacingMark. GraphemeBreakProperty.txt lists +#: them nowhere else, so they fall through to GCB=Other -- not Extend. +_NOT_SPACING_MARK = frozenset( + { + 0x102B, 0x102C, 0x1038, 0x1062, 0x1063, 0x1064, 0x1067, 0x1068, 0x1069, + 0x106A, 0x106B, 0x106C, 0x106D, 0x1083, 0x1087, 0x1088, 0x1089, 0x108A, + 0x108B, 0x108C, 0x108F, 0x109A, 0x109B, 0x109C, 0x1A61, 0x1A63, 0x1A64, + 0xAA7B, 0xAA7D, 0x11720, 0x11721, + }, +) + +#: Lo characters that ARE GCB=SpacingMark. +_EXTRA_SPACING_MARK = frozenset({0x0E33, 0x0EB3}) + +#: Hangul jamo, for GB6/GB7/GB8. +_HANGUL_L = (range(0x1100, 0x1160), range(0xA960, 0xA97D)) +_HANGUL_V = (range(0x1160, 0x11A8), range(0xD7B0, 0xD7C7)) +_HANGUL_T = (range(0x11A8, 0x1200), range(0xD7CB, 0xD7FC)) +_HANGUL_SYLLABLES = range(0xAC00, 0xD7A4) + +#: Extended_Pictographic, for GB11. Generated from Elixir, not hand-written: +#: ExtPict is not a break class, so a codepoint-bucket sweep cannot check it +#: and an over-broad range here is invisible to that test. The earlier +#: hand-written version collapsed sparse sets into solid blocks and claimed +#: hundreds of codepoints too many -- U+2713 CHECK MARK among them, which made +#: `✓` one grapheme here and two in Elixir. +#: Regenerate with tools/unicode_parity/probe.exs (see extpict). +_EXT_PICT_RANGES = ( + (0x00A9, 0x00A9), (0x00AE, 0x00AE), (0x203C, 0x203C), + (0x2049, 0x2049), (0x2122, 0x2122), (0x2139, 0x2139), + (0x2194, 0x2199), (0x21A9, 0x21AA), (0x231A, 0x231B), + (0x2328, 0x2328), (0x2388, 0x2388), (0x23CF, 0x23CF), + (0x23E9, 0x23F3), (0x23F8, 0x23FA), (0x24C2, 0x24C2), + (0x25AA, 0x25AB), (0x25B6, 0x25B6), (0x25C0, 0x25C0), + (0x25FB, 0x25FE), (0x2600, 0x2605), (0x2607, 0x2612), + (0x2614, 0x2685), (0x2690, 0x2705), (0x2708, 0x2712), + (0x2714, 0x2714), (0x2716, 0x2716), (0x271D, 0x271D), + (0x2721, 0x2721), (0x2728, 0x2728), (0x2733, 0x2734), + (0x2744, 0x2744), (0x2747, 0x2747), (0x274C, 0x274C), + (0x274E, 0x274E), (0x2753, 0x2755), (0x2757, 0x2757), + (0x2763, 0x2767), (0x2795, 0x2797), (0x27A1, 0x27A1), + (0x27B0, 0x27B0), (0x27BF, 0x27BF), (0x2934, 0x2935), + (0x2B05, 0x2B07), (0x2B1B, 0x2B1C), (0x2B50, 0x2B50), + (0x2B55, 0x2B55), (0x3030, 0x3030), (0x303D, 0x303D), + (0x3297, 0x3297), (0x3299, 0x3299), (0x1F000, 0x1F0FF), + (0x1F10D, 0x1F10F), (0x1F12F, 0x1F12F), (0x1F16C, 0x1F171), + (0x1F17E, 0x1F17F), (0x1F18E, 0x1F18E), (0x1F191, 0x1F19A), + (0x1F1AD, 0x1F1E5), (0x1F201, 0x1F20F), (0x1F21A, 0x1F21A), + (0x1F22F, 0x1F22F), (0x1F232, 0x1F23A), (0x1F23C, 0x1F23F), + (0x1F249, 0x1F3FA), (0x1F400, 0x1F53D), (0x1F546, 0x1F64F), + (0x1F680, 0x1F6FF), (0x1F774, 0x1F77F), (0x1F7D5, 0x1F7FF), + (0x1F80C, 0x1F80F), (0x1F848, 0x1F84F), (0x1F85A, 0x1F85F), + (0x1F888, 0x1F88F), (0x1F8AE, 0x1F8FF), (0x1F90C, 0x1F93A), + (0x1F93C, 0x1F945), (0x1F947, 0x1FAFF), (0x1FC00, 0x1FFFD), +) + + +#: Codepoints assigned after the Unicode version Python's `unicodedata` ships +#: (3.11 carries Unicode 14.0; Elixir 1.18.3 is on a later one). Without these +#: they look unassigned here, fall through to GCB=Other, and the count drifts +#: from Elixir on any name using a script added since -- Kawi, Nag Mundari, the +#: Egyptian hieroglyph controls. +#: Regenerate with tools/unicode_parity/probe.exs (see classmap). +_LAG_EXTEND = ( + (0x0CF3, 0x0CF3), (0x0ECE, 0x0ECE), (0x10EFD, 0x10EFF), + (0x11241, 0x11241), (0x11F00, 0x11F01), (0x11F03, 0x11F03), + (0x11F34, 0x11F3A), (0x11F3E, 0x11F42), (0x13440, 0x13440), + (0x13447, 0x13455), (0x1E08F, 0x1E08F), (0x1E4EC, 0x1E4EF), +) + +_LAG_CONTROL = ( + (0x2065, 0x2065), (0xFFF0, 0xFFF8), (0x13439, 0x1343F), + (0xE0000, 0xE0000), (0xE0002, 0xE001F), (0xE0080, 0xE00FF), + (0xE01F0, 0xE0FFF), +) + +# Boundary classes, named so the rule table below reads like UAX #29. +_OTHER, _CONTROL, _EXTEND, _SPACING, _PREP = 0, 1, 2, 3, 4 +_L, _V, _T, _LV, _LVT, _RI, _JOIN = 5, 6, 7, 8, 9, 10, 11 + + +def _in_ranges(code: int, ranges: tuple) -> bool: + return any(low <= code <= high for low, high in ranges) + + +def _is_ext_pict(code: int) -> bool: + """True for Extended_Pictographic, which GB11 needs on both sides of a ZWJ.""" + return _in_ranges(code, _EXT_PICT_RANGES) + + +def _break_class(char: str) -> int: # noqa: PLR0911, PLR0912 - one branch per UAX #29 class + code = ord(char) + if char == _ZWJ: + return _JOIN + if char in (_CR, _LF): + return _CONTROL + if code in _PREPEND: + return _PREP + if code in _TAGS or code in _OTHER_GRAPHEME_EXTEND or code in _SKIN_TONES: + return _EXTEND + if _in_ranges(code, _LAG_EXTEND): + return _EXTEND + if _in_ranges(code, _LAG_CONTROL): + return _CONTROL + if code in _REGIONAL_INDICATOR: + return _RI + + category = unicodedata.category(char) + if category in ("Mn", "Me"): + return _EXTEND + if category == "Mc": + return _OTHER if code in _NOT_SPACING_MARK else _SPACING + if code in _EXTRA_SPACING_MARK: + return _SPACING + if category in ("Cc", "Cf", "Zl", "Zp", "Cs"): + return _CONTROL + + if code in _HANGUL_SYLLABLES: + return _LV if (code - 0xAC00) % 28 == 0 else _LVT + if _in_ranges(code, tuple((r.start, r.stop - 1) for r in _HANGUL_L)): + return _L + if _in_ranges(code, tuple((r.start, r.stop - 1) for r in _HANGUL_V)): + return _V + if _in_ranges(code, tuple((r.start, r.stop - 1) for r in _HANGUL_T)): + return _T + + return _OTHER + + +#: What an emoji run's lookback may cross on its way back to the pictograph. +#: UAX #29 GB11 says Extend* only; Elixir also crosses SpacingMark, verified +#: against 1.18.3 over every intervening class (Other, ZWJ, Prepend, Control +#: and a non-SpacingMark Mc all stop it). +_RUN_CONTINUES = (_EXTEND, _SPACING) + + +def _ext_pict_run_before(codes: list[int], classes: list[int], zwj_index: int) -> bool: + """True if the ZWJ at `zwj_index` closes an `ExtPict (Extend | SpacingMark)*` run.""" + index = zwj_index - 1 + while index >= 0 and classes[index] in _RUN_CONTINUES: + index -= 1 + return index >= 0 and classes[index] == _OTHER and _is_ext_pict(codes[index]) + + +def _fallback_clusters(text: str) -> list[str]: # noqa: PLR0912 - one branch per boundary rule + """Split into grapheme clusters the way Elixir does, not the way UAX #29 + says. GB1-GB13, with the two deviations described in the comment above.""" + if not text: + return [] + + classes = [_break_class(char) for char in text] + codes = [ord(char) for char in text] + clusters = [] + start = 0 + ri_run = 0 + + for index in range(1, len(text)): + before, after = classes[index - 1], classes[index] + + if before == _RI: + ri_run += 1 + else: + ri_run = 0 + + if text[index - 1] == _CR and text[index] == _LF: + brk = False # GB3 + elif _CONTROL in (before, after): + brk = True # GB4, GB5 + elif before == _JOIN and _ext_pict_run_before(codes, classes, index - 1): + # GB11 as Elixir implements it, not as the spec says. See above. + brk = not _is_ext_pict(codes[index]) + elif before == _L and after in (_L, _V, _LV, _LVT): + brk = False # GB6 + elif before in (_LV, _V) and after in (_V, _T): + brk = False # GB7 + elif before in (_LVT, _T) and after == _T: + brk = False # GB8 + elif after in (_EXTEND, _JOIN): + brk = False # GB9 + elif after == _SPACING: + brk = False # GB9a + elif before == _PREP: + brk = False # GB9b + elif before == _RI and after == _RI and ri_run % 2 == 1: + brk = False # GB12, GB13 + else: + brk = True # GB999 + + if brk: + clusters.append(text[start:index]) + start = index + + clusters.append(text[start:]) + return clusters + + +def grapheme_clusters(text: str) -> list[str]: + """Split `text` into user-perceived characters, the way Elixir would.""" + if not text: + return [] + return _fallback_clusters(text) + + +def grapheme_length(text: str) -> int: + """Count user-perceived characters, the way Ecto's `validate_length` does.""" + return len(grapheme_clusters(text)) + + +def truncate_graphemes(text: str, limit: int) -> str: + """Cut `text` to `limit` graphemes without splitting one in half.""" + clusters = grapheme_clusters(text) + if len(clusters) <= limit: + return text + return "".join(clusters[:limit]) + + +# Normalisation. + + +def normalize_nfc(text: str) -> str: + """NFC, straight out of the standard library. + + This was hand-written until recently, to reproduce a composition bug in + OTP 27's normaliser. Lightning ran on OTP 27, Lightning is what stores the + name, and step lookup matches names as text, so a name Apollo normalised + differently was a name Apollo could not find. Lightning#5109 moves + Lightning to OTP 28, which fixes that bug, and the standard library is now + the closer of the two: measured over the 72,269-row corpus restricted to + inputs that could be a step name, Python disagrees with OTP 28 on 16,622 + rows and the hand-written composer on 16,898. + + The residual gap both share is one Hangul shape -- a bare jamo next to a + complete syllable -- which only a half-finished IME produces. Real Korean + words normalise identically under OTP 28, Python and ICU. + """ + return unicodedata.normalize("NFC", text) + + +def sanitize_name(name: str, unicode_mode: bool | None = None) -> str: + """Return `name` with every character the active rule forbids removed. + + Under the ASCII rule, letters are folded to their nearest ASCII form first + (``Café`` -> ``Cafe``) so accented names degrade into something readable + rather than losing whole words, and anything still not ASCII is dropped. + Under the permissive rule nothing is folded and nothing is dropped except + the rejected set -- the name is kept exactly as typed. + + In both modes: forbidden whitespace becomes a plain space, the result is + trimmed with exactly the set Elixir's `String.trim/1` strips, then + NFC-normalised, then capped at MAX_NAME_LENGTH graphemes without ever + cutting a grapheme in half. The result is a fixed point. + """ + if not name or not isinstance(name, str): + return name + if unicode_mode is None: + unicode_mode = unicode_names_enabled() + + text = normalize_nfc(name) + + # Forbidden-but-whitespace characters become a plain space here, before the + # ASCII fold rather than after it. A tab, a newline or a U+2028 is rejected + # either way, but the useful reading of it in a name is "a space", and doing + # it up front means both modes agree -- the fold would otherwise drop the + # non-ASCII ones outright and silently join the words either side. + text = "".join(" " if _is_forbidden(c) and c.isspace() else c for c in text) + + if not unicode_mode: + # Fold diacritics onto their base letters, then drop whatever is left + # that is not ASCII. This is the long-standing behaviour, plus a table + # for the handful of letters NFKD cannot decompose at all. NFKD also + # turns the exotic spaces (NBSP, ideographic space) into plain ones. + text = text.translate(_ASCII_TRANSLITERATIONS) + text = unicodedata.normalize("NFKD", text).encode("ascii", "ignore").decode("ascii") + + kept = [] + for char in text: + if _is_forbidden(char): + continue + if unicode_mode or char in _ASCII_ALLOWED: + kept.append(char) + + # Trim before normalising, not after. Trimming can uncover a combining mark + # that only composes once the character in front of it is gone, and + # truncating can uncover the same boundary again, so repeat until it + # settles. is_valid_name asks whether a name equals this function's output, + # so that output has to be a fixed point or a sanitised name reads as + # invalid. + text = "".join(kept) + for _ in range(_SANITIZE_PASSES): + settled = text + text = normalize_nfc(text.strip(_TRIM_CHARS)) + text = truncate_graphemes(text, MAX_NAME_LENGTH).strip(_TRIM_CHARS) + if text == settled: + return text + + raise RuntimeError( + f"sanitize_name did not settle in {_SANITIZE_PASSES} passes. Two are " + "enough for every input tested, so reaching this means an assumption " + "in normalize_nfc or truncate_graphemes has moved. Returning here " + "would hand back a name that is_valid_name then calls invalid." + ) + + +def is_valid_name(name: str, unicode_mode: bool | None = None) -> bool: + """Return True if `name` already satisfies the active rule (sanitising is a no-op).""" + if not isinstance(name, str): + return False + return sanitize_name(name, unicode_mode) == name + + +def first_invalid_char(name: str, unicode_mode: bool | None = None) -> str | None: + """Return the first character of `name` the active rule forbids, or None.""" + if unicode_mode is None: + unicode_mode = unicode_names_enabled() + for char in name: + if not is_allowed_char(char, unicode_mode): + return char + return None + + +def describe_rule(unicode_mode: bool | None = None) -> str: + """One sentence stating the active rule, for the workflow-generation prompt.""" + if unicode_mode is None: + unicode_mode = unicode_names_enabled() + if unicode_mode: + return ( + "Job names may contain anything except control characters. Letters and marks from any " + "script, punctuation, symbols and emoji are all fine, so `Vérifier l'état`, `患者確認`, " + "`Проверка данных` and `Import A/B` are all valid names. Write the name the user asked " + "for, as they wrote it — do not strip accents or transliterate." + ) + return ( + "Job names may use only unaccented English letters, digits, spaces, hyphens and underscores. " + "Write accented or non-Latin names in that form instead (`Vérifier l'état` becomes " + "`Verifier letat`)." + ) + + +def describe_rule_for_prompt(unicode_mode: bool | None = None) -> str: + """The full job-naming bullet used in the workflow-generation prompt.""" + return ( + f"{describe_rule(unicode_mode)} Names must be at most {MAX_NAME_LENGTH} characters " + "and must be unique within a workflow." + ) + + +def describe_rule_for_judge(unicode_mode: bool | None = None) -> str: + """The naming rule as a grading instruction, for the acceptance-test judges. + + `judges.load_judge` substitutes this into each rubric. + """ + if unicode_mode is None: + unicode_mode = unicode_names_enabled() + if unicode_mode: + common = ( + "Job names, job keys, trigger keys and edge `source_*`/`target_*` references must " + "contain no control characters. Nothing else about their characters is a defect: " + "accented Latin (`Vérifier l'état`), non-Latin (`患者確認`, `Проверка данных`), " + "punctuation, symbols and emoji are all valid. Do not flag a name for being " + "non-English, accented, or containing punctuation." + ) + else: + common = ( + "Job names, job keys, trigger keys and edge `source_*`/`target_*` references must use " + "only unaccented English letters, digits, spaces, hyphens and underscores. Flag " + "anything else: an accented or non-Latin name that reached the output means the " + "service failed to fold it." + ) + return ( + f"{common} Job names must be unique within a workflow and at most " + f"{MAX_NAME_LENGTH} characters." + ) + + +def normalize_for_lookup(name: str) -> str: + """Fold a name into the key used to match it against a job key or job name. + + Case-folded, NFC-normalised, and every character that is not a letter, mark + or digit replaced with a hyphen. Unicode-aware in both modes, so a + non-Latin name folds to itself rather than to the empty string. + + Case folding rather than lowercasing, so that the pairs `.lower()` leaves + distinct still match: Greek final sigma against medial sigma, and German + ss against sz. + + Callers must treat an empty result as "no fuzzy match available" rather + than as a key -- see ``yaml_utils.find_job_in_yaml``. + """ + if not isinstance(name, str): + return "" + text = normalize_nfc(name).casefold() + folded = "".join( + char if unicodedata.category(char)[0] in _LETTER_MARK_DIGIT else "-" for char in text + ) + return folded.strip("-") diff --git a/services/testing/judges.py b/services/testing/judges.py index 8d416a9f..8005e7f4 100644 --- a/services/testing/judges.py +++ b/services/testing/judges.py @@ -9,17 +9,48 @@ # rules - bullet rules that apply to every evaluation under this judge +The token `{name_rule}` in either section is replaced at load time with the +active step-name rule, so the judges never restate it as static prose. + To add a new judge: drop a new markdown file in `services/testing/judges/` and reference its filename (without `.md`) in a spec's `judges:` frontmatter field. Default judge is `general`. """ +import re from dataclasses import dataclass from pathlib import Path +from name_rules import describe_rule_for_judge _JUDGES_DIR = Path(__file__).parent / "judges" +#: A judge that restated the rule as static prose would go stale the moment +#: APOLLO_UNICODE_STEP_NAMES moved, and would then either pass names the +#: sanitizer mangles or fail names it correctly leaves alone. +_NAME_RULE_TOKEN = "{name_rule}" + +#: A bare `{lower_snake_case}` run. Prose and code samples in these files use +#: braces freely (`create({ name: $.x })`, `() => {}`), but never in this shape. +_PLACEHOLDER = re.compile(r"\{[a-z_][a-z0-9_]*\}") + + +def _reject_unsubstituted_placeholders(name: str, path: Path, text: str) -> None: + """Raise if any placeholder survived substitution. + + Substitution is `str.replace`, which is a silent no-op when the token is + misspelled. A judge that meant to state the active naming rule and instead + stated nothing would grade every workflow name as acceptable, and nothing + would say so. Not every judge needs the rule — the code-quality one grades + job bodies — so a missing token is fine; a *mangled* one is not. + """ + leftover = sorted(set(_PLACEHOLDER.findall(text))) + if leftover: + raise ValueError( + f"Judge '{name}' ({path}) has unsubstituted placeholders: {', '.join(leftover)}. " + f"The only one this loader fills is {_NAME_RULE_TOKEN}.", + ) + @dataclass class JudgeConfig: @@ -37,9 +68,10 @@ def load_judge(name: str) -> JudgeConfig: if not path.exists(): available = sorted(p.stem for p in _JUDGES_DIR.glob("*.md")) raise FileNotFoundError( - f"Judge '{name}' not found at {path}. Available: {available}" + f"Judge '{name}' not found at {path}. Available: {available}", ) - text = path.read_text() + text = path.read_text().replace(_NAME_RULE_TOKEN, describe_rule_for_judge()) + _reject_unsubstituted_placeholders(name, path, text) return JudgeConfig( name=name, role=_extract_section(text, "role").strip(), diff --git a/services/testing/judges/general.md b/services/testing/judges/general.md index c9fa7715..3fafafd6 100644 --- a/services/testing/judges/general.md +++ b/services/testing/judges/general.md @@ -8,7 +8,7 @@ You will be given (a) optional universal rules that apply to every response, (b) - Every job, trigger, and edge in a returned workflow YAML has a non-empty `id` field. - Every job in a returned workflow YAML has a `body` that is either real adaptor code or the canonical empty-job placeholder `// Add operations here`. Reject other placeholder-style markers such as `// PLACEHOLDER`, numbered placeholders, `TODO`, `FIXME`, or `` — these are leftover generation artifacts. -- Job names and edge source/target/key references in a returned workflow YAML use only letters, numbers, spaces, hyphens, and underscores. +- {name_rule} - When the user is editing an existing workflow, every job and edge from the existing YAML is present and unchanged in the response unless the user asked to remove or modify it. Additions are fine. - Any returned YAML parses as valid YAML. - Never claim an adaptor function or signature doesn't exist or is wrong unless adaptor documentation provided in this evaluation contradicts it — you do not have reliable knowledge of adaptor APIs. diff --git a/services/testing/judges/openfn_workflow_expert.md b/services/testing/judges/openfn_workflow_expert.md index fa5cbfe3..c4995aab 100644 --- a/services/testing/judges/openfn_workflow_expert.md +++ b/services/testing/judges/openfn_workflow_expert.md @@ -17,7 +17,7 @@ These mirror the workflow-generation contract. Reject the YAML if any are violat - Output parses as valid YAML. - Every job, trigger, and edge in the returned workflow YAML has a non-empty `id` field. (The workflow_chat service auto-generates IDs for newly added items during post-processing, so the YAML you grade should already have them — flag any item that is still missing one.) - Every job has a `body` that is either real adaptor code or the canonical empty-job placeholder `// Add operations here`. Reject other placeholder markers such as `// PLACEHOLDER`, numbered placeholders, `TODO`, `FIXME`, or `` — these are leftover generation artifacts. -- Job names and edge `source_*` / `target_*` / key references contain only letters, numbers, spaces, hyphens, and underscores. Job names must be unique within a workflow and under 100 characters. +- {name_rule} - When the user is editing an existing workflow, every job and edge from the existing YAML is present and unchanged in the response unless the user asked to remove or modify it. Additions are fine. ## Triggers diff --git a/services/testing/yaml_assertions.py b/services/testing/yaml_assertions.py index c2136e34..37cccfe3 100644 --- a/services/testing/yaml_assertions.py +++ b/services/testing/yaml_assertions.py @@ -4,6 +4,14 @@ import re import yaml +from name_rules import ( + MAX_EDGE_KEY_LENGTH, + describe_rule, + grapheme_length, + is_control_char, + is_valid_name, + truncate_graphemes, +) def path_matches(path, allowed_paths: list[str]) -> bool: @@ -40,7 +48,7 @@ def compare(o, n, path): compare(oi, ni, path + [str(i)]) elif o != n: diff = "\n".join( - difflib.unified_diff([str(o)], [str(n)], fromfile="original", tofile="response", lineterm="") + difflib.unified_diff([str(o)], [str(n)], fromfile="original", tofile="response", lineterm=""), ) raise AssertionError(f"Value mismatch at {'.'.join(path)}:\n{diff}") @@ -49,12 +57,12 @@ def compare(o, n, path): except AssertionError as e: diff = "\n".join( difflib.unified_diff( - yaml.dump(orig, sort_keys=True).splitlines(), - yaml.dump(new, sort_keys=True).splitlines(), + yaml.dump(orig, sort_keys=True, allow_unicode=True).splitlines(), + yaml.dump(new, sort_keys=True, allow_unicode=True).splitlines(), fromfile="original", tofile="response", lineterm="", - ) + ), ) raise AssertionError(f"{context}\n{e}\nFull YAML diff:\n{diff}") @@ -98,27 +106,103 @@ def assert_yaml_jobs_have_body(yaml_str_or_dict, context: str = "") -> None: assert job_data["body"] not in (None, "", []), f"{context}: Job '{job_key}' has empty 'body' field." -_SPECIAL_CHAR = re.compile(r"[^a-zA-Z0-9\s\-_]") +def assert_no_special_chars(yaml_str_or_dict, context: str = "") -> None: + """Assert every name in the workflow obeys the active step-name rule. + + Covers job keys, job names, trigger keys and edge endpoint references, and + uses `is_valid_name`, so it checks the length cap as well as the character + set. Checking only job names with a character-set regex is how a name that + was pushed over 100 characters by a uniquifying suffix, and a trigger key + that was never sanitized at all, both went unnoticed. + Also checks referential integrity: every edge endpoint must name something + that exists. A character check alone passes a perfectly well-formed name + that happens to point at no step, which is what a broken key mapping or a + stray sentinel produces. -def assert_no_special_chars(yaml_str_or_dict, context: str = "") -> None: - """Assert job names and edge source/target/keys use only [A-Za-z0-9 _-].""" + The rule is whichever one `name_rules` has active, so this assertion tracks + the sanitizer instead of restating it. + """ data = _as_dict(yaml_str_or_dict) def check(value, descriptor): - match = _SPECIAL_CHAR.search(value) - assert not match, f"{context}: {descriptor} '{value}' contains special character '{match.group(0)}'" + assert is_valid_name(value), ( + f"{context}: {descriptor} '{value}' does not obey the step-name rule. {describe_rule()}" + ) - for job_key, job_data in data.get("jobs", {}).items(): - if job_data.get("name"): + # `jobs:` with nothing under it parses as None, which is valid YAML. + jobs = data.get("jobs") or {} + triggers = data.get("triggers") or {} + edges = data.get("edges") or {} + + for job_key, job_data in jobs.items(): + check(str(job_key), f"Job key '{job_key}'") + if (job_data or {}).get("name"): check(str(job_data["name"]), f"Job '{job_key}' name") - for edge_key, edge_data in data.get("edges", {}).items(): - for field in ("source_job", "target_job"): - if edge_data.get(field): - check(str(edge_data[field]), f"Edge '{edge_key}' {field}") + for trigger_key in triggers: + check(str(trigger_key), f"Trigger key '{trigger_key}'") + + for edge_key, raw_edge in edges.items(): + edge = raw_edge or {} + for field, targets, what in ( + ("source_job", jobs, "job"), + ("target_job", jobs, "job"), + ("source_trigger", triggers, "trigger"), + ("target_trigger", triggers, "trigger"), + ): + if edge.get(field): + value = str(edge[field]) + check(value, f"Edge '{edge_key}' {field}") + assert value in targets, ( + f"{context}: Edge '{edge_key}' {field} '{value}' is not a {what} " + f"in this workflow (have: {sorted(targets)})." + ) + + _check_edge_key(edge_key, edge, context) + + +def _check_edge_key(edge_key: str, edge_data: dict, context: str) -> None: + """Assert an edge's key is the label its own endpoints imply. + + Deliberately does not split the key on "->", which is a legal run of + characters inside a step name under the permissive rule. Mirrors + `_edge_label` in workflow_chat: endpoints known means the key is derived. + """ + edge_key = str(edge_key) + + assert grapheme_length(edge_key) <= MAX_EDGE_KEY_LENGTH, ( + f"{context}: Edge key '{edge_key}' is {grapheme_length(edge_key)} graphemes, " + f"over the {MAX_EDGE_KEY_LENGTH} limit." + ) + + source = edge_data.get("source_job") or edge_data.get("source_trigger") + target = edge_data.get("target_job") or edge_data.get("target_trigger") + + if not (source and target): + # Nothing to derive the label from; just make sure it is storable. + assert not any(is_control_char(ch) for ch in edge_key), ( + f"{context}: Edge key '{edge_key}' contains a control character." + ) + return + + label = f"{source}->{target}" + + # The sanitizer suffixes duplicate labels and makes room inside the cap, so + # the key is a grapheme prefix of the label, optionally with a `-N` tail. + candidates = [edge_key] + tail = _COLLISION_SUFFIX.search(edge_key) + if tail: + candidates.append(edge_key[: tail.start()]) + + assert any( + candidate == truncate_graphemes(label, grapheme_length(candidate)) + for candidate in candidates + ), ( + f"{context}: Edge key '{edge_key}' does not match its own endpoints " + f"(expected '{truncate_graphemes(label, MAX_EDGE_KEY_LENGTH)}', " + f"optionally trimmed for a -N suffix)." + ) + - if "->" in edge_key: - source_part, target_part = edge_key.split("->", 1) - check(source_part, f"Edge key '{edge_key}' source part") - check(target_part, f"Edge key '{edge_key}' target part") +_COLLISION_SUFFIX = re.compile(r"-\d+$") diff --git a/services/workflow_chat/gen_project_prompt.py b/services/workflow_chat/gen_project_prompt.py index 35eeaf29..943f3b21 100644 --- a/services/workflow_chat/gen_project_prompt.py +++ b/services/workflow_chat/gen_project_prompt.py @@ -1,6 +1,9 @@ import os -from .config_loader import ConfigLoader + +from name_rules import describe_rule_for_prompt + from .available_adaptors import get_adaptors_string +from .config_loader import ConfigLoader base_dir = os.path.dirname(os.path.abspath(__file__)) config_path = os.path.join(base_dir, "gen_project_config.yaml") @@ -10,18 +13,45 @@ config = config_loader.config +NAME_RULE_TOKEN = "{name_rule}" + + +def _general_knowledge(): + """Render the general-knowledge prompt, with the active step-name rule in it. + + `str.format` ignores a keyword the template does not use, so dropping the + token from the yaml would silently ship a prompt that states no naming rule + at all while the sanitizer carried on enforcing one. Check for it first. + """ + rule = describe_rule_for_prompt() + rendered = config_loader.get_prompt("general_knowledge").format( + adaptors=get_adaptors_string(), + name_rule=rule, + ) + + # Check the *rendered* text, not the template. A doubled `{{name_rule}}` is + # how `.format` escapes a literal brace: it contains the token as a + # substring, so a template-side check waves it through, and what reaches the + # model is the four words "{name_rule}" rather than any rule at all. + if NAME_RULE_TOKEN in rendered or rule not in rendered: + raise ValueError( + f"The general_knowledge prompt did not render the step-name rule. It must contain " + f"exactly {NAME_RULE_TOKEN}, unescaped and unduplicated — the rule stated to the " + f"model and the rule the sanitizer enforces have to come from the same place.", + ) + return rendered + + def build_system_message(mode_config, existing_yaml=None): """Build system message with mode-specific configuration.""" system_message = config_loader.get_prompt("main_system_prompt").format( mode_specific_intro=config_loader.get_prompt(mode_config["intro"]), yaml_structure=config_loader.get_prompt(mode_config["yaml_structure"]), - general_knowledge=config_loader.get_prompt("general_knowledge").format( - adaptors=get_adaptors_string() - ), + general_knowledge=_general_knowledge(), output_format=config_loader.get_prompt(mode_config["output_format"]), mode_specific_answering_instructions=config_loader.get_prompt( - mode_config["answering_instructions"] - ) + mode_config["answering_instructions"], + ), ) if existing_yaml: @@ -53,7 +83,7 @@ def build_prompt(content, existing_yaml=None, errors=None, history=None, read_on "yaml_structure": "yaml_structure_without_ids", "output_format": "unstructured_output_format", "answering_instructions": "readonly_mode_answering_instructions", - "yaml_prefix": "\nFor context, the user is viewing this read-only YAML:\n" + "yaml_prefix": "\nFor context, the user is viewing this read-only YAML:\n", } user_content = content elif errors: @@ -62,7 +92,7 @@ def build_prompt(content, existing_yaml=None, errors=None, history=None, read_on "yaml_structure": "yaml_structure_with_ids", "output_format": "json_output_format", "answering_instructions": "error_mode_answering_instructions", - "yaml_prefix": "\nThis is the YAML causing the error:\n" + "yaml_prefix": "\nThis is the YAML causing the error:\n", } user_content = f"{content}\nThis is the error message:\n{errors}" if content else f"\nThis is the error message:\n{errors}" else: @@ -71,7 +101,7 @@ def build_prompt(content, existing_yaml=None, errors=None, history=None, read_on "yaml_structure": "yaml_structure_with_ids", "output_format": "json_output_format", "answering_instructions": "normal_mode_answering_instructions", - "yaml_prefix": "\nFor context, the user is currently editing this YAML:\n" + "yaml_prefix": "\nFor context, the user is currently editing this YAML:\n", } user_content = content diff --git a/services/workflow_chat/gen_project_prompts.yaml b/services/workflow_chat/gen_project_prompts.yaml index ae5b4ca7..9ca2743a 100644 --- a/services/workflow_chat/gen_project_prompts.yaml +++ b/services/workflow_chat/gen_project_prompts.yaml @@ -151,7 +151,7 @@ prompts: ## Rules for Job Identification 1. Each distinct action should become its own job - 2. Jobs should have clear, descriptive names. Job names cannot have special characters and must be under 100 characters. All job names must be unique within a workflow. + 2. Jobs should have clear, descriptive names. {name_rule} 3. Jobs should be connected in a logical sequence 4. Choose the most specific adaptor available for each operation 5. When in doubt about an adaptor, use `@openfn/language-common@latest` for data transformation and `@openfn/language-http@latest` for platform integrations. diff --git a/services/workflow_chat/tests/test_pass_fail.py b/services/workflow_chat/tests/test_pass_fail.py index 832d8621..22a34929 100644 --- a/services/workflow_chat/tests/test_pass_fail.py +++ b/services/workflow_chat/tests/test_pass_fail.py @@ -218,9 +218,9 @@ def test_rename_two_jobs_commcare(): def test_special_characters(): print("==================TEST==================") - print("Description: Ask for a workflow that uses platforms with special characters in their names. " - "Verify that diacritics and punctuation removed/normalised correctly (e.g. é->e) in job names " - "in the generated YAML.") + print("Description: Ask for a workflow that uses platforms with accents and punctuation in their " + "names. Verify the job names in the generated YAML obey whichever step-name rule is active " + "(see name_rules): folded to ASCII by default, kept as typed with APOLLO_UNICODE_STEP_NAMES on.") existing_yaml = """""" history = [ {"role": "user", "content": "Create a workflow that retrieves data from mwater, google sheets, netsuite, ferntech.io and processed it and sends it to frappé"}, diff --git a/services/workflow_chat/tests/unit/client/test_sanitize.py b/services/workflow_chat/tests/unit/client/test_sanitize.py index 3bfd30a9..635c0ba4 100644 --- a/services/workflow_chat/tests/unit/client/test_sanitize.py +++ b/services/workflow_chat/tests/unit/client/test_sanitize.py @@ -1,12 +1,57 @@ +"""Job-name sanitizing, in both step-name modes. + +The rule is switchable at runtime (see `name_rules`), so every test here pins +the mode it is testing with the APOLLO_UNICODE_STEP_NAMES environment variable +rather than relying on whatever the environment happens to be set to. +""" + +import unicodedata + +import pytest +import yaml +from name_rules import ( + MAX_EDGE_KEY_LENGTH, + MAX_NAME_LENGTH, + UNICODE_FLAG_ENV, + grapheme_length, +) +from testing.yaml_assertions import assert_no_special_chars from workflow_chat.workflow_chat import AnthropicClient -def test_sanitize_job_names_removes_diacritics(): +@pytest.fixture +def ascii_mode(monkeypatch: pytest.MonkeyPatch) -> None: + """Pin the restrictive ASCII rule (the default).""" + monkeypatch.setenv(UNICODE_FLAG_ENV, "false") + + +@pytest.fixture +def unicode_mode(monkeypatch: pytest.MonkeyPatch) -> None: + """Pin the permissive Unicode rule.""" + monkeypatch.setenv(UNICODE_FLAG_ENV, "true") + + +# --- default (ASCII) mode --------------------------------------------------- + + +@pytest.mark.usefixtures("ascii_mode") +def test_default_mode_is_ascii(monkeypatch: pytest.MonkeyPatch) -> None: + """With the flag unset at all, the ASCII rule applies.""" + monkeypatch.delenv(UNICODE_FLAG_ENV, raising=False) + yaml_data = {"jobs": {"job1": {"name": "Café München"}}} + + AnthropicClient.sanitize_job_names(yaml_data) + + assert yaml_data["jobs"]["job1"]["name"] == "Cafe Munchen" + + +@pytest.mark.usefixtures("ascii_mode") +def test_ascii_mode_removes_diacritics() -> None: yaml_data = { "jobs": { "job1": {"name": "Café München"}, "job2": {"name": "Naïve résumé"}, - } + }, } AnthropicClient.sanitize_job_names(yaml_data) @@ -15,12 +60,13 @@ def test_sanitize_job_names_removes_diacritics(): assert yaml_data["jobs"]["job2"]["name"] == "Naive resume" -def test_sanitize_job_names_removes_special_characters(): +@pytest.mark.usefixtures("ascii_mode") +def test_ascii_mode_removes_special_characters() -> None: yaml_data = { "jobs": { "job1": {"name": "Job@#$%Name!"}, "job2": {"name": "Process&Data*With+Symbols"}, - } + }, } AnthropicClient.sanitize_job_names(yaml_data) @@ -29,7 +75,8 @@ def test_sanitize_job_names_removes_special_characters(): assert yaml_data["jobs"]["job2"]["name"] == "ProcessDataWithSymbols" -def test_sanitize_job_names_preserves_allowed_characters(): +@pytest.mark.usefixtures("ascii_mode") +def test_ascii_mode_preserves_allowed_characters() -> None: yaml_data = {"jobs": {"job1": {"name": "Valid Job-Name_123"}}} AnthropicClient.sanitize_job_names(yaml_data) @@ -37,7 +84,1536 @@ def test_sanitize_job_names_preserves_allowed_characters(): assert yaml_data["jobs"]["job1"]["name"] == "Valid Job-Name_123" -def test_sanitize_job_names_handles_empty_data(): +def test_handles_empty_data() -> None: assert AnthropicClient.sanitize_job_names(None) is None assert AnthropicClient.sanitize_job_names({}) is None assert AnthropicClient.sanitize_job_names({"jobs": {}}) is None + + +@pytest.mark.parametrize("payload", [[], "not a workflow", 42, 0.5, {"jobs": "nope"}]) +def test_tolerates_a_payload_that_is_not_a_workflow(payload: object) -> None: + """One call site swallows every exception from this, so raising loses the YAML silently.""" + assert AnthropicClient.sanitize_job_names(payload) is None + + +# --- Unicode mode ----------------------------------------------------------- + + +@pytest.mark.usefixtures("unicode_mode") +def test_unicode_mode_keeps_accents_and_apostrophes() -> None: + yaml_data = { + "jobs": { + "job1": {"name": "Vérifier l'état"}, + "job2": {"name": "O'Brien's Step"}, + }, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + assert yaml_data["jobs"]["job1"]["name"] == "Vérifier l'état" + assert yaml_data["jobs"]["job2"]["name"] == "O'Brien's Step" + + +@pytest.mark.usefixtures("unicode_mode") +def test_unicode_mode_keeps_non_latin_scripts() -> None: + yaml_data = { + "jobs": { + "job1": {"name": "患者確認"}, + "job2": {"name": "Проверка данных"}, + "job3": {"name": "ß straße"}, + "job4": {"name": "रोगी की जाँच"}, + }, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + assert yaml_data["jobs"]["job1"]["name"] == "患者確認" + assert yaml_data["jobs"]["job2"]["name"] == "Проверка данных" + assert yaml_data["jobs"]["job3"]["name"] == "ß straße" + assert yaml_data["jobs"]["job4"]["name"] == "रोगी की जाँच" + + +@pytest.mark.usefixtures("unicode_mode") +def test_unicode_mode_keeps_symbols_and_punctuation() -> None: + """The permissive rule strips nothing but control characters.""" + yaml_data = { + "jobs": { + "job1": {"name": "Résumé ✅ @#$%"}, + "job2": {"name": "Import A/B"}, + "job3": {"name": "50% & rising"}, + }, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + assert yaml_data["jobs"]["job1"]["name"] == "Résumé ✅ @#$%" + assert yaml_data["jobs"]["job2"]["name"] == "Import A/B" + assert yaml_data["jobs"]["job3"]["name"] == "50% & rising" + + +@pytest.mark.usefixtures("unicode_mode") +def test_unicode_mode_keeps_an_arrow_inside_a_name() -> None: + """Nothing anywhere splits an edge key on "->" — it is a label, not identity. + + The edge label is rebuilt from `source_job`/`target_job`, so a name + containing "->" cannot dangle an edge. + """ + yaml_data = { + "jobs": {"a->b": {"name": "a->b"}, "c": {"name": "C"}}, + "edges": { + "a->b->c": {"source_job": "a->b", "target_job": "c"}, + }, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + assert yaml_data["jobs"]["a->b"]["name"] == "a->b" + assert list(yaml_data["jobs"]) == ["a->b", "c"] + edge = yaml_data["edges"]["a->b->c"] + assert edge["source_job"] == "a->b" + assert edge["target_job"] == "c" + + +# --- control characters, rejected in every mode ----------------------------- + + +@pytest.mark.parametrize("mode", ["false", "true"]) +def test_control_characters_are_rejected_in_every_mode(monkeypatch: pytest.MonkeyPatch, mode: str) -> None: + """A NUL byte crashes Lightning's Postgres insert, so it never gets through.""" + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + yaml_data = {"jobs": {"job1": {"name": "Fetch\x00Data\x1b[31m\x9b"}}} + + AnthropicClient.sanitize_job_names(yaml_data) + + name = yaml_data["jobs"]["job1"]["name"] + assert "\x00" not in name + assert "\x1b" not in name + assert "\x9b" not in name + # Only the controls go. The "[" is an ordinary character, so it survives + # under the permissive rule and is dropped by the ASCII whitelist. + assert name == ("FetchData[31m" if mode == "true" else "FetchData31m") + + +@pytest.mark.parametrize("mode", ["false", "true"]) +def test_noncharacters_are_rejected_in_every_mode(monkeypatch: pytest.MonkeyPatch, mode: str) -> None: + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + yaml_data = {"jobs": {"job1": {"name": "Fetch\ufffeData\uffff"}}} + + AnthropicClient.sanitize_job_names(yaml_data) + + assert yaml_data["jobs"]["job1"]["name"] == "FetchData" + + +@pytest.mark.parametrize("mode", ["false", "true"]) +def test_tabs_and_newlines_become_spaces(monkeypatch: pytest.MonkeyPatch, mode: str) -> None: + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + yaml_data = {"jobs": {"job1": {"name": "Fetch\tthe\ndata"}}} + + AnthropicClient.sanitize_job_names(yaml_data) + + assert yaml_data["jobs"]["job1"]["name"] == "Fetch the data" + + +@pytest.mark.parametrize("mode", ["false", "true"]) +def test_names_are_trimmed_and_capped(monkeypatch: pytest.MonkeyPatch, mode: str) -> None: + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + yaml_data = {"jobs": {"job1": {"name": " Fetch Data "}, "job2": {"name": "a" * 200}}} + + AnthropicClient.sanitize_job_names(yaml_data) + + assert yaml_data["jobs"]["job1"]["name"] == "Fetch Data" + assert len(yaml_data["jobs"]["job2"]["name"]) == MAX_NAME_LENGTH + + +# --- collisions ------------------------------------------------------------- + + +@pytest.mark.usefixtures("ascii_mode") +def test_names_that_sanitize_to_the_same_string_stay_distinct() -> None: + """`Résumé` and `Resume` both fold to `Resume` under the ASCII rule. + + The prompt requires job names to be unique within a workflow, and Lightning + enforces it with a unique index, so the second one has to be nudged. + """ + yaml_data = {"jobs": {"a": {"name": "Résumé"}, "b": {"name": "Resume"}}} + + AnthropicClient.sanitize_job_names(yaml_data) + + names = [job["name"] for job in yaml_data["jobs"].values()] + assert names == ["Resume", "Resume-2"] + assert len(set(names)) == len(names) + + +@pytest.mark.usefixtures("ascii_mode") +def test_job_keys_that_sanitize_to_the_same_string_stay_distinct() -> None: + yaml_data = { + "jobs": {"résumé": {"name": "One"}, "resume": {"name": "Two"}}, + "edges": { + "résumé->resume": {"source_job": "résumé", "target_job": "resume"}, + }, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + assert list(yaml_data["jobs"]) == ["resume", "resume-2"] + edge = yaml_data["edges"]["resume->resume-2"] + assert edge["source_job"] == "resume" + assert edge["target_job"] == "resume-2" + + +@pytest.mark.usefixtures("ascii_mode") +def test_name_that_sanitizes_away_falls_back_to_the_job_key() -> None: + """A wholly non-Latin name folds to nothing under the ASCII rule. + + An empty name fails Lightning's `validate_required`, so fall back to + something rather than emitting a workflow that cannot be saved. + """ + yaml_data = {"jobs": {"check-patient": {"name": "患者確認"}}} + + AnthropicClient.sanitize_job_names(yaml_data) + + assert yaml_data["jobs"]["check-patient"]["name"] == "check-patient" + + +@pytest.mark.usefixtures("ascii_mode") +def test_key_that_sanitizes_away_falls_back_to_a_positional_key() -> None: + yaml_data = {"jobs": {"患者確認": {"name": "Check Patient"}}} + + AnthropicClient.sanitize_job_names(yaml_data) + + assert list(yaml_data["jobs"]) == ["step-1"] + assert yaml_data["jobs"]["step-1"]["name"] == "Check Patient" + + +# --- the jobs-key / edge-reference asymmetry -------------------------------- + + +@pytest.mark.usefixtures("ascii_mode") +def test_job_keys_and_edge_references_stay_in_step() -> None: + """Edges must still point at jobs that exist after sanitizing. + + Job keys used to be left alone while edge references were sanitized, so a + workflow keyed on a non-ASCII name came out with every edge dangling. + """ + yaml_data = { + "jobs": { + "Vérifier-l-état": {"name": "Vérifier l'état"}, + "envoyer-données": {"name": "Envoyer données"}, + }, + "triggers": {"webhook": {"type": "webhook"}}, + "edges": { + "webhook->Vérifier-l-état": { + "source_trigger": "webhook", + "target_job": "Vérifier-l-état", + }, + "Vérifier-l-état->envoyer-données": { + "source_job": "Vérifier-l-état", + "target_job": "envoyer-données", + }, + }, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + job_keys = set(yaml_data["jobs"]) + assert job_keys == {"Verifier-l-etat", "envoyer-donnees"} + + for edge_key, edge in yaml_data["edges"].items(): + source, target = edge_key.split("->", 1) + assert target in job_keys, f"edge key '{edge_key}' targets a job that does not exist" + if source != "webhook": + assert source in job_keys, f"edge key '{edge_key}' sources a job that does not exist" + for field in ("source_job", "target_job"): + if field in edge: + assert edge[field] in job_keys, f"edge {field} '{edge[field]}' is not a job" + + +@pytest.mark.usefixtures("unicode_mode") +def test_unicode_mode_leaves_a_valid_workflow_untouched() -> None: + yaml_data = { + "jobs": { + "Vérifier-l-état": {"name": "Vérifier l'état"}, + "患者確認": {"name": "患者確認"}, + }, + "edges": { + "Vérifier-l-état->患者確認": { + "source_job": "Vérifier-l-état", + "target_job": "患者確認", + }, + }, + } + before = yaml_data.copy() + + AnthropicClient.sanitize_job_names(yaml_data) + + assert list(yaml_data["jobs"]) == ["Vérifier-l-état", "患者確認"] + assert yaml_data["edges"] == before["edges"] + + +@pytest.mark.usefixtures("ascii_mode") +def test_an_edge_with_no_usable_endpoints_keeps_its_key() -> None: + """Only when there is nothing to derive a label from. + + An edge that *does* have endpoints gets the derived label even if its key + has no arrow — see test_an_edge_key_with_no_arrow_still_gets_the_derived_label. + """ + yaml_data = { + "jobs": {"a": {"name": "A"}}, + "edges": {"some-edge": {"condition_type": "always"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + assert list(yaml_data["edges"]) == ["some-edge"] + + +# --- two edges between the same pair ------------------------------------------ + + +@pytest.mark.usefixtures("ascii_mode") +def test_two_edges_between_the_same_pair_both_survive() -> None: + """An on_success and an on_failure edge between two steps is an ordinary workflow. + + The label is derived from the endpoints, which is not injective, so keying + on the bare label would silently drop one of the two edges. + """ + yaml_data = { + "jobs": {"A": {"name": "A"}, "B": {"name": "B"}}, + "edges": { + "A->B": {"source_job": "A", "target_job": "B", "condition_type": "on_job_success"}, + "A->B (on failure)": { + "source_job": "A", "target_job": "B", "condition_type": "on_job_failure", + }, + }, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + conditions = sorted(edge["condition_type"] for edge in yaml_data["edges"].values()) + assert conditions == ["on_job_failure", "on_job_success"] + for edge in yaml_data["edges"].values(): + assert edge["source_job"] == "A" + assert edge["target_job"] == "B" + + +@pytest.mark.usefixtures("ascii_mode") +def test_three_edges_between_the_same_pair_all_survive() -> None: + yaml_data = { + "jobs": {"A": {"name": "A"}, "B": {"name": "B"}}, + "edges": { + f"A->B ({n})": {"source_job": "A", "target_job": "B", "n": n} + for n in range(3) + }, + } + + expected = sorted(edge["n"] for edge in yaml_data["edges"].values()) + + AnthropicClient.sanitize_job_names(yaml_data) + + assert sorted(edge["n"] for edge in yaml_data["edges"].values()) == expected + + +# --- the uniquifying suffix must live inside the cap -------------------------- + + +@pytest.mark.parametrize("mode", ["false", "true"]) +def test_uniquifying_suffix_does_not_push_a_name_over_the_cap( + monkeypatch: pytest.MonkeyPatch, mode: str, +) -> None: + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + yaml_data = { + "jobs": { + "a": {"name": "x" * MAX_NAME_LENGTH}, + "b": {"name": "x" * MAX_NAME_LENGTH}, + "c": {"name": "x" * MAX_NAME_LENGTH}, + }, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + names = [job["name"] for job in yaml_data["jobs"].values()] + assert len(set(names)) == len(names), "names collapsed onto each other" + for name in names: + assert grapheme_length(name) <= MAX_NAME_LENGTH + + +@pytest.mark.parametrize("mode", ["false", "true"]) +def test_uniquifying_long_job_keys_stays_inside_the_cap( + monkeypatch: pytest.MonkeyPatch, mode: str, +) -> None: + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + long_key = "k" * MAX_NAME_LENGTH + yaml_data = {"jobs": {long_key: {"name": "A"}, long_key + "!": {"name": "B"}}} + + AnthropicClient.sanitize_job_names(yaml_data) + + keys = list(yaml_data["jobs"]) + assert len(set(keys)) == len(keys) + for key in keys: + assert grapheme_length(key) <= MAX_NAME_LENGTH + + +# --- references that sanitize away ------------------------------------------- + + +@pytest.mark.usefixtures("ascii_mode") +def test_a_reference_that_sanitizes_away_does_not_leak_the_raw_value() -> None: + """Returning the original on an empty result put raw non-ASCII back into the YAML.""" + yaml_data = { + "jobs": {"a": {"name": "A"}}, + "edges": {"患者->a": {"source_job": "患者", "target_job": "a"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + edge = next(iter(yaml_data["edges"].values())) + assert edge["source_job"] == AnthropicClient.UNRESOLVED_REFERENCE + assert "患者" not in str(yaml_data) + + +# --- triggers ----------------------------------------------------------------- + + +@pytest.mark.usefixtures("ascii_mode") +def test_trigger_keys_and_references_are_sanitized_too() -> None: + """Triggers were read for the edge label but never sanitized or remapped.""" + yaml_data = { + "jobs": {"a": {"name": "A"}}, + "triggers": {"ウェブフック": {"type": "webhook"}}, + "edges": {"ウェブフック->a": {"source_trigger": "ウェブフック", "target_job": "a"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + trigger_key = next(iter(yaml_data["triggers"])) + assert trigger_key.isascii() + edge_key, edge = next(iter(yaml_data["edges"].items())) + assert edge["source_trigger"] == trigger_key + assert edge_key == f"{trigger_key}->a" + assert "ウェブフック" not in str(yaml_data) + + +@pytest.mark.usefixtures("unicode_mode") +def test_permissive_mode_leaves_a_non_latin_trigger_alone() -> None: + yaml_data = { + "jobs": {"a": {"name": "A"}}, + "triggers": {"ウェブフック": {"type": "webhook"}}, + "edges": {"ウェブフック->a": {"source_trigger": "ウェブフック", "target_job": "a"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + assert list(yaml_data["triggers"]) == ["ウェブフック"] + assert list(yaml_data["edges"]) == ["ウェブフック->a"] + + +# --- jobs and triggers must not share a namespace ----------------------------- + + +@pytest.mark.usefixtures("ascii_mode") +def test_a_trigger_and_a_job_with_the_same_original_key_stay_separate() -> None: + """One shared mapping let the jobs pass overwrite the trigger's entry. + + The edge's source_trigger then pointed at a job and the trigger was + orphaned, which reads as a valid workflow and is not one. + """ + yaml_data = { + "triggers": {"Café": {"type": "webhook"}}, + "jobs": {"Cafe": {"name": "One"}, "Café": {"name": "Two"}}, + "edges": {"Café->Cafe": {"source_trigger": "Café", "target_job": "Cafe"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + trigger_key = next(iter(yaml_data["triggers"])) + edge = next(iter(yaml_data["edges"].values())) + + assert edge["source_trigger"] == trigger_key, "trigger reference was bound to a job" + assert edge["source_trigger"] not in yaml_data["jobs"] or trigger_key in yaml_data["jobs"] + assert edge["target_job"] in yaml_data["jobs"] + + +@pytest.mark.usefixtures("unicode_mode") +def test_trailing_whitespace_cannot_collide_a_trigger_onto_a_job() -> None: + """Permissive mode reaches the same collision through trimming.""" + yaml_data = { + "triggers": {"hook ": {"type": "webhook"}}, + "jobs": {"hook": {"name": "A"}, "hook ": {"name": "B"}}, + "edges": {"hook ->hook": {"source_trigger": "hook ", "target_job": "hook"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + edge = next(iter(yaml_data["edges"].values())) + assert edge["source_trigger"] in yaml_data["triggers"] + assert edge["target_job"] in yaml_data["jobs"] + + +# --- the unresolved sentinel -------------------------------------------------- + + +@pytest.mark.usefixtures("ascii_mode") +def test_the_sentinel_cannot_bind_to_a_real_step_of_the_same_name() -> None: + """A user may name a step `unresolved-step`; keys are uniquified against + each other, not against the sentinel.""" + yaml_data = { + "jobs": {AnthropicClient.UNRESOLVED_REFERENCE: {"name": "Real Step"}}, + "edges": { + "患者->x": { + "source_job": "患者", + "target_job": AnthropicClient.UNRESOLVED_REFERENCE, + }, + }, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + edge = next(iter(yaml_data["edges"].values())) + assert edge["target_job"] == AnthropicClient.UNRESOLVED_REFERENCE + assert edge["source_job"] not in yaml_data["jobs"], "sentinel bound to a real step" + + +# --- the sanitiser and its assertion must agree ------------------------------- + + +@pytest.mark.usefixtures("ascii_mode") +def test_an_edge_key_with_no_arrow_still_gets_the_derived_label() -> None: + """Leaving it alone here while the test assertion demanded the label meant + the sanitiser emitted output its own assertion rejected.""" + yaml_data = { + "jobs": {"A": {"name": "A"}, "B": {"name": "B"}}, + "edges": {"e1": {"source_job": "A", "target_job": "B"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + assert list(yaml_data["edges"]) == ["A->B"] + assert_no_special_chars(yaml_data, context="derived label") + + +@pytest.mark.parametrize("mode", ["false", "true"]) +def test_sanitised_output_always_satisfies_its_own_assertion( + monkeypatch: pytest.MonkeyPatch, mode: str, +) -> None: + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + yaml_data = { + "triggers": {"webhook": {"type": "webhook"}}, + "jobs": { + "Vérifier-l-état": {"name": "Vérifier l'état"}, + "患者確認": {"name": "患者確認"}, + "x" * 120: {"name": "y" * 120}, + }, + "edges": { + "webhook->Vérifier-l-état": { + "source_trigger": "webhook", "target_job": "Vérifier-l-état", + }, + "e-no-arrow": {"source_job": "Vérifier-l-état", "target_job": "患者確認"}, + "long": {"source_job": "x" * 120, "target_job": "患者確認"}, + }, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + assert_no_special_chars(yaml_data, context=f"mode={mode}") + + +# --- edge key length ---------------------------------------------------------- + + +@pytest.mark.parametrize("mode", ["false", "true"]) +def test_edge_keys_are_length_capped(monkeypatch: pytest.MonkeyPatch, mode: str) -> None: + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + first, second = "a" * MAX_NAME_LENGTH, "b" * MAX_NAME_LENGTH + yaml_data = { + "jobs": {first: {"name": "A"}, second: {"name": "B"}}, + "edges": {"k": {"source_job": first, "target_job": second}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + for edge_key in yaml_data["edges"]: + assert grapheme_length(edge_key) <= MAX_EDGE_KEY_LENGTH + + +# --- null sections across the whole finalize pipeline ------------------------- + +NULL_SECTION_DOCUMENTS = [ + "name: w\njobs:\n a:\n id: x\n body: code()\n b:\nedges:\n", + "name: w\njobs:\nedges:\n", + "name: w\ntriggers:\n webhook:\nedges:\n e:\n", + "name: w\njobs:\n a:\nedges:\n a->a:\n", + "name: w\n", +] + + +@pytest.mark.parametrize("document", NULL_SECTION_DOCUMENTS) +@pytest.mark.parametrize("mode", ["false", "true"]) +def test_the_whole_pipeline_survives_a_null_section( + monkeypatch: pytest.MonkeyPatch, document: str, mode: str, +) -> None: + """A model output ending in a bare `edges:` used to raise inside + extract/restore, get swallowed, and reach the user as prose with no + workflow while the log claimed the YAML would not parse.""" + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + + preserved, _ = AnthropicClient.extract_and_preserve_components(yaml.safe_load(document)) + + parsed = yaml.safe_load(document) + AnthropicClient.sanitize_job_names(parsed) + client = AnthropicClient.__new__(AnthropicClient) + AnthropicClient.restore_components(client, parsed, preserved) + + assert yaml.dump(parsed, allow_unicode=True) is not None + + +# --- referential integrity at runtime ----------------------------------------- + + +@pytest.mark.usefixtures("ascii_mode") +def test_an_edge_referring_to_a_step_by_name_is_resolved_to_its_key() -> None: + """The likeliest real model mistake. It used to ship as a well-formed + dangling edge with nothing logged.""" + yaml_data = { + "jobs": {"fetch-data": {"name": "Fetch Data"}, "send": {"name": "Send"}}, + "edges": {"e": {"source_job": "Fetch Data", "target_job": "send"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + edge = next(iter(yaml_data["edges"].values())) + assert edge["source_job"] == "fetch-data" + assert_no_special_chars(yaml_data, context="by-name reference") + + +@pytest.mark.usefixtures("ascii_mode") +def test_a_genuinely_dangling_edge_is_reported(caplog: pytest.LogCaptureFixture) -> None: + yaml_data = { + "jobs": {"a": {"name": "A"}}, + "edges": {"e": {"source_job": "nowhere-at-all", "target_job": "a"}}, + } + + with caplog.at_level("WARNING"): + AnthropicClient.sanitize_job_names(yaml_data) + + assert any("match no step or trigger" in r.message for r in caplog.records) + + +@pytest.mark.usefixtures("ascii_mode") +def test_the_sentinel_is_unique_against_job_names_too() -> None: + yaml_data = { + "jobs": {"a": {"name": AnthropicClient.UNRESOLVED_REFERENCE}}, + "edges": {"e": {"source_job": "患者", "target_job": "a"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + names = {job["name"] for job in yaml_data["jobs"].values()} + edge = next(iter(yaml_data["edges"].values())) + assert edge["source_job"] not in names + + +# --- typed keys --------------------------------------------------------------- + + +@pytest.mark.usefixtures("ascii_mode") +def test_an_int_key_and_a_string_key_do_not_shadow_each_other() -> None: + """YAML gives `1:` as the int 1 and `"1":` as the string "1".""" + yaml_data = {"jobs": {1: {"name": "Int"}, "1": {"name": "Str"}}, "edges": {}} + + AnthropicClient.sanitize_job_names(yaml_data) + + assert sorted(job["name"] for job in yaml_data["jobs"].values()) == ["Int", "Str"] + + +@pytest.mark.usefixtures("ascii_mode") +def test_a_boolean_edge_key_does_not_raise() -> None: + """An unquoted `on:` in the YAML parses as True, not a string.""" + yaml_data = {"jobs": {"x": {"name": "X"}}, "edges": {True: {"condition_type": "always"}}} + + AnthropicClient.sanitize_job_names(yaml_data) + + assert list(yaml_data["edges"]) == ["True"] + + +# --- the collision suffix lives inside the cap -------------------------------- + + +@pytest.mark.parametrize("mode", ["false", "true"]) +def test_two_edges_between_two_maximal_names_stay_inside_the_key_cap( + monkeypatch: pytest.MonkeyPatch, mode: str, +) -> None: + """Capping first and appending the suffix after gave a 204-grapheme key.""" + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + first, second = "a" * MAX_NAME_LENGTH, "b" * MAX_NAME_LENGTH + yaml_data = { + "jobs": {first: {"name": "A"}, second: {"name": "B"}}, + "edges": { + "e1": {"source_job": first, "target_job": second, "n": 1}, + "e2": {"source_job": first, "target_job": second, "n": 2}, + }, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + assert sorted(e["n"] for e in yaml_data["edges"].values()) == [1, 2] + for edge_key in yaml_data["edges"]: + assert grapheme_length(edge_key) <= MAX_EDGE_KEY_LENGTH + assert_no_special_chars(yaml_data, context=f"mode={mode}") + + +# --- by-name resolution must not bind to the wrong step ------------------------ + + +@pytest.mark.usefixtures("ascii_mode") +def test_a_nameless_job_and_an_empty_reference_do_not_bind() -> None: + """`str(job_data.get("name"))` gave "None" for a job with no name, and + `str(reference)` gave "None" for an empty `source_job:`, so they matched + and the edge bound to a fabricated step.""" + yaml_data = { + "jobs": {"a": {}, "b": {"name": "B"}}, + "edges": {"e": {"source_job": "", "target_job": "b"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + edge = next(iter(yaml_data["edges"].values())) + assert edge["source_job"] != "a" + assert edge["source_job"] == AnthropicClient.UNRESOLVED_REFERENCE + + +@pytest.mark.usefixtures("ascii_mode") +def test_by_name_resolution_uses_the_name_the_model_wrote() -> None: + """Matching after sanitizing folded `Résumé` and `Resume` together, so + which step an edge bound to depended on document order.""" + yaml_data = { + "jobs": {"k1": {"name": "Résumé"}, "k2": {"name": "Resume"}}, + "edges": {"e": {"source_job": "Resume", "target_job": "k1"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + assert next(iter(yaml_data["edges"].values()))["source_job"] == "k2" + + +@pytest.mark.usefixtures("ascii_mode") +def test_by_name_resolution_is_order_independent() -> None: + """The same workflow with the two jobs the other way round.""" + yaml_data = { + "jobs": {"k2": {"name": "Resume"}, "k1": {"name": "Résumé"}}, + "edges": {"e": {"source_job": "Resume", "target_job": "k1"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + assert next(iter(yaml_data["edges"].values()))["source_job"] == "k2" + + +@pytest.mark.usefixtures("ascii_mode") +def test_a_boolean_reference_does_not_bind_to_an_int_key() -> None: + """`hash(True) == hash(1)`, so keying the mapping on the raw key swapped + str shadowing for hash shadowing.""" + yaml_data = { + "jobs": {1: {"name": "One"}}, + "edges": {"e": {"source_job": True, "target_job": 1}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + edge = next(iter(yaml_data["edges"].values())) + assert edge["target_job"] in yaml_data["jobs"] + assert edge["source_job"] != edge["target_job"] + + +# --- preservation goes through the same walker as redaction -------------------- + +LIGHTNING_EXPORT = """\ +name: my-project +workflows: + my-workflow: + jobs: + a: + name: A + body: | + const SECRET_X = 'leak-me'; +""" + + +def test_component_extraction_does_not_leak_a_nested_body() -> None: + """`extract_and_preserve_components` read only a top-level `jobs:`, so a + Lightning export matched nothing, was swapped for nothing, and went into + the system prompt whole.""" + _, processed = AnthropicClient.extract_and_preserve_components( + yaml.safe_load(LIGHTNING_EXPORT), + ) + + assert "SECRET_X" not in processed + assert "leak-me" not in processed + + +@pytest.mark.parametrize( + "document", + [ + LIGHTNING_EXPORT, + "jobs:\n a:\n body: {k: SECRET_X}\n", + "x:\n - body: SECRET_X\n", + "deep:\n deeper:\n body: SECRET_X\n", + ], +) +def test_no_document_shape_reaches_the_prompt_with_a_body(document: str) -> None: + _, processed = AnthropicClient.extract_and_preserve_components(yaml.safe_load(document)) + + assert "SECRET_X" not in str(processed) + + +def test_the_normal_shape_keeps_its_placeholder_naming() -> None: + """The prompt tells the model placeholders look like `__CODE_BLOCK___`.""" + preserved, _ = AnthropicClient.extract_and_preserve_components( + yaml.safe_load("jobs:\n fetch:\n id: i\n body: get('/x');\n"), + ) + + assert "__CODE_BLOCK_fetch__" in preserved + assert preserved["__CODE_BLOCK_fetch__"] == "get('/x');" + + +# --- a sanitised reference must not bind to a real but wrong step -------------- + + +@pytest.mark.usefixtures("unicode_mode") +def test_a_reference_in_a_different_normal_form_still_resolves_by_name() -> None: + """The by-name map compared raw strings, so a name stored in NFD and a + reference written in NFC were different strings and never matched.""" + nfd = unicodedata.normalize("NFD", "Résumé") + nfc = unicodedata.normalize("NFC", "Résumé") + yaml_data = { + "jobs": {"k1": {"name": nfd}, "k2": {"name": "Other"}}, + "edges": {"e": {"source_job": nfc, "target_job": "k2"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + assert next(iter(yaml_data["edges"].values()))["source_job"] == "k1" + + +@pytest.mark.usefixtures("unicode_mode") +def test_a_reference_with_a_trailing_space_resolves_to_the_step() -> None: + """The reference and the key are the same name written differently, so this + edge is correct and must survive. Round six sent it to the sentinel.""" + yaml_data = { + "jobs": {"fetch": {"name": "F"}}, + "edges": {"e": {"source_job": "fetch ", "target_job": "fetch"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + edge = next(iter(yaml_data["edges"].values())) + assert edge["source_job"] == "fetch" + assert edge["target_job"] == "fetch" + + +@pytest.mark.usefixtures("ascii_mode") +def test_a_boolean_key_and_its_string_form_resolve_to_the_same_step() -> None: + """`on:` parses as the boolean True and sanitizes to the string "True", so + after sanitizing both references name the same, only, step. Binding them + both to it is correct — the bug is binding across *different* steps, which + the test below covers.""" + yaml_data = { + "jobs": {True: {"name": "T"}}, + "edges": {"e": {"source_job": "True", "target_job": True}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + edge = next(iter(yaml_data["edges"].values())) + assert edge["source_job"] in yaml_data["jobs"] + assert edge["target_job"] == edge["source_job"] + + +# --- null name, and an edge key with nothing to derive a label from ------------ + + +@pytest.mark.usefixtures("ascii_mode") +def test_a_null_name_does_not_become_the_literal_string_none() -> None: + yaml_data = {"jobs": {"a": {"name": None}}, "edges": {}} + + AnthropicClient.sanitize_job_names(yaml_data) + + assert yaml_data["jobs"]["a"]["name"] != "None" + assert yaml_data["jobs"]["a"]["name"] is None + + +@pytest.mark.parametrize("mode", ["false", "true"]) +def test_an_edge_key_with_no_endpoints_is_still_sanitised( + monkeypatch: pytest.MonkeyPatch, mode: str, +) -> None: + """It used to ship verbatim, NUL included — which crashes the insert on + Lightning's side just as surely as a name would.""" + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + yaml_data = {"jobs": {"a": {"name": "A"}}, "edges": {"bad\x00key": {"enabled": True}}} + + AnthropicClient.sanitize_job_names(yaml_data) + + for edge_key in yaml_data["edges"]: + assert "\x00" not in edge_key + + +# --- exact match beats a fold, and ambiguity is refused ------------------------ + + +@pytest.mark.usefixtures("ascii_mode") +@pytest.mark.parametrize("reverse", [False, True]) +def test_an_exact_name_match_wins_over_a_fold(reverse: bool) -> None: + """`Fetch Patients` and `fetch patients` fold together but are two names. + + Taking the first fold hit bound the edge to whichever came first in the + document, so reversing the jobs flipped the binding. + """ + jobs = [("upper", {"name": "Fetch Patients"}), ("lower", {"name": "fetch patients"})] + if reverse: + jobs.reverse() + yaml_data = { + "jobs": dict(jobs), + "edges": {"e": {"source_job": "fetch patients", "target_job": "upper"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + assert next(iter(yaml_data["edges"].values()))["source_job"] == "lower" + + +@pytest.mark.usefixtures("ascii_mode") +def test_an_ambiguous_name_reference_is_refused_not_guessed() -> None: + """Two names that fold together and neither matches exactly. A visible + dangle beats a silent binding to the wrong step.""" + yaml_data = { + "jobs": {"a": {"name": "Fetch Patients"}, "b": {"name": "fetch-patients"}}, + "edges": {"e": {"source_job": "FETCH PATIENTS", "target_job": "a"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + assert next(iter(yaml_data["edges"].values()))["source_job"] not in ("a", "b") + + +# --- the sentinel guard must not destroy correct edges ------------------------- + + +@pytest.mark.parametrize( + ("mode", "job_key"), + [ + ("false", "fetch "), + ("false", "fetch\t"), + ("false", "fetch\x00"), + ("true", "fetch "), + ("true", "fetch\x00"), + ], +) +def test_a_key_that_sanitises_to_the_reference_still_resolves( + monkeypatch: pytest.MonkeyPatch, mode: str, job_key: str, +) -> None: + """Round six pushed these onto the sentinel. The key and the reference are + the same name written differently, so the edge was right and got destroyed.""" + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + yaml_data = { + "jobs": {job_key: {"name": "F"}}, + "edges": {"e": {"source_job": "fetch", "target_job": "fetch"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + edge = next(iter(yaml_data["edges"].values())) + assert edge["source_job"] == "fetch" + assert edge["source_job"] in yaml_data["jobs"] + + +@pytest.mark.usefixtures("unicode_mode") +def test_a_key_in_a_different_normal_form_still_resolves() -> None: + nfd = unicodedata.normalize("NFD", "fetché") + nfc = unicodedata.normalize("NFC", "fetché") + yaml_data = { + "jobs": {nfd: {"name": "F"}}, + "edges": {"e": {"source_job": nfc, "target_job": nfc}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + edge = next(iter(yaml_data["edges"].values())) + assert edge["source_job"] in yaml_data["jobs"] + + +# --- the backstop must not delete the user's code ------------------------------ + + +def test_a_nested_body_survives_the_round_trip() -> None: + """The swap walks the whole tree; restore used to walk a top-level `jobs:` + only, so the user's code went out as `body: __CODE_BLOCK_nested_0__`.""" + data = yaml.safe_load(LIGHTNING_EXPORT) + preserved, prompt = AnthropicClient.extract_and_preserve_components(data) + + assert "SECRET_X" not in prompt + + client = AnthropicClient.__new__(AnthropicClient) + AnthropicClient.restore_components(client, data, preserved) + restored = yaml.dump(data, allow_unicode=True) + + assert "SECRET_X" in restored + assert "__CODE_BLOCK_" not in restored + + +def test_an_unresolvable_placeholder_becomes_the_empty_marker() -> None: + """Losing the code is bad; shipping a swap token the user will save is worse.""" + data = yaml.safe_load("jobs:\n a:\n body: __CODE_BLOCK_job_gone__\n") + + client = AnthropicClient.__new__(AnthropicClient) + AnthropicClient.restore_components(client, data, {}) + + assert data["jobs"]["a"]["body"] == "// Add operations here" + + +def test_a_job_key_cannot_collide_with_a_backstop_index() -> None: + """Both loops keyed into one flat namespace, so a job keyed `1` and + backstop index 1 built the same token.""" + data = yaml.safe_load("jobs:\n 1:\n body: AAA\nx:\n - body: BBB\n") + + preserved, _ = AnthropicClient.extract_and_preserve_components(data) + + assert len(set(preserved.values())) == len(preserved) + assert sorted(k for k in preserved if "CODE_BLOCK" in k) == [ + "__CODE_BLOCK_1__", + "__CODE_BLOCK_nested_1__", + ] + + +# --- a block-scalar placeholder must not ship as the user's code --------------- + + +@pytest.mark.parametrize( + "written_back", + [ + "__CODE_BLOCK_fetch__", + "__CODE_BLOCK_fetch__\n", + " __CODE_BLOCK_fetch__ ", + "__CODE_BLOCK_fetch__\n\n", + ], +) +def test_a_placeholder_written_back_as_a_block_scalar_restores_the_code( + written_back: str, +) -> None: + """A block scalar is the natural style for a `body:`, and it parses with a + trailing newline. The lookup did not strip while `_is_redacted` did, so the + raw token matched neither branch and shipped to the user in place of their + code — which they would then save.""" + data = {"jobs": {"fetch": {"body": written_back, "id": "i"}}} + preserved = {"__CODE_BLOCK_fetch__": "get('/patients');"} + + client = AnthropicClient.__new__(AnthropicClient) + AnthropicClient.restore_components(client, data, preserved) + + assert data["jobs"]["fetch"]["body"] == "get('/patients');" + + +def test_a_block_scalar_placeholder_through_finalize_yaml() -> None: + """The whole pipeline, not just the one function: `finalize_yaml` detected + the surviving token, logged it, and returned it anyway.""" + parsed = yaml.safe_load( + "jobs:\n fetch:\n id: i\n adaptor: '@openfn/language-common@latest'\n" + " body: |\n __CODE_BLOCK_fetch__\n", + ) + preserved = {"__CODE_BLOCK_fetch__": "get('/patients');", "__ID_JOB_fetch__": "i"} + + client = AnthropicClient.__new__(AnthropicClient) + client.validate_adaptors = lambda _data: None + out = AnthropicClient.finalize_yaml(client, parsed, preserved) + + assert "__CODE_BLOCK_" not in out + assert "get('/patients');" in out + + +def test_an_unknown_block_scalar_placeholder_does_not_ship() -> None: + """No preserved value for it. Losing the code is bad; shipping a token the + user will save is worse — that is the state origin/main did not reach.""" + data = {"jobs": {"a": {"body": "__CODE_BLOCK_gone__\n"}}} + + client = AnthropicClient.__new__(AnthropicClient) + AnthropicClient.restore_components(client, data, {}) + + assert data["jobs"]["a"]["body"] == "// Add operations here" + + +# --- an exact name match is not automatically unique -------------------------- + + +@pytest.mark.usefixtures("ascii_mode") +@pytest.mark.parametrize("reverse", [False, True]) +def test_two_jobs_sharing_a_name_are_refused_not_ordered(reverse: bool) -> None: + """`_unique_name` in this same class exists because two jobs can arrive + sharing a name. The exact-match loop took the first hit, so the binding + flipped with document order.""" + jobs = [("first", {"name": "Fetch Data"}), ("second", {"name": "Fetch Data"})] + if reverse: + jobs.reverse() + yaml_data = { + "jobs": dict(jobs), + "edges": {"e": {"source_job": "Fetch Data", "target_job": "first"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + assert next(iter(yaml_data["edges"].values()))["source_job"] not in ("first", "second") + + +# --- a decorated placeholder must not ship either ------------------------------ + +DECORATED_PLACEHOLDERS = { + "fenced code block": "```\n__CODE_BLOCK_fetch__\n```", + "inline backticks": "`__CODE_BLOCK_fetch__`", + "line comment": "// __CODE_BLOCK_fetch__", + "byte order mark": "\ufeff__CODE_BLOCK_fetch__", + "zero width space": "\u200b__CODE_BLOCK_fetch__", + "quoted": '"__CODE_BLOCK_fetch__"', + "key prefix": "code: __CODE_BLOCK_fetch__", + "token then code": "__CODE_BLOCK_fetch__\nfn(s => s);", +} + + +@pytest.mark.parametrize(("shape", "body"), DECORATED_PLACEHOLDERS.items()) +def test_a_decorated_placeholder_never_ships_as_the_body(shape: str, body: str) -> None: + """Stripping only catches the token written back bare. A fenced code block + is a strong model habit, so every one of these shipped the raw token as the + user's code.""" + data = {"jobs": {"fetch": {"id": "i", "body": body}}} + preserved = {"__CODE_BLOCK_fetch__": "get('/patients');"} + + client = AnthropicClient.__new__(AnthropicClient) + AnthropicClient.restore_components(client, data, preserved) + + restored = data["jobs"]["fetch"]["body"] + assert "__CODE_BLOCK_" not in restored, shape + assert "get('/patients');" in restored, shape + # Whatever else the model wrote around the token is still there. Replacing + # the whole body on a mere mention collapsed a long body to one statement. + for surrounding in ("fn(s => s);", "code:"): + if surrounding in body: + assert surrounding in restored, shape + + +@pytest.mark.parametrize(("shape", "body"), DECORATED_PLACEHOLDERS.items()) +def test_a_decorated_token_we_issued_recovers_the_code(shape: str, body: str) -> None: + """We know what the token stood for, so put the code back rather than + throwing it away. Degrading here lost a body we were holding.""" + parsed = {"jobs": {"fetch": {"id": "i", "body": body}}} + preserved = {"__CODE_BLOCK_fetch__": "get('/patients');"} + + client = AnthropicClient.__new__(AnthropicClient) + client.validate_adaptors = lambda _data: None + out = AnthropicClient.finalize_yaml(client, parsed, preserved) + restored = yaml.safe_load(out)["jobs"]["fetch"]["body"] + + assert "get('/patients');" in restored, shape + assert "__CODE_BLOCK_" not in restored, shape + for surrounding in ("fn(s => s);", "code:"): + if surrounding in body: + assert surrounding in restored, shape + + +@pytest.mark.parametrize( + "body", + [ + "// see __CODE_BLOCK_jobname__ in the prompt\nfn(s => s);", + 'const marker = "__CODE_BLOCK_jobname__";\npost(marker);', + "// __CODE_BLOCK_jobname__ is what the prompt calls it\nget('/x');", + ], +) +def test_real_code_that_mentions_the_sentinel_survives(body: str) -> None: + """`gen_project_prompts.yaml` shows the model the literal token, so a model + quoting it back is ordinary output. + + `preserved` is deliberately non-empty: the docstring describes the model + quoting the prompt *while editing a job*, which is exactly when there are + preserved bodies. An earlier version passed `{}` and so could not reach the + case it was named for — with a real `preserved` the code was destroyed. + """ + parsed = {"jobs": {"a": {"id": "i", "body": body}}} + preserved = {"__CODE_BLOCK_a__": "get('/patients');", "__CODE_BLOCK_other__": "post('/x');"} + + client = AnthropicClient.__new__(AnthropicClient) + client.validate_adaptors = lambda _data: None + out = AnthropicClient.finalize_yaml(client, parsed, preserved) + restored = yaml.safe_load(out)["jobs"]["a"]["body"] + + assert "Add operations here" not in restored + assert "__CODE_BLOCK_jobname__" in restored, "a token we never issued is real code, not a placeholder" + assert restored == body, "the body must come back exactly as the model wrote it" + + +def test_id_shaped_text_inside_real_code_is_not_flagged() -> None: + """`"__ID_" in dumped` matched inside a body and raised a Sentry error + claiming a token survived "outside a job body", which was false.""" + parsed = {"jobs": {"a": {"id": "real-id", "body": "const __ID_FIELD = state.data.id;"}}} + + client = AnthropicClient.__new__(AnthropicClient) + client.validate_adaptors = lambda _data: None + out = AnthropicClient.finalize_yaml(client, parsed, {}) + + assert "__ID_FIELD" in out + + +@pytest.mark.parametrize("body", ["__CODE_BLOCK_gone__", "__CODE_BLOCK_gone__\n", "```\n__CODE_BLOCK_gone__\n```"]) +def test_a_token_we_never_issued_still_degrades(body: str) -> None: + """Nothing to restore it from, and shipping it puts a swap token in front + of the user as if it were their code.""" + parsed = {"jobs": {"a": {"id": "i", "body": body}}} + + client = AnthropicClient.__new__(AnthropicClient) + client.validate_adaptors = lambda _data: None + out = AnthropicClient.finalize_yaml(client, parsed, {}) + + assert "__CODE_BLOCK_" not in out + assert "Add operations here" in out + + +@pytest.mark.parametrize("body", ["__CODE_BLOCK_fetch__", "__CODE_BLOCK_fetch__\n", " __CODE_BLOCK_fetch__ "]) +def test_a_resolvable_placeholder_still_restores(body: str) -> None: + """The broadened degrade must not swallow the bodies that do resolve.""" + parsed = {"jobs": {"fetch": {"id": "__ID_JOB_fetch__", "body": body}}} + preserved = {"__CODE_BLOCK_fetch__": "get('/patients');", "__ID_JOB_fetch__": "the-id"} + + client = AnthropicClient.__new__(AnthropicClient) + client.validate_adaptors = lambda _data: None + out = AnthropicClient.finalize_yaml(client, parsed, preserved) + + assert "get('/patients');" in out + assert "__CODE_BLOCK_" not in out + assert "the-id" in out + + +# --- non-string job names ----------------------------------------------------- + + +@pytest.mark.usefixtures("ascii_mode") +def test_non_string_names_are_coerced_and_sanitised() -> None: + """`name: 2024`, `name: on` and `name: 01` are ordinary model output, and + YAML hands them over as an int, a bool and an int. + + A string-only filter left them unsanitized and unrenamed, and Ecto rejects + a `:string` cast from an integer — so a workflow that used to save stopped + saving. Base coerced with `str(...)` first. + """ + yaml_data = { + "jobs": { + "s1": {"name": 2024}, + "s2": {"name": True}, + "s3": {"name": 1}, + }, + "edges": {}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + names = [job["name"] for job in yaml_data["jobs"].values()] + assert all(isinstance(name, str) for name in names), names + assert names == ["2024", "True", "1"] + + +@pytest.mark.usefixtures("ascii_mode") +def test_an_edge_resolves_a_non_string_name() -> None: + """The by-name map had the same filter, so an edge referencing such a job + by name never resolved.""" + yaml_data = { + "jobs": {"s1": {"name": 2024}, "s2": {"name": "Other"}}, + "edges": {"e": {"source_job": "2024", "target_job": "s2"}}, + } + + AnthropicClient.sanitize_job_names(yaml_data) + + assert next(iter(yaml_data["edges"].values()))["source_job"] == "s1" + + +@pytest.mark.usefixtures("ascii_mode") +def test_a_null_name_is_still_left_alone() -> None: + """The filter was written for this case, and it is the only one it was + right about: `str(None)` is the literal name "None".""" + yaml_data = {"jobs": {"a": {"name": None}}, "edges": {}} + + AnthropicClient.sanitize_job_names(yaml_data) + + assert yaml_data["jobs"]["a"]["name"] is None + + +# --- restoring must not discard the code around the token ---------------------- + + +def test_a_token_embedded_in_a_long_body_keeps_the_rest_of_it() -> None: + """Replacing the whole body on a mere mention collapsed a 500-line body + whose last line named the token into the one statement it stood for — and + handed that back as working code, which is worse than losing it visibly.""" + lines = 50 + body = "\n".join(["const x = 1;", *[f"post('/y/{n}', x);" for n in range(lines)], "// __CODE_BLOCK_a__"]) + data = {"jobs": {"a": {"id": "i", "body": body}}} + + client = AnthropicClient.__new__(AnthropicClient) + AnthropicClient.restore_components(client, data, {"__CODE_BLOCK_a__": "get('/patients');"}) + + restored = data["jobs"]["a"]["body"] + assert "get('/patients');" in restored + assert "const x = 1;" in restored + assert restored.count("post('/y/") == lines + + +def test_a_token_alone_on_a_comment_line_is_not_left_commented_out() -> None: + """Substituting the token alone turns `// __CODE_BLOCK_a__` into + `// get(...)`, which preserves the text and makes the step do nothing.""" + data = {"jobs": {"a": {"id": "i", "body": "// __CODE_BLOCK_a__\nconst x = 1;"}}} + + client = AnthropicClient.__new__(AnthropicClient) + AnthropicClient.restore_components(client, data, {"__CODE_BLOCK_a__": "get('/patients');"}) + + assert data["jobs"]["a"]["body"] == "get('/patients');\nconst x = 1;" + + +# --- two tokens in one body ---------------------------------------------------- + + +def test_two_issued_tokens_in_one_body_both_restore() -> None: + """"Merge step a and step b" produces exactly this. Returning `None` on an + ambiguous match left the body alone, so two raw swap tokens shipped as the + user's code with nothing logged.""" + data = {"jobs": {"merged": {"id": "i", "body": "__CODE_BLOCK_a__\n__CODE_BLOCK_b__"}}} + preserved = {"__CODE_BLOCK_a__": "get('/patients');", "__CODE_BLOCK_b__": "post('/dhis2');"} + + client = AnthropicClient.__new__(AnthropicClient) + AnthropicClient.restore_components(client, data, preserved) + + restored = data["jobs"]["merged"]["body"] + assert "__CODE_BLOCK_" not in restored + assert "get('/patients');" in restored + assert "post('/dhis2');" in restored + + +def test_two_tokens_we_never_issued_do_not_ship() -> None: + data = {"jobs": {"a": {"id": "i", "body": "__CODE_BLOCK_x__\n__CODE_BLOCK_y__"}}} + + client = AnthropicClient.__new__(AnthropicClient) + AnthropicClient.restore_components(client, data, {}) + + assert "__CODE_BLOCK_" not in data["jobs"]["a"]["body"] + + +# --- claims ------------------------------------------------------------------- + + +def test_the_same_body_restored_into_two_steps_is_reported( + caplog: pytest.LogCaptureFixture, +) -> None: + """The placeholder contract lets the model move code between steps, so this + is legitimate — but two steps ending up with the same body is not something + to discover from a support ticket.""" + data = { + "jobs": { + "a": {"id": "1", "body": "__CODE_BLOCK_b__"}, + "b": {"id": "2", "body": "__CODE_BLOCK_b__"}, + }, + } + preserved = {"__CODE_BLOCK_a__": "get('/a');", "__CODE_BLOCK_b__": "post('/b');"} + + client = AnthropicClient.__new__(AnthropicClient) + with caplog.at_level("WARNING"): + AnthropicClient.restore_components(client, data, preserved) + + assert any("more than one step" in record.message for record in caplog.records) + + +def test_a_preserved_body_nobody_asked_for_is_reported( + caplog: pytest.LogCaptureFixture, +) -> None: + """Job a's own preserved value never being claimed is the other half of the + same contamination, and it was equally silent.""" + data = {"jobs": {"a": {"id": "1", "body": "// Add operations here"}}} + preserved = {"__CODE_BLOCK_a__": "get('/a');"} + + client = AnthropicClient.__new__(AnthropicClient) + # INFO, not WARNING: deleting a step leaves its preserved body unclaimed + # every time, so this is ordinary and must not page anyone or ship a + # Sentry event carrying the request context. + with caplog.at_level("INFO"): + AnthropicClient.restore_components(client, data, preserved) + + unclaimed = [r for r in caplog.records if "never restored" in r.message] + assert unclaimed + assert all(record.levelname == "INFO" for record in unclaimed) + + +# --- a job key containing `__` ------------------------------------------------ + + +def test_a_job_key_containing_a_double_underscore_restores() -> None: + """`__CODE_BLOCK_sync__patients__` with a non-greedy match stops at + `__CODE_BLOCK_sync__` and leaves `patients__` behind, so the body came back + with the real code gone and the tail of the token still in it. Underscore + is legal in a step name in both modes and in Lightning's own rule.""" + data = {"jobs": {"sync__patients": {"id": "i", "body": "// unchanged\n__CODE_BLOCK_sync__patients__"}}} + + client = AnthropicClient.__new__(AnthropicClient) + AnthropicClient.restore_components(client, data, {"__CODE_BLOCK_sync__patients__": "get('/patients');"}) + + restored = data["jobs"]["sync__patients"]["body"] + assert "__CODE_BLOCK_" not in restored + assert restored == "// unchanged\nget('/patients');" + + +def test_a_truncated_prefix_sibling_is_not_spliced_in() -> None: + """With a second job whose key is the truncated prefix, the short match + picked that job's body instead.""" + data = {"jobs": {"sync__patients": {"id": "i", "body": "__CODE_BLOCK_sync__patients__"}}} + preserved = {"__CODE_BLOCK_sync__": "WRONG();", "__CODE_BLOCK_sync__patients__": "RIGHT();"} + + client = AnthropicClient.__new__(AnthropicClient) + AnthropicClient.restore_components(client, data, preserved) + + assert data["jobs"]["sync__patients"]["body"] == "RIGHT();" + + +@pytest.mark.parametrize( + ("body", "issued", "expected"), + [ + # A key containing `__`. A non-greedy pattern stopped at + # `__CODE_BLOCK_sync__`. + ("__CODE_BLOCK_sync__patients__", ["__CODE_BLOCK_sync__patients__"], + ["__CODE_BLOCK_sync__patients__"]), + # A prefix key must not win over the longer one it is a prefix of. + ("__CODE_BLOCK_sync__patients__", + ["__CODE_BLOCK_sync__", "__CODE_BLOCK_sync__patients__"], + ["__CODE_BLOCK_sync__patients__"]), + # An identifier character after the token. The lookahead that fixed the + # case above lost this one: zero tokens found, so the raw token shipped. + ("__CODE_BLOCK_a__1", ["__CODE_BLOCK_a__"], ["__CODE_BLOCK_a__"]), + # Two adjacent. The same lookahead swallowed both as one bogus match, + # and the body was replaced with the empty marker. + ("__CODE_BLOCK_a____CODE_BLOCK_b__", ["__CODE_BLOCK_a__", "__CODE_BLOCK_b__"], + ["__CODE_BLOCK_a__", "__CODE_BLOCK_b__"]), + ("__CODE_BLOCK_a__\n__CODE_BLOCK_b__", ["__CODE_BLOCK_a__", "__CODE_BLOCK_b__"], + ["__CODE_BLOCK_a__", "__CODE_BLOCK_b__"]), + # A token we never issued is not ours to find. + ("__CODE_BLOCK_jobname__", ["__CODE_BLOCK_a__"], []), + ], +) +def test_tokens_are_matched_against_the_keys_we_issued( + body: str, issued: list, expected: list, +) -> None: + """Matched against `preserved_values`, longest key first — the ground truth + we already hold — rather than by a pattern guessing at the token's shape. + Two rounds of tuning that pattern traded one failure for another.""" + preserved = dict.fromkeys(issued, "code();") + + assert AnthropicClient._issued_tokens_in(body, preserved) == expected + + +# --- a block comment must not leave the step inert ----------------------------- + + +@pytest.mark.parametrize("body", ["/* __CODE_BLOCK_a__ */", "/* __CODE_BLOCK_a__ */\nconst x = 1;"]) +def test_a_block_comment_wrapping_the_token_is_replaced_whole(body: str) -> None: + """Recognising only `//` and `#` turned `/* __CODE_BLOCK_a__ */` into + `/* get(...) */`: body intact, step does nothing.""" + data = {"jobs": {"a": {"id": "i", "body": body}}} + + client = AnthropicClient.__new__(AnthropicClient) + AnthropicClient.restore_components(client, data, {"__CODE_BLOCK_a__": "get('/x');"}) + + restored = data["jobs"]["a"]["body"] + assert restored.startswith("get('/x');") + assert "/*" not in restored.split("\n")[0] + + +# --- a non-string body anywhere in the tree ------------------------------------ + + +def test_a_non_string_nested_body_does_not_ship_a_token() -> None: + """The walker reaches nested holders and skips non-strings; the `jobs:` + default pass only reaches the top level, so this shipped a raw token.""" + data = {"workflows": {"w": {"jobs": {"a": {"body": ["__CODE_BLOCK_nested_0__"]}}}}} + + client = AnthropicClient.__new__(AnthropicClient) + AnthropicClient.restore_components(client, data, {}) + + assert data["workflows"]["w"]["jobs"]["a"]["body"] == "// Add operations here" + + +@pytest.mark.parametrize( + ("body", "expected"), + [ + ("__CODE_BLOCK_a__1", "get('/a');1"), + ("__CODE_BLOCK_a____CODE_BLOCK_b__", "get('/a');\npost('/b');"), + ], +) +def test_the_two_regressions_from_tuning_the_pattern(body: str, expected: str) -> None: + """`__CODE_BLOCK_a__1` found zero tokens and shipped the raw token with no + warning; two adjacent tokens merged into one bogus match and the body was + replaced with the empty marker, destroying both preserved bodies.""" + data = {"jobs": {"j": {"id": "i", "body": body}}} + preserved = {"__CODE_BLOCK_a__": "get('/a');", "__CODE_BLOCK_b__": "post('/b');"} + + client = AnthropicClient.__new__(AnthropicClient) + AnthropicClient.restore_components(client, data, preserved) + + restored = data["jobs"]["j"]["body"] + assert "__CODE_BLOCK_" not in restored + assert restored == expected + + +# --- prefix-pair token texts --------------------------------------------------- + + +@pytest.mark.parametrize( + ("key_one", "key_two"), + [("sync", "sync_"), ("sync", "sync__"), ("a", "a_"), ("a", "a__"), + ("long_key", "long_key_"), ("1", "1_")], +) +@pytest.mark.parametrize("separator", ["", "\n", " "]) +def test_adjacent_prefix_pair_tokens_both_restore( + key_one: str, key_two: str, separator: str, +) -> None: + """Issued token texts are not prefix-free: `__CODE_BLOCK_{key}__` makes one + a prefix of another exactly when the second key is the first plus one or + two underscores. Written adjacently, the longer token matched *across the + boundary*, so the shorter key's body was lost and the fragment + `CODE_BLOCK_sync___` was left in the user's code. + """ + token_one, token_two = f"__CODE_BLOCK_{key_one}__", f"__CODE_BLOCK_{key_two}__" + preserved = {token_one: "ONE();", token_two: "TWO();"} + data = {"jobs": {"j": {"id": "i", "body": token_one + separator + token_two}}} + + client = AnthropicClient.__new__(AnthropicClient) + AnthropicClient.restore_components(client, data, preserved) + + restored = data["jobs"]["j"]["body"] + assert "CODE_BLOCK" not in restored, "a fragment of the token survived" + assert "ONE();" in restored + assert "TWO();" in restored + + +def test_a_prefix_pair_in_the_other_order_also_restores() -> None: + token_two = "__CODE_BLOCK_sync__" + preserved = {"__CODE_BLOCK_sync___": "UNDER();", token_two: "PLAIN();"} + data = {"jobs": {"j": {"id": "i", "body": "__CODE_BLOCK_sync___" + token_two}}} + + client = AnthropicClient.__new__(AnthropicClient) + AnthropicClient.restore_components(client, data, preserved) + + restored = data["jobs"]["j"]["body"] + assert "CODE_BLOCK" not in restored + assert "UNDER();" in restored + assert "PLAIN();" in restored + + +def test_token_debris_is_reported_loudly(caplog: pytest.LogCaptureFixture) -> None: + """An unclaimed preserved body is ordinary — deleting a step produces one + every time — so that is info. A fragment of our machinery sitting in the + user's code never is.""" + data = {"jobs": {"j": {"id": "i", "body": "SYNC();CODE_BLOCK_sync___"}}} + + client = AnthropicClient.__new__(AnthropicClient) + with caplog.at_level("ERROR"): + AnthropicClient.restore_components(client, data, {}) + + assert any("fragment of a code placeholder" in record.message for record in caplog.records) + assert all(record.levelname == "ERROR" for record in caplog.records) diff --git a/services/workflow_chat/tests/unit/gen_project/conftest.py b/services/workflow_chat/tests/unit/gen_project/conftest.py new file mode 100644 index 00000000..aa558136 --- /dev/null +++ b/services/workflow_chat/tests/unit/gen_project/conftest.py @@ -0,0 +1,25 @@ +"""Keep the prompt-building tests offline. + +Building a system message pulls in the adaptor list, and +`get_latest_adaptors_cached` fetches it from the GitHub API whenever the +on-disk cache is missing or more than an hour old. That made this file's tests +depend on the network and on GitHub rate limits. Stub the fetch instead — none +of these tests care what the adaptor list contains. +""" + +import pytest +from workflow_chat import available_adaptors + +_FAKE_ADAPTORS = { + "common": {"version": "1.0.0", "description": "Common operations", "label": "Common"}, + "http": {"version": "1.0.0", "description": "HTTP requests", "label": "HTTP"}, +} + + +@pytest.fixture(autouse=True) +def _offline_adaptors(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr( + available_adaptors, + "get_latest_adaptors_cached", + lambda: dict(_FAKE_ADAPTORS), + ) diff --git a/services/workflow_chat/tests/unit/gen_project/test_prompt_build.py b/services/workflow_chat/tests/unit/gen_project/test_prompt_build.py index b1500d7f..dd1ff8a2 100644 --- a/services/workflow_chat/tests/unit/gen_project/test_prompt_build.py +++ b/services/workflow_chat/tests/unit/gen_project/test_prompt_build.py @@ -1,3 +1,5 @@ +import pytest +from name_rules import UNICODE_FLAG_ENV, describe_rule_for_prompt from workflow_chat.gen_project_prompt import build_prompt @@ -77,3 +79,47 @@ def test_build_prompt_readonly_mode(): assert "name: readonly-workflow" in system_msg assert prompt[-1]["content"] == "What does this workflow do?" + + +@pytest.mark.parametrize("mode", ["false", "true"]) +def test_build_prompt_states_the_active_name_rule(monkeypatch: pytest.MonkeyPatch, mode: str) -> None: + """The rule the model is told and the rule the sanitizer enforces come from + the same source, so the prompt has to move when the flag moves.""" + monkeypatch.setenv(UNICODE_FLAG_ENV, mode) + + system_msg, _ = build_prompt(content="Create a workflow") + + assert describe_rule_for_prompt() in system_msg + assert "{name_rule}" not in system_msg + + +def test_build_prompt_name_rule_differs_between_modes(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv(UNICODE_FLAG_ENV, "false") + ascii_msg, _ = build_prompt(content="Create a workflow") + + monkeypatch.setenv(UNICODE_FLAG_ENV, "true") + unicode_msg, _ = build_prompt(content="Create a workflow") + + assert ascii_msg != unicode_msg + assert "any script" in unicode_msg + assert "any script" not in ascii_msg + + +def test_a_prompt_that_drops_the_name_rule_token_is_rejected_loudly( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """`str.format` ignores an unused keyword, so losing the token is silent. + + The prompt would then state no naming rule at all while the sanitizer + carried on enforcing one, and the model would be left guessing. + """ + from workflow_chat import gen_project_prompt + + monkeypatch.setattr( + gen_project_prompt.config_loader, + "get_prompt", + lambda name: "no token here {adaptors}" if name == "general_knowledge" else "x", + ) + + with pytest.raises(ValueError, match="did not render the step-name rule"): + gen_project_prompt.build_prompt(content="Create a workflow") diff --git a/services/workflow_chat/workflow_chat.py b/services/workflow_chat/workflow_chat.py index 662bf52a..c04f0733 100644 --- a/services/workflow_chat/workflow_chat.py +++ b/services/workflow_chat/workflow_chat.py @@ -2,11 +2,30 @@ import os import re import uuid -import unicodedata -from typing import List, Optional, Dict, Any -import yaml from dataclasses import dataclass +from typing import Any, Dict, List, Optional + +import yaml from models import resolve_model +from name_rules import ( + MAX_EDGE_KEY_LENGTH, + MAX_NAME_LENGTH, + grapheme_length, + normalize_for_lookup, + sanitize_name, + truncate_graphemes, + unicode_names_enabled, +) +from yaml_utils import ( + BODY_KEY, + CODE_PLACEHOLDER_PREFIX, + WITHHELD_NOTICE, + has_unredacted_body, + iter_body_holders, + iter_id_holders, + redact_job_bodies, + remove_ids, +) _dir = os.path.dirname(os.path.abspath(__file__)) with open(os.path.join(_dir, "gen_project_config.yaml")) as _f: @@ -21,13 +40,13 @@ "yaml": { "anyOf": [ {"type": "string"}, - {"type": "null"} - ] + {"type": "null"}, + ], }, - "text": {"type": "string"} + "text": {"type": "string"}, }, "required": ["yaml", "text"], - "additionalProperties": False + "additionalProperties": False, } # Subagent mode (called from global_chat): adds a "handover" field so the model @@ -40,38 +59,39 @@ "handover": { "anyOf": [ {"type": "string"}, - {"type": "null"} - ] + {"type": "null"}, + ], }, - **_OUTPUT_SCHEMA["properties"] + **_OUTPUT_SCHEMA["properties"], }, "required": ["handover", "yaml", "text"], - "additionalProperties": False + "additionalProperties": False, } +import sentry_sdk from anthropic import ( Anthropic, APIConnectionError, - BadRequestError, AuthenticationError, - PermissionDeniedError, + BadRequestError, + InternalServerError, NotFoundError, - UnprocessableEntityError, + PermissionDeniedError, RateLimitError, - InternalServerError, + UnprocessableEntityError, ) -import sentry_sdk -from langfuse import observe, propagate_attributes, get_client as get_langfuse_client -from langfuse_util import should_track, build_tags, build_generation_diff, drop_code, mask_secrets -from util import ApolloError, create_logger, add_page_prefix, APOLLO_VERSION -from yaml_utils import WITHHELD_NOTICE, redact_job_bodies, remove_ids -from .gen_project_prompt import build_prompt -from workflow_chat.available_adaptors import get_available_adaptors +from langfuse import get_client as get_langfuse_client +from langfuse import observe, propagate_attributes +from langfuse_util import build_generation_diff, build_tags, drop_code, mask_secrets, should_track from streaming_util import ( - StreamManager, - STATUS_REVIEWING_WORKFLOW, - STATUS_NEW_WORKFLOW, STATUS_DESIGNING_WORKFLOW, + STATUS_NEW_WORKFLOW, + STATUS_REVIEWING_WORKFLOW, + StreamManager, ) +from util import APOLLO_VERSION, ApolloError, add_page_prefix, create_logger +from workflow_chat.available_adaptors import get_available_adaptors + +from .gen_project_prompt import build_prompt logger = create_logger("workflow_chat") @@ -232,16 +252,16 @@ def generate( errors=errors, history=history, read_only=read_only, - subagent=subagent + subagent=subagent, ) # Structured outputs config — guarantees valid JSON matching schema output_config = { "format": { "type": "json_schema", - "schema": _SUBAGENT_OUTPUT_SCHEMA if subagent else _OUTPUT_SCHEMA + "schema": _SUBAGENT_OUTPUT_SCHEMA if subagent else _OUTPUT_SCHEMA, }, - "effort": "medium" + "effort": "medium", } accumulated_usage = { @@ -274,7 +294,7 @@ def generate( model=self.config.model, system=system_message, output_config=output_config, - thinking={"type": "adaptive"} + thinking={"type": "adaptive"}, ) as stream_obj: for event in stream_obj: accumulated_response, text_started, sent_length = self.process_stream_event( @@ -283,7 +303,7 @@ def generate( text_started, sent_length, stream_manager, - preserved_values + preserved_values, ) message = stream_obj.get_final_message() @@ -300,7 +320,7 @@ def generate( message = self.client.messages.create( max_tokens=self.config.max_tokens, messages=prompt, model=self.config.model, system=system_message, output_config=output_config, - thinking={"type": "adaptive"} + thinking={"type": "adaptive"}, ) # Track usage from this attempt @@ -385,77 +405,587 @@ def remove_ids_from_yaml(self, yaml_str): # This used to be a third id-walker with neither, and it runs on # client-supplied YAML whenever read_only is set. remove_ids(yaml_data) - return yaml.dump(yaml_data, sort_keys=False, default_flow_style=False) + return yaml.dump(yaml_data, sort_keys=False, default_flow_style=False, allow_unicode=True) except Exception as error: # Type only: a PyYAML mark quotes the document. logger.warning(f"Could not remove IDs from YAML ({type(error).__name__})") return yaml_str @staticmethod - def sanitize_job_names(yaml_data): + def _resolve_by_name(reference, job_names): + """Resolve a step reference against job *names*, or None. + + Exact match wins outright. Only if nothing matches exactly does it fall + back to the lookup fold, and then only when the fold picks out exactly + one job: `Fetch Patients` and `fetch patients` are two different names + that fold together, so taking the first fold hit bound the edge to + whichever happened to come first in the document. An ambiguous + reference is not resolved at all — a visible dangle beats a silent + binding to the wrong step. """ - Sanitize job names by removing special characters and normalizing diacritics. - Also sanitizes job references in edges (source and target fields) and edge keys. + if not reference: + return None + + def unambiguous(matches, how): + if len(matches) == 1: + return matches[0] + if matches: + logger.warning( + f"Step reference {reference!r} {how} {len(matches)} jobs " + f"({', '.join(sorted(matches))}); leaving it unresolved rather than guessing", + ) + return None + + # Exact is not automatically unique: two jobs can arrive sharing a name. + exact = [key for key, name in job_names.items() if name == reference] + if exact: + return unambiguous(exact, "exactly matches the name of") + + wanted = normalize_for_lookup(reference) + if not wanted: + return None + + folded = [key for key, name in job_names.items() if normalize_for_lookup(name) == wanted] + return unambiguous(folded, "matches the folded name of") + + #: Token-shaped text. Used for ONE question only: "is this a swap token we + #: never issued?" — the degrade branch. Deliberately not used to find our + #: own tokens: we know exactly which ones we issued, and a pattern can only + #: guess at their shape. + _FOREIGN_TOKEN = re.compile(r"__CODE_BLOCK_\S*?__") + + @staticmethod + def _issued_spans_in(value, preserved_values): + """Where each token we actually issued appears in `value`. + + Matched against the keys of `preserved_values` — the ground truth we + already hold — rather than by a pattern guessing at the token's shape. + + Longest-first alone is not enough, because issued token texts are not + prefix-free: `__CODE_BLOCK_{key}__` makes one token a prefix of another + exactly when the second key is the first plus one or two underscores. + With steps keyed `sync` and `sync_` written adjacently, the longer + token matches *across the boundary*, swallowing the shorter token's + leading underscores — `sync`'s body was lost and the fragment + `CODE_BLOCK_sync___` was left in the user's code. + + So this scans left to right from each token start and prefers a + candidate that leaves a clean remainder: one that does not begin + mid-token. Only 24 of 6,972 ordered key pairs can produce the + collision, all `key2 == key1 + "_"` or `+ "__"`, but a user can name + two steps that way and the uniquifier will not stop them. """ - if not yaml_data: + if not isinstance(value, str): + return [] + + issued = sorted( + (token for token in preserved_values if token.startswith(CODE_PLACEHOLDER_PREFIX)), + key=len, + reverse=True, + ) + if not issued: + return [] + + spans: list[tuple[int, int, str]] = [] + index = 0 + while index < len(value): + if not value.startswith(CODE_PLACEHOLDER_PREFIX, index): + index += 1 + continue + + matches = [token for token in issued if value.startswith(token, index)] + if not matches: + index += 1 + continue + + clean = [ + token + for token in matches + if AnthropicClient._leaves_clean_remainder(value, index + len(token)) + ] + chosen = (clean or matches)[0] + spans.append((index, index + len(chosen), chosen)) + index += len(chosen) + + return spans + + @staticmethod + def _leaves_clean_remainder(value, end): + """True if consuming up to `end` does not cut into a following token. + + The failure it rules out is a match that ate the next token's leading + underscores, which leaves `CODE_BLOCK_...` or `_CODE_BLOCK_...` + stranded at `end`. Ordinary code after a token is clean. + """ + remainder = value[end:] + return not ( + remainder.startswith("CODE_BLOCK_") or remainder.startswith("_CODE_BLOCK_") + ) + + @staticmethod + def _issued_tokens_in(value, preserved_values): + """The tokens we issued that `value` mentions, in the order written.""" + return [token for _, _, token in AnthropicClient._issued_spans_in(value, preserved_values)] + + @staticmethod + def _substitute_issued(value, preserved_values): + """Replace every issued token with the body it stood for. + + Works from the spans rather than `str.replace`, so a token that is a + substring of another cannot be substituted inside it. + """ + spans = AnthropicClient._issued_spans_in(value, preserved_values) + out = [] + cursor = 0 + for start, end, token in spans: + out.append(value[cursor:start]) + out.append(preserved_values[token]) + cursor = end + out.append(value[cursor:]) + return "".join(out) + + @staticmethod + def _is_only_placeholders(value, preserved_values): + """True if `value` is swap tokens and decoration, with no other content. + + This is what separates "the model mangled our token" from "the model + wrote code that mentions one". Restoring on a mere mention replaced the + whole body, so a long body whose last line was a comment naming the + token collapsed to a single statement. + + Issued tokens are removed by span, foreign ones by pattern — the two + questions have different ground truth and are answered differently. + """ + if not isinstance(value, str) or CODE_PLACEHOLDER_PREFIX not in value: + return False + + spans = AnthropicClient._issued_spans_in(value, preserved_values) + remainder, cursor = [], 0 + for start, end, _ in spans: + remainder.append(value[cursor:start]) + cursor = end + remainder.append(value[cursor:]) + text = AnthropicClient._FOREIGN_TOKEN.sub("", "".join(remainder)) + text = re.sub(r"```[a-zA-Z]*", "", text) + text = re.sub(r"(?m)^[ \t]*(?://+|#+|/\*)[ \t]*", "", text) + text = text.replace("*/", "") + return text.strip(" \t\r\n\ufeff\u200b`'\"") == "" + + #: A line that is a comment wrapper and nothing else once the token is + #: taken out. `/* ... */` as well as `//` and `#`: recognising only the + #: latter two turned `/* __CODE_BLOCK_a__ */` into `/* get(...) */`, so the + #: body looked intact and the step did nothing. + _COMMENT_ONLY = re.compile(r"[ \t]*(?://+|\#+|/\*)?[ \t]*(?:\*/)?[ \t]*") + + @staticmethod + def _substitute_issued_by_line(body, preserved_values): + """Substitute every issued token, taking the whole line where the line + is only a comment marker wrapped around it.""" + lines = body.split("\n") + for index, line in enumerate(lines): + spans = AnthropicClient._issued_spans_in(line, preserved_values) + if not spans: + continue + stripped = AnthropicClient._substitute_issued(line, dict.fromkeys( + (token for _, _, token in spans), "", + )) + if len(spans) == 1 and AnthropicClient._COMMENT_ONLY.fullmatch(stripped): + lines[index] = preserved_values[spans[0][2]] + else: + lines[index] = AnthropicClient._substitute_issued(line, preserved_values) + return "\n".join(lines) + + #: The tail a mis-tokenised match leaves behind. Not the full prefix — that + #: is what a clean unresolved token looks like — but the fragment that + #: survives when a match ate another token's leading underscores. + _TOKEN_DEBRIS = re.compile(r"(? 1) + if duplicated: + msg = f"{len(duplicated)} preserved job body/bodies restored into more than one step" + logger.warning(f"{msg}: {', '.join(duplicated)}") + sentry_sdk.capture_message(msg, level="warning") + + unclaimed = sorted( + token + for token in preserved_values + if token.startswith(CODE_PLACEHOLDER_PREFIX) and token not in claims + ) + if unclaimed: + # Log only, deliberately. Deleting a step is an ordinary edit and + # leaves its preserved body unclaimed every time, so capturing this + # to Sentry would fire on routine use — and each event carries the + # request context with it. + logger.info( + f"{len(unclaimed)} preserved job body/bodies were never restored " + f"(ordinary when a step was deleted): {', '.join(unclaimed)}", + ) + + @staticmethod + def _reference_key(value): + """A mapping key that distinguishes `1`, `"1"` and `True`. + + YAML types keys: `1:` is an int, `"1":` a string, `on:` a boolean. + Keying a mapping on `str(value)` makes the first two collide; keying it + on the raw value makes the last two collide, because `hash(True) == + hash(1)`. Pairing the type name with the text avoids both. + """ + return (type(value).__name__, str(value)) + + @staticmethod + def _section(yaml_data, name): + """Return `yaml_data[name]` as a dict of dicts, or {} if it is anything else. + + `jobs:` with nothing under it parses as None, and a single bare entry + (`b:`) gives a None value. Both are valid YAML and both used to raise + somewhere in this pipeline, where the exception was swallowed and the + user got prose and no workflow. + """ + if not isinstance(yaml_data, dict): + return {} + section = yaml_data.get(name) + if not isinstance(section, dict): + return {} + for key, value in list(section.items()): + if not isinstance(value, dict): + section[key] = {} + return section + + #: Stand-in for a reference that sanitizes away to nothing. Uniquified + #: against the workflow's own keys at sanitize time, because a user can + #: perfectly well name a step "unresolved-step" — keys are uniquified + #: against each other, not against this. An edge carrying the sentinel + #: stays visibly broken rather than silently binding to a real step. + UNRESOLVED_REFERENCE = "unresolved-step" + + #: Key for an edge whose own key sanitizes away to nothing and which has no + #: endpoints to derive a label from. + UNNAMED_EDGE = "edge" + + @staticmethod + def _unique_name(candidate: str, taken: set, fallback: str) -> str: + """Return `candidate` (or `fallback` if it sanitized away) made unique against `taken`. + + Job names and job keys must both be unique within a workflow. Two names + that differ only in characters the rule strips — `Résumé` and `Resume` + under the ASCII rule — would otherwise collapse onto each other and the + second job would overwrite the first. + + The suffix is added inside the length cap, not on top of it: appending + `-2` to a name that is already at the limit would push it over, and + Lightning would reject the result. + """ + candidate = candidate or fallback + if candidate not in taken: + taken.add(candidate) + return candidate + + suffix = 2 + while True: + tail = f"-{suffix}" + trimmed = truncate_graphemes(candidate, MAX_NAME_LENGTH - grapheme_length(tail)) + unique = f"{trimmed}{tail}" + if unique not in taken: + taken.add(unique) + return unique + suffix += 1 + + @staticmethod + def _edge_label(edge_key: str, edge_data: object, remap_reference: object) -> str: + """Derive an edge's `source->target` label after its endpoints were renamed. + + The label comes from the edge's own `source_*`/`target_*` fields + wherever it has them, rather than from splitting the old label on "->". + Under the permissive rule "->" is a legal run of characters inside a + step name, which makes that split ambiguous — and the fields are the + real identity anyway; the key is only a label. + + An edge whose endpoints are both known always gets the derived label, + whatever its old key looked like. Deriving it for `a->b` but leaving + `e1` alone would mean the sanitizer emits keys its own test assertion + rejects. Only an edge with no usable endpoints keeps its old key, and + then it is split on the first "->" if it has one. + """ + # YAML gives an unquoted `on:` as the boolean True, not a string. + edge_key = str(edge_key) + + if isinstance(edge_data, dict): + source = edge_data.get("source_job") or edge_data.get("source_trigger") + target = edge_data.get("target_job") or edge_data.get("target_trigger") + if source and target: + return f"{source}->{target}" + + if "->" not in edge_key: + # Nothing to derive a label from and no arrow to split on. Still + # sanitize it — a key carrying a NUL crashes the insert on + # Lightning's side just as surely as a name would. + return sanitize_name(edge_key, unicode_names_enabled()) or AnthropicClient.UNNAMED_EDGE + + source_part, target_part = edge_key.split("->", 1) + return f"{remap_reference(source_part)}->{remap_reference(target_part)}" + + @staticmethod + def sanitize_job_names(yaml_data: object) -> None: + """ + Bring every job key, job name, trigger key and edge reference in the + workflow into line with the active step-name rule (see `name_rules`). + + Keys are rewritten alongside names, and edges are rewritten through the + resulting key mapping rather than sanitized independently. Sanitizing + the two separately is how edges used to end up pointing at jobs that no + longer existed. + """ + if not isinstance(yaml_data, dict): + # A non-dict payload is not a workflow. One caller swallows every + # exception from this, so raising here would silently drop YAML. return - - def sanitize_single_name(name): - if not name or not isinstance(name, str): - return name - # Normalize unicode characters (removes diacritics) - normalized = unicodedata.normalize('NFKD', name) - ascii_name = normalized.encode('ascii', 'ignore').decode('ascii') - # Keep only alphanumeric, spaces, hyphens, and underscores - return re.sub(r'[^a-zA-Z0-9\s\-_]', '', ascii_name) - - if "jobs" in yaml_data: - jobs = yaml_data["jobs"] - name_mapping = {} - + + unicode_mode = unicode_names_enabled() + + # One mapping per section, never shared. A workflow may legitimately + # have a trigger and a job whose original keys are the same string, and + # a single mapping keyed on that string would let the jobs pass + # overwrite the trigger's entry — rewriting the edge's source_trigger + # to point at a job and orphaning the trigger. + key_mappings = {"jobs": {}, "triggers": {}} + + def sanitize_section(section: str, fallback_prefix: str, label: str) -> dict | None: + """Sanitize the keys of `jobs:` or `triggers:`, recording the renames.""" + entries = yaml_data.get(section) + if not isinstance(entries, dict): + return None + + mapping = key_mappings[section] + taken = set() + rebuilt = {} + renamed = [] + for index, (key, data) in enumerate(entries.items()): + original = str(key) + new_key = AnthropicClient._unique_name( + sanitize_name(original, unicode_mode), taken, f"{fallback_prefix}-{index + 1}", + ) + # Keyed on type *and* text; see `_reference_key`. + mapping[AnthropicClient._reference_key(key)] = new_key + rebuilt[new_key] = data + if original != new_key: + renamed.append(f"{original!r} -> {new_key!r}") + + if renamed: + logger.info(f"Sanitized {len(renamed)} {label} key(s): {', '.join(renamed)}") + + yaml_data[section] = rebuilt + return rebuilt + + triggers = sanitize_section("triggers", "trigger", "trigger") + jobs = sanitize_section("jobs", "step", "job") + + # Captured before the renaming loop below, so a by-name reference is + # matched against what the model actually wrote. + # `str(...)`, not a string-only filter. A model writing `name: 2024`, + # `name: on` or `name: 01` is ordinary output, and YAML hands those over + # as an int, a bool and an int. Filtering them out left them unsanitized + # and unrenamed, and Ecto rejects a `:string` cast from an integer, so a + # workflow that used to save stopped saving. The `None` case the filter + # was written for is handled by excluding None explicitly. + original_job_names = { + job_key: str(job_data["name"]) + for job_key, job_data in (jobs or {}).items() + if isinstance(job_data, dict) + and job_data.get("name") is not None + and str(job_data["name"]).strip() + } + + taken_names = set() + if jobs: + renamed = [] for job_key, job_data in jobs.items(): - if "name" in job_data: + if isinstance(job_data, dict) and job_data.get("name") is not None: original_name = str(job_data["name"]) - sanitized_name = sanitize_single_name(original_name) - - job_data["name"] = sanitized_name - name_mapping[original_name] = sanitized_name - - if original_name != sanitized_name: - logger.info(f"Sanitized job name: '{original_name}' -> '{sanitized_name}'") - - if "edges" in yaml_data: + new_name = AnthropicClient._unique_name( + sanitize_name(original_name, unicode_mode), taken_names, job_key, + ) + job_data["name"] = new_name + if original_name != new_name: + renamed.append(f"{original_name!r} -> {new_name!r}") + + if renamed: + logger.info(f"Sanitized {len(renamed)} job name(s): {', '.join(renamed)}") + + # The sentinel must not collide with anything a user can type — keys or + # names. Names count because a dangling edge is reported by name, and a + # reader matching it against the step list would be misled. + unresolved = AnthropicClient._unique_name( + AnthropicClient.UNRESOLVED_REFERENCE, + set(jobs or {}) | set(triggers or {}) | taken_names, + AnthropicClient.UNRESOLVED_REFERENCE, + ) + + def remap_reference(reference: object, section: str | None = None) -> str: + """Map a reference through the mapping for `section` (or either, for a key part). + + `source_job` resolves against jobs and `source_trigger` against + triggers. An edge *key* part has no field to say which it is, so it + tries jobs first and then triggers. + """ + sections = (section,) if section else ("jobs", "triggers") + for name in sections: + new_key = key_mappings[name].get(AnthropicClient._reference_key(reference)) + if new_key is not None: + return new_key + + # Not a key. The likeliest model mistake is referring to a step by + # its *name* instead of its key, which otherwise ships as a + # well-formed edge pointing at nothing. + if "jobs" in sections and reference is not None: + by_name = AnthropicClient._resolve_by_name(str(reference), original_job_names) + if by_name is not None: + logger.info( + f"Edge referred to step by name {str(reference)!r}; " + f"resolved to job key {by_name!r}", + ) + return by_name + + # Genuinely unresolvable. Sanitize it so it at least obeys the rule; + # if nothing survives, say so rather than leaking the raw value out. + resolved = sanitize_name(str(reference), unicode_mode) or unresolved + + # If the sanitized form is a real key, that is usually the right + # answer and not a coincidence: a key with a trailing space, a tab, + # a NUL, or one written in a different normal form all sanitize to + # exactly what the model wrote. Only treat it as a collision when + # the *original* key it belongs to was something else entirely. + owner = _sanitized_key_owner(resolved, sections) + if owner is not None: + if _is_the_same_reference(owner, reference): + return resolved + logger.warning( + f"Unresolvable reference {str(reference)!r} sanitizes to {resolved!r}, " + f"which belongs to a different step ({owner!r}); using the unresolved " + f"marker rather than binding to it", + ) + resolved = unresolved + + dangling.add(resolved) + return resolved + + def _sanitized_key_owner(resolved: str, sections: tuple) -> object: + """Return the original key that `resolved` is the sanitized form of.""" + for name in sections: + for original, new_key in key_mappings[name].items(): + if new_key == resolved: + return original[1] + return None + + def _is_the_same_reference(original_key: str, reference: object) -> bool: + """True if `original_key` and `reference` are the same name written differently. + + Whitespace, control characters and normal form are all differences a + reader would not see. A genuinely different name is not. + + The length cap is *not* one of them: `normalize_for_lookup` does not + truncate, so a 150-character key sanitizes to a 100-character one + that a 100-character reference matches, and this returns False — + the edge goes to the sentinel. That is a real gap, and it is here + rather than hidden because the fix belongs in whichever of the two + should stop caring about length. + """ + return normalize_for_lookup(original_key) == normalize_for_lookup(str(reference)) + + dangling = set() + + edges = yaml_data.get("edges") + if isinstance(edges, dict): sanitized_edges = {} - - for edge_key, edge_data in yaml_data["edges"].items(): - if "source_job" in edge_data: - original_source = str(edge_data["source_job"]) - edge_data["source_job"] = sanitize_single_name(original_source) - if original_source != edge_data["source_job"]: - logger.info(f"Sanitized edge source_job: '{original_source}' -> '{edge_data['source_job']}'") - - if "target_job" in edge_data: - original_target = str(edge_data["target_job"]) - edge_data["target_job"] = sanitize_single_name(original_target) - if original_target != edge_data["target_job"]: - logger.info(f"Sanitized edge target_job: '{original_target}' -> '{edge_data['target_job']}'") - - if "->" in edge_key: - source_part, target_part = edge_key.split("->", 1) - sanitized_source = sanitize_single_name(source_part) - sanitized_target = sanitize_single_name(target_part) - sanitized_edge_key = f"{sanitized_source}->{sanitized_target}" - - if sanitized_edge_key != edge_key: - logger.info(f"Sanitized edge key: '{edge_key}' -> '{sanitized_edge_key}'") - - sanitized_edges[sanitized_edge_key] = edge_data - else: - # If there's no arrow, just keep the original key - sanitized_edges[edge_key] = edge_data - + + remapped_fields = 0 + + for edge_key, edge_data in edges.items(): + if isinstance(edge_data, dict): + for field, section in ( + ("source_job", "jobs"), + ("target_job", "jobs"), + ("source_trigger", "triggers"), + ("target_trigger", "triggers"), + ): + if field in edge_data: + original_reference = edge_data[field] + edge_data[field] = remap_reference(original_reference, section) + if str(original_reference) != edge_data[field]: + remapped_fields += 1 + + label = AnthropicClient._edge_label(edge_key, edge_data, remap_reference) + + # The label is not unique on its own: two edges may join the + # same pair of steps (an on_success and an on_failure edge is an + # ordinary workflow). Keying on the bare label would drop one of + # them, so disambiguate instead of overwriting, with the suffix + # *inside* the cap, the same rule `_unique_name` follows. + sanitized_edge_key = truncate_graphemes(label, MAX_EDGE_KEY_LENGTH) + suffix = 2 + while sanitized_edge_key in sanitized_edges: + tail = f"-{suffix}" + trimmed = truncate_graphemes(label, MAX_EDGE_KEY_LENGTH - grapheme_length(tail)) + sanitized_edge_key = f"{trimmed}{tail}" + suffix += 1 + + sanitized_edges[sanitized_edge_key] = edge_data + + if remapped_fields: + logger.info(f"Remapped {remapped_fields} edge endpoint reference(s)") + + if len(sanitized_edges) != len(edges): # pragma: no cover - defensive + logger.error( + f"Edge count changed while sanitizing: {len(edges)} in, {len(sanitized_edges)} out", + ) + yaml_data["edges"] = sanitized_edges + if dangling: + # A well-formed edge pointing at nothing looks fine to every + # character check, so it used to ship in silence. It is still + # emitted — dropping the edge would lose more than it saves — but + # it is no longer invisible. Only the count goes to Sentry; the + # names are the caller's own, so they go to the log. + logger.warning( + f"Workflow has edge endpoints that match no step or trigger: " + f"{', '.join(sorted(dangling))}", + ) + sentry_sdk.capture_message( + f"Workflow has {len(dangling)} edge endpoint(s) matching no step or trigger", + level="warning", + ) + def finalize_yaml(self, parsed_yaml, preserved_values=None): """ Apply the full post-processing pipeline to a parsed workflow dict and @@ -472,7 +1002,31 @@ def finalize_yaml(self, parsed_yaml, preserved_values=None): self.sanitize_job_names(parsed_yaml) with sentry_sdk.start_span(description="restore_components"): self.restore_components(parsed_yaml, preserved_values) - return yaml.dump(parsed_yaml, sort_keys=False) + + dumped = yaml.dump(parsed_yaml, sort_keys=False, allow_unicode=True) + + # There is deliberately no remediation pass here. `restore_components` + # already degrades every unresolvable placeholder, so anything reaching + # this point in a body is *restored code* — and "contains the prefix + # anywhere" then means real code that happens to mention the token. + # That is reachable: `gen_project_prompts.yaml` shows the model the + # literal `__CODE_BLOCK_jobname__`, so a model quoting it back in a + # comment would have had that step's body replaced with the empty + # marker. The pass had no true-positive path and one way to destroy + # code, so it is gone. The id check below looks at ids only, not at + # bodies, for the same reason. + stray_ids = [ + value + for holder in iter_id_holders(parsed_yaml) + for value in (holder.get("id"),) + if isinstance(value, str) and value.startswith("__ID_") + ] + if stray_ids: + msg = f"{len(stray_ids)} id placeholder(s) survived finalize_yaml" + logger.error(msg) + sentry_sdk.capture_message(msg, level="error") + + return dumped def split_format_yaml(self, response, preserved_values=None, stream_manager=None): """Split text and YAML in response and format the YAML.""" @@ -574,36 +1128,58 @@ def extract_and_preserve_components(yaml_data): preserved_values = {} - if "jobs" in yaml_data: - for job_key, job_data in yaml_data["jobs"].items(): - if "body" in job_data: - body_content = job_data["body"].strip() - if body_content and body_content != "// Add operations here": - placeholder = f"__CODE_BLOCK_{job_key}__" - preserved_values[placeholder] = body_content - job_data["body"] = placeholder + for job_key, job_data in AnthropicClient._section(yaml_data, "jobs").items(): + if isinstance(job_data.get("body"), str): + body_content = job_data["body"].strip() + if body_content and body_content != "// Add operations here": + placeholder = f"{CODE_PLACEHOLDER_PREFIX}{job_key}__" + preserved_values[placeholder] = body_content + job_data["body"] = placeholder - if "id" in job_data: - placeholder = f"__ID_JOB_{job_key}__" - preserved_values[placeholder] = job_data["id"] - job_data["id"] = placeholder - - if "triggers" in yaml_data: - for trigger_key, trigger_data in yaml_data["triggers"].items(): - if "id" in trigger_data: - # Store the trigger ID directly without placeholder - preserved_values["trigger_id"] = trigger_data["id"] - # Remove the id key from what we send to the model - del trigger_data["id"] + if "id" in job_data: + placeholder = f"__ID_JOB_{job_key}__" + preserved_values[placeholder] = job_data["id"] + job_data["id"] = placeholder - if "edges" in yaml_data: - for edge_key, edge_data in yaml_data["edges"].items(): - if "id" in edge_data: - placeholder = f"__ID_EDGE_{edge_key}__" - preserved_values[placeholder] = edge_data["id"] - edge_data["id"] = placeholder - - return preserved_values, yaml.dump(yaml_data, sort_keys=False) + for trigger_data in AnthropicClient._section(yaml_data, "triggers").values(): + if "id" in trigger_data: + # Store the trigger ID directly without placeholder + preserved_values["trigger_id"] = trigger_data["id"] + # Remove the id key from what we send to the model + del trigger_data["id"] + + for edge_key, edge_data in AnthropicClient._section(yaml_data, "edges").items(): + if "id" in edge_data: + placeholder = f"__ID_EDGE_{edge_key}__" + preserved_values[placeholder] = edge_data["id"] + edge_data["id"] = placeholder + + # Backstop, through the same walker the redactor uses. The loop above + # only sees a top-level `jobs:`; a Lightning project export nests them + # under `workflows: -> : -> jobs:`, so nothing matched, nothing + # was swapped, and the dump below put every body into the system + # prompt. Anything the structured pass missed gets a placeholder here. + for index, holder in enumerate(iter_body_holders(yaml_data)): + if not has_unredacted_body({BODY_KEY: holder[BODY_KEY]}): + continue + # Both loops key into one namespace, and the structured pass above + # uses the job key verbatim — `__CODE_BLOCK___` is the form the + # prompt documents to the model, so it cannot change. Bump until the + # token is free instead: a job keyed `nested_1` would otherwise + # collide with backstop index 1 and carry another step's code. + suffix = index + placeholder = f"{CODE_PLACEHOLDER_PREFIX}nested_{suffix}__" + while placeholder in preserved_values: + suffix += 1 + placeholder = f"{CODE_PLACEHOLDER_PREFIX}nested_{suffix}__" + preserved_values[placeholder] = holder[BODY_KEY] + holder[BODY_KEY] = placeholder + + if has_unredacted_body(yaml_data): # pragma: no cover - defensive + logger.error("A job body survived component extraction; withholding the workflow") + return preserved_values, WITHHELD_NOTICE + + return preserved_values, yaml.dump(yaml_data, sort_keys=False, allow_unicode=True) def restore_components(self, yaml_data, preserved_values=None): """ @@ -613,54 +1189,125 @@ def restore_components(self, yaml_data, preserved_values=None): return preserved_values = preserved_values or {} - - if "jobs" in yaml_data: - for job_key, job_data in yaml_data["jobs"].items(): - if "body" in job_data: - current_body = job_data["body"] - if isinstance(current_body, str) and current_body in preserved_values: - job_data["body"] = preserved_values[current_body] - else: - job_data["body"] = "// Add operations here" - else: - job_data["body"] = "// Add operations here" + + # Bodies are restored through the same walker that swapped them. The + # swap walks the whole tree and this used to walk only a top-level + # `jobs:`, so a nested document went out to the user with + # `body: __CODE_BLOCK_nested_0__` where their code had been — the + # prompt was correctly redacted and the workflow was destroyed. + claims: dict[str, int] = {} + + for holder in iter_body_holders(yaml_data): + current_body = holder[BODY_KEY] + # Strip first. The model writing a placeholder back as a block + # scalar — the natural style for a `body:` — parses as + # `'__CODE_BLOCK_a__\n'`, which matched nothing, so the token + # shipped to the user in place of their code. + lookup = current_body.strip() if isinstance(current_body, str) else current_body + if isinstance(lookup, str) and lookup in preserved_values: + holder[BODY_KEY] = preserved_values[lookup] + claims[lookup] = claims.get(lookup, 0) + 1 + continue + + if not isinstance(current_body, str): + # A list or mapping body, which the `jobs:` pass below does + # not reach. + if CODE_PLACEHOLDER_PREFIX in str(current_body): + msg = "A non-string job body carries a code placeholder; replacing it" + logger.warning(msg) + sentry_sdk.capture_message(msg, level="warning") + holder[BODY_KEY] = "// Add operations here" + continue + + issued = AnthropicClient._issued_tokens_in(current_body, preserved_values) + if not issued and CODE_PLACEHOLDER_PREFIX not in current_body: + continue + + only_placeholders = AnthropicClient._is_only_placeholders( + current_body, preserved_values, + ) + + if issued and only_placeholders: + # The body is our token(s) and decoration, nothing else. Join + # them in the order written: "merge step a and step b" produces + # exactly `__CODE_BLOCK_a__\n__CODE_BLOCK_b__`, and returning + # only one of them, or neither, loses a body we are holding. + holder[BODY_KEY] = "\n".join(preserved_values[token] for token in issued) + for token in issued: + claims[token] = claims.get(token, 0) + 1 + logger.warning(f"Recovered {len(issued)} decorated code placeholder(s)") + + elif issued: + # A token embedded in other content. Substitute in place rather + # than replacing the whole body: a 500-line body whose last line + # is a comment naming the token used to collapse to the one + # statement the token stood for, which reads as working code. + holder[BODY_KEY] = AnthropicClient._substitute_issued_by_line( + current_body, preserved_values, + ) + for token in dict.fromkeys(issued): + claims[token] = claims.get(token, 0) + 1 + logger.warning( + f"Substituted {len(set(issued))} code placeholder(s) embedded in other content", + ) + + elif only_placeholders: + # Token-shaped and nothing else, but not one we issued — a stale + # token from an earlier turn. Nothing to restore it from, and + # shipping it puts a swap token in front of the user as if it + # were their code. + msg = "Unresolvable code placeholder in a job body, replacing with the empty-job marker" + logger.warning(msg) + sentry_sdk.capture_message(msg, level="warning") + holder[BODY_KEY] = "// Add operations here" + + else: + # Token-shaped text inside real code. `gen_project_prompts.yaml` + # shows the model the literal `__CODE_BLOCK_jobname__`, so a + # model quoting it back is ordinary output and must survive. + logger.info("A job body mentions a placeholder-shaped string; leaving it as written") + + AnthropicClient._report_claims(claims, preserved_values) + AnthropicClient._report_token_debris(yaml_data) + + for job_data in self._section(yaml_data, "jobs").values(): + if not isinstance(job_data.get("body"), str) or not job_data["body"].strip(): + job_data["body"] = "// Add operations here" - if "id" in job_data: - current_id = job_data["id"] + if "id" in job_data: + current_id = job_data["id"] - if isinstance(current_id, str) and current_id in preserved_values: - job_data["id"] = preserved_values[current_id] - elif isinstance(current_id, str) and current_id.startswith("__ID_") and current_id.endswith("__"): - msg = f"Unknown placeholder {current_id}, generating new ID" - logger.warning(msg) - sentry_sdk.capture_message(msg, level="warning") - job_data["id"] = str(uuid.uuid4()) - else: + if isinstance(current_id, str) and current_id in preserved_values: + job_data["id"] = preserved_values[current_id] + elif isinstance(current_id, str) and current_id.startswith("__ID_") and current_id.endswith("__"): + msg = f"Unknown placeholder {current_id}, generating new ID" + logger.warning(msg) + sentry_sdk.capture_message(msg, level="warning") job_data["id"] = str(uuid.uuid4()) + else: + job_data["id"] = str(uuid.uuid4()) - if "triggers" in yaml_data: - for trigger_key, trigger_data in yaml_data["triggers"].items(): - if "trigger_id" in preserved_values: - # Directly restore the preserved trigger ID - trigger_data["id"] = preserved_values["trigger_id"] - elif "id" not in trigger_data: - # Generate new ID if no preserved ID exists - trigger_data["id"] = str(uuid.uuid4()) - - if "edges" in yaml_data: - for edge_key, edge_data in yaml_data["edges"].items(): - if "id" in edge_data: - current_id = edge_data["id"] + for trigger_data in self._section(yaml_data, "triggers").values(): + if "trigger_id" in preserved_values: + # Directly restore the preserved trigger ID + trigger_data["id"] = preserved_values["trigger_id"] + elif "id" not in trigger_data: + # Generate new ID if no preserved ID exists + trigger_data["id"] = str(uuid.uuid4()) + + for edge_data in self._section(yaml_data, "edges").values(): + if "id" in edge_data: + current_id = edge_data["id"] - if isinstance(current_id, str) and current_id in preserved_values: - edge_data["id"] = preserved_values[current_id] - elif isinstance(current_id, str) and current_id.startswith("__ID_") and current_id.endswith("__"): - msg = f"Unknown placeholder {current_id}, generating new ID" - logger.warning(msg) - sentry_sdk.capture_message(msg, level="warning") - edge_data["id"] = str(uuid.uuid4()) - else: + if isinstance(current_id, str) and current_id in preserved_values: + edge_data["id"] = preserved_values[current_id] + elif isinstance(current_id, str) and current_id.startswith("__ID_") and current_id.endswith("__"): + msg = f"Unknown placeholder {current_id}, generating new ID" + logger.warning(msg) + sentry_sdk.capture_message(msg, level="warning") edge_data["id"] = str(uuid.uuid4()) + else: + edge_data["id"] = str(uuid.uuid4()) def process_stream_event(self, event, accumulated_response, text_started, sent_length, stream_manager, preserved_values=None): """ @@ -779,7 +1426,7 @@ def main(data_dict: dict) -> dict: page_name = data.context.get("page_name") current_page = { "type": "workflow", - "name": page_name + "name": page_name, } config = ChatConfig(api_key=data.api_key) if data.api_key else None @@ -826,7 +1473,7 @@ def main(data_dict: dict) -> dict: "response_yaml": result.content_yaml, "history": result.history, "usage": result.usage, - "meta": {"apollo_version": APOLLO_VERSION} + "meta": {"apollo_version": APOLLO_VERSION}, } if result.handover: @@ -851,7 +1498,7 @@ def main(data_dict: dict) -> dict: raise ApolloError(401, "Authentication failed", type="AUTH_ERROR") except RateLimitError as e: raise ApolloError( - 429, "Rate limit exceeded, please try again later", type="RATE_LIMIT", details={"retry_after": 60} + 429, "Rate limit exceeded, please try again later", type="RATE_LIMIT", details={"retry_after": 60}, ) except BadRequestError as e: # Not `str(e)`: Anthropic echoes the offending request, which is the prompt. diff --git a/services/yaml_utils.py b/services/yaml_utils.py index 940cd530..e623f170 100644 --- a/services/yaml_utils.py +++ b/services/yaml_utils.py @@ -4,10 +4,10 @@ Used by global_chat (router, planner, subagent caller) and by job_chat in subagent mode for job extraction, code stitching, and step inspection. """ -import re from collections.abc import Iterator import yaml +from name_rules import normalize_for_lookup from util import create_logger logger = create_logger("yaml_utils") @@ -23,20 +23,24 @@ def get_page_view(page: str | None) -> tuple[str | None, str | None]: workflows/ -> ("overview", None) workflow canvas settings / absent / anything else -> (None, None) - Because a name may itself contain "/", the returned step name is a - best-effort candidate — the caller must validate it against the workflow - YAML rather than trust it. + A step name may itself contain "/", so everything after the workflow + segment is taken as the step name rather than just the third segment — + otherwise "workflows/wf/Import A/B" loses the step focus entirely. The + split between workflow and step is still a guess when the *workflow* name + contains a "/", so the returned step name is a best-effort candidate: the + caller must validate it against the workflow YAML rather than trust it. """ if not page: return None, None parts = page.strip("/").split("/") - if parts[0] != "workflows": + if parts[0] != "workflows" or len(parts) < 2: return None, None if len(parts) == 2: return "overview", None - if len(parts) == 3 and parts[2] != "settings": - return "step", parts[2] - return None, None + step = "/".join(parts[2:]) + if step == "settings": + return None, None + return "step", step def get_step_name_from_page(page: str | None) -> str | None: @@ -54,43 +58,80 @@ def get_step_name_from_page(page: str | None) -> str | None: def normalize_name(name: str) -> str: - """Normalize a name for fuzzy matching: lowercase, non-alphanumeric chars become hyphens.""" - return re.sub(r'[^a-z0-9]', '-', name.lower()).strip('-') + """Normalize a name for fuzzy matching: lowercase, non-alphanumeric chars become hyphens. + + Unicode-aware — see ``name_rules.normalize_for_lookup``. "Alphanumeric" + means a letter, mark or digit in any script, so a non-Latin name folds to + itself rather than to the empty string. + """ + return normalize_for_lookup(name) def find_job_in_yaml(yaml_str: str, step_name: str) -> tuple[str | None, dict | None]: """ Find a job in the workflow YAML by step name. - Tries direct key match first, then normalized name comparison against - both the job key and the job's name field. + Resolution order, strictest first: an exact key, an exact name, then the + normalized fold — and the fold resolves only when it picks out exactly one + job. Anything ambiguous returns (None, None). + + The order matters because the result is *written* to: `router` and + `planner` hand the key straight to `stitch_job_code`, which replaces that + step's body. Taking the first fold hit meant an earlier job's *key* fold + could beat a later job's *exact name* — steps keyed `upload-data` + ("Legacy uploader") and `upload-data-2` ("Upload Data"), a lookup for + "Upload Data", and the model's generated code landed on the legacy step. + A miss costs a retry; a wrong hit destroys work. Returns: - (job_key, job_data) or (None, None) if not found or on parse error + (job_key, job_data) or (None, None) if not found, ambiguous, or on + parse error """ try: yaml_data = yaml.safe_load(yaml_str) except Exception: return None, None - if not yaml_data or "jobs" not in yaml_data: + if not isinstance(yaml_data, dict) or not isinstance(yaml_data.get("jobs"), dict): return None, None jobs = yaml_data["jobs"] - # Direct key match if step_name in jobs: return step_name, jobs[step_name] - # Normalized match: compare against job key and name field + exact_names = [ + key for key, data in jobs.items() if (data or {}).get("name") == step_name + ] + if exact_names: + return _only_match(exact_names, jobs, step_name, "name") + + # An empty normalization carries no information (the name was all + # punctuation), so never match on it. normalized_step = normalize_name(step_name) - for job_key, job_data in jobs.items(): - if normalize_name(job_key) == normalized_step: - return job_key, job_data - job_name = job_data.get("name", "") - if normalize_name(job_name) == normalized_step: - return job_key, job_data + if not normalized_step: + return None, None + folded = [ + key + for key, data in jobs.items() + if normalize_name(key) == normalized_step + or ((data or {}).get("name") and normalize_name(data["name"]) == normalized_step) + ] + return _only_match(folded, jobs, step_name, "folded name") + + +def _only_match( + matches: list, jobs: dict, step_name: str, how: str, +) -> tuple[str | None, dict | None]: + """Return the single match, or nothing when more than one job qualifies.""" + if len(matches) == 1: + return matches[0], jobs[matches[0]] + logger.warning( + f"Step reference {step_name!r} matches the {how} of {len(matches)} jobs " + f"({', '.join(sorted(str(match) for match in matches))}); leaving it " + f"unresolved rather than guessing, because the caller writes to it", + ) return None, None @@ -109,7 +150,7 @@ def workflow_has_job_code(yaml_str: str | None) -> bool: yaml_data = yaml.safe_load(yaml_str) except Exception: return False - if not yaml_data or "jobs" not in yaml_data: + if not isinstance(yaml_data, dict) or not isinstance(yaml_data.get("jobs"), dict): return False for job_data in yaml_data["jobs"].values(): body = (job_data or {}).get("body") @@ -316,11 +357,17 @@ def stitch_job_code(yaml_str: str, job_key: str, new_code: str) -> str: """ try: yaml_data = yaml.safe_load(yaml_str) - if yaml_data and "jobs" in yaml_data and job_key in yaml_data["jobs"]: - yaml_data["jobs"][job_key]["body"] = new_code - return yaml.dump(yaml_data, sort_keys=False) - except Exception: - pass + jobs = yaml_data.get("jobs") if isinstance(yaml_data, dict) else None + if isinstance(jobs, dict) and isinstance(jobs.get(job_key), dict): + jobs[job_key]["body"] = new_code + return yaml.dump(yaml_data, sort_keys=False, allow_unicode=True) + logger.error( + f"Could not stitch job code: no job keyed '{job_key}' in the workflow. " + f"The generated code has been discarded.", + ) + except Exception as error: + # Not `logger.exception`: a PyYAML error mark carries document text. + logger.error(f"Could not stitch job code into the workflow YAML ({type(error).__name__})") return yaml_str diff --git a/tools/unicode_parity/.gitignore b/tools/unicode_parity/.gitignore new file mode 100644 index 00000000..89f9ac04 --- /dev/null +++ b/tools/unicode_parity/.gitignore @@ -0,0 +1 @@ +out/ diff --git a/tools/unicode_parity/check.py b/tools/unicode_parity/check.py new file mode 100644 index 00000000..9896a388 --- /dev/null +++ b/tools/unicode_parity/check.py @@ -0,0 +1,204 @@ +"""Compare services/name_rules.py against the Elixir ground truth. + +Run `elixir probe.exs` first (see its header). This script does two things: + + * reports every disagreement between Apollo's clustering and Elixir's, in + both the per-codepoint classification and the derived tables; + * prints the table literals to paste back into `name_rules` when a Unicode + version has moved. + +Exit status is non-zero if anything disagrees, so it can be wired into CI on a +runner that has Elixir. + +Usage, from this directory: + + elixir probe.exs + python3 check.py # report + python3 check.py --tables # also print the table literals +""" + +# This is a developer CLI: printing is its output, and it is all about raw +# codepoint values, so the "magic value" and "no print" rules do not apply. +# ruff: noqa: T201, PLR2004 + +from __future__ import annotations + +import sys +import unicodedata +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[2] / "services")) + +import name_rules as nr + +OUT = Path(__file__).parent / "out" + + +def _codepoints(path: Path) -> set[int]: + return {int(line.split()[0], 16) for line in path.read_text().split("\n") if line.strip()} + + +def _ranges(codes: set[int]) -> list[tuple[int, int]]: + out: list[list[int]] = [] + for code in sorted(codes): + if out and code == out[-1][1] + 1: + out[-1][1] = code + else: + out.append([code, code]) + return [(a, b) for a, b in out] + + +def _literal(name: str, ranges: list[tuple[int, int]], per_line: int = 3) -> str: + rows = [ + " " + " ".join(f"(0x{a:04X}, 0x{b:04X})," for a, b in ranges[i : i + per_line]) + for i in range(0, len(ranges), per_line) + ] + return f"{name} = (\n" + "\n".join(rows) + "\n)\n" + + +def check_classes() -> tuple[int, dict[str, list[int]]]: + """Every codepoint must land in the same break-class bucket as Elixir.""" + elixir: dict[int, str] = {} + for line in (OUT / "classmap.txt").read_text().split("\n"): + if line.strip(): + code, bucket = line.split() + elixir[int(code, 16)] = bucket + + buckets = {nr._EXTEND: "A", nr._SPACING: "A", nr._PREP: "P", nr._CONTROL: "C", nr._JOIN: "J"} + wrong: dict[str, list[int]] = {"A": [], "P": [], "C": [], "O": []} + + for code in range(0x110000): + if 0xD800 <= code <= 0xDFFF: + continue + mine = buckets.get(nr._break_class(chr(code)), "O") + if mine == "J": + continue + theirs = elixir.get(code, "O") + if mine != theirs: + wrong[theirs].append(code) + + return sum(len(v) for v in wrong.values()), wrong + + +def check_extpict() -> set[int]: + """Extended_Pictographic is not a break class, so it needs its own check. + + This is the one the codepoint sweep structurally cannot make. An over-broad + set here silently changes GB11 and nothing else notices. + """ + theirs = _codepoints(OUT / "extpict.txt") + mine = {c for c in range(0x110000) if not (0xD800 <= c <= 0xDFFF) and nr._is_ext_pict(c)} + return mine ^ theirs + + +def check_trim() -> set[str]: + theirs = {chr(c) for c in _codepoints(OUT / "trim.txt")} + return theirs ^ set(nr._TRIM_CHARS) + + +def check_lookback() -> set[int]: + """What a GB11 emoji run may be separated from its ZWJ by.""" + theirs = _codepoints(OUT / "lookback.txt") + mine = { + c + for c in range(0x110000) + if not (0xD800 <= c <= 0xDFFF) and nr._break_class(chr(c)) in nr._RUN_CONTINUES + } + return mine ^ theirs + + +def check_clusters() -> tuple[int, int, list]: + """Cluster boundaries against Elixir, not just cluster counts. + + Nothing else here can see the clusterer's rules: the per-codepoint sweep + buckets Hangul and regional indicators to "other", so the GB12/GB13 parity + rule and the CR-LF rule were untested by every check in this file. + """ + path = OUT / "clusters.txt" + mismatches = [] + total = 0 + for line in path.read_text().split("\n"): + if not line.strip(): + continue + total += 1 + raw, expected = line.split("\t") + text = "".join(chr(int(c, 16)) for c in raw.split(",")) + want = [ + "".join(chr(int(c, 16)) for c in group.split("+")) + for group in expected.split(",") + ] + got = nr.grapheme_clusters(text) + if got != want: + mismatches.append((text, want, got)) + return len(mismatches), total, mismatches + + +#: Everything probe.exs writes. Checked up front so a partial or stale run is +#: an error rather than a quiet subset. +EXPECTED_OUTPUTS = ( + "classmap.txt", "extpict.txt", "trim.txt", "lookback.txt", + "clusters.txt", "range_edges.txt", "version.txt", +) + + +def main() -> int: + if not OUT.exists(): + print(f"No probe output at {OUT}. Run `elixir probe.exs` first.") + return 2 + + missing = [name for name in EXPECTED_OUTPUTS if not (OUT / name).exists()] + if missing: + print(f"Probe output incomplete: {', '.join(missing)}. Re-run `elixir probe.exs`.") + return 2 + + stale = [p.name for p in OUT.iterdir() if p.is_file() and p.name not in EXPECTED_OUTPUTS] + if stale: + print(f"Stale files in {OUT}: {', '.join(sorted(stale))}. Delete them; this directory " + f"accumulates and an unread file is a check nobody is running.") + return 2 + + print((OUT / "version.txt").read_text().strip()) + print(f"python unicodedata {unicodedata.unidata_version}\n") + + failures = 0 + + total, wrong = check_classes() + print(f"break classes {'OK' if not total else f'{total} DISAGREEMENTS'}") + failures += total + + for bucket, codes in wrong.items(): + if codes: + print(f" Elixir says {bucket} for {len(codes)} codepoints we call something else") + + for label, diff in ( + ("ExtPict", check_extpict()), + ("trim set", check_trim()), + ("GB11 lookback", check_lookback()), + ): + print(f"{label:18} {'OK' if not diff else f'{len(diff)} DISAGREEMENTS'}") + failures += len(diff) + + bad, total, examples = check_clusters() + print(f"cluster boundaries {'OK' if not bad else f'{bad} of {total} DISAGREE'}") + failures += bad + for text, want, got in examples[:5]: + print(f" in {[hex(ord(c)) for c in text]}") + print(f" otp {want}") + print(f" py {got}") + + if "--tables" in sys.argv: + print("\n# --- paste into services/name_rules.py ---\n") + print(_literal("_EXT_PICT_RANGES", _ranges(_codepoints(OUT / "extpict.txt")))) + print(_literal("_LAG_EXTEND", _ranges(set(wrong["A"])))) + print(_literal("_LAG_CONTROL", _ranges(set(wrong["C"])))) + + if failures: + print( + f"\n{failures} disagreement(s). Re-run with --tables and paste the literals into " + f"name_rules, then re-run the unit suite.", + ) + return 1 if failures else 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tools/unicode_parity/edges.py b/tools/unicode_parity/edges.py new file mode 100644 index 00000000..9902e132 --- /dev/null +++ b/tools/unicode_parity/edges.py @@ -0,0 +1,80 @@ +"""Emit the edge codepoints of every range table in `services/name_rules.py`. + +`probe.exs` used to sweep a hand-picked slice of each range, which never steps +across a boundary: U+1160 HANGUL JUNGSEONG FILLER is assigned and GCB=V, and +narrowing `_HANGUL_V` to start at U+1161 left the harness at exit 0. + +So the probe reads this file rather than naming codepoints itself. Every range +in the tables contributes its first and last member and one either side, which +is where an off-by-one lives. Add a range to `name_rules` and it is swept +automatically; that is the point. + +Run before `probe.exs`: + + python3 edges.py && elixir probe.exs && python3 check.py +""" + +# A developer CLI: printing is its output, and it is all about raw codepoint +# values. +# ruff: noqa: T201, PLR2004 + +from __future__ import annotations + +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[2] / "services")) + +import name_rules as nr + +OUT = Path(__file__).parent / "out" + +#: Every range-shaped table. `_LAG_*` and `_EXT_PICT_RANGES` are tuples of +#: (low, high) pairs; the Hangul ones are tuples of `range` objects. +RANGE_TABLES = { + "_HANGUL_L": nr._HANGUL_L, + "_HANGUL_V": nr._HANGUL_V, + "_HANGUL_T": nr._HANGUL_T, + "_HANGUL_SYLLABLES": (nr._HANGUL_SYLLABLES,), + "_REGIONAL_INDICATOR": (nr._REGIONAL_INDICATOR,), + "_TAGS": (nr._TAGS,), + "_SKIN_TONES": (nr._SKIN_TONES,), + "_C0": (nr._C0,), + "_C1": (nr._C1,), + "_SURROGATES": (nr._SURROGATES,), + "_LAG_EXTEND": nr._LAG_EXTEND, + "_LAG_CONTROL": nr._LAG_CONTROL, + "_EXT_PICT_RANGES": nr._EXT_PICT_RANGES, +} + + +def _bounds(entry: object) -> tuple[int, int]: + if isinstance(entry, range): + return entry.start, entry.stop - 1 + low, high = entry + return low, high + + +def main() -> int: + edges: set[int] = set() + for table in RANGE_TABLES.values(): + for entry in table: + low, high = _bounds(entry) + # The boundary and one step outside it, both ends. Inside-the-range + # values are already covered by the sweeps; it is the step across + # the edge that a hand-picked slice never makes. + edges.update({low - 1, low, high, high + 1}) + + edges = {code for code in edges if 0 <= code <= 0x10FFFF and not 0xD800 <= code <= 0xDFFF} + + OUT.mkdir(exist_ok=True) + (OUT / "range_edges.txt").write_text( + "\n".join(f"{code:X}" for code in sorted(edges)) + "\n", + ) + print(f"wrote out/range_edges.txt: {len(edges)} edge codepoints " + f"from {sum(len(t) for t in RANGE_TABLES.values())} ranges") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tools/unicode_parity/probe.exs b/tools/unicode_parity/probe.exs new file mode 100644 index 00000000..114ad617 --- /dev/null +++ b/tools/unicode_parity/probe.exs @@ -0,0 +1,252 @@ +# Ground truth for services/name_rules.py, generated from the Elixir that +# Lightning actually runs. +# +# Apollo caps step names at 100 graphemes because Ecto's `validate_length` +# counts graphemes, so Apollo's clustering has to agree with Elixir's +# `String.length/1`. Elixir deviates from UAX #29 in two known places, so the +# target is Elixir's behaviour, not the spec, and the only honest way to get it +# is to ask Elixir. +# +# Usage, from this directory, with the Elixir version Lightning runs: +# +# elixir probe.exs # writes the data files below +# python3 check.py # compares, and prints tables to paste back +# +# Outputs (all under ./out): +# classmap.txt every codepoint's grapheme-break bucket +# extpict.txt the Extended_Pictographic set +# trim.txt what String.trim/1 strips +# lookback.txt what a GB11 emoji run may be separated from its ZWJ by +# clusters.txt whole strings, split into graphemes +# version.txt the Elixir and OTP versions these came from +# +# Run `python3 edges.py` FIRST: it writes out/range_edges.txt from the range +# tables in name_rules, and this probe crosses those edges into its shapes. +# +# Re-run whenever Elixir's or Python's Unicode version moves. + +File.mkdir_p!("out") + +zwj = <<0x200D::utf8>> +emoji = <<0x1F600::utf8>> +acute = <<0x301::utf8>> + +codepoints = Enum.reject(0..0x10FFFF, &(&1 >= 0xD800 and &1 <= 0xDFFF)) + +hex = fn cp -> Integer.to_string(cp, 16) end + +# --- 1. break-class buckets --------------------------------------------------- +# A = attaches to what precedes it, P = prepends to what follows, +# C = control (breaks on both sides), O = anything else (not emitted). +bucket = fn cp -> + s = <> + + cond do + String.length("a" <> s) == 1 -> "A" + String.length(s <> "a") == 1 -> "P" + String.length(s <> acute) == 2 -> "C" + true -> "O" + end +end + +classmap = + codepoints + |> Enum.map(&{&1, bucket.(&1)}) + |> Enum.reject(fn {_, b} -> b == "O" end) + |> Enum.map_join("\n", fn {cp, b} -> "#{hex.(cp)} #{b}" end) + +File.write!("out/classmap.txt", classmap <> "\n") + +# --- 2. Extended_Pictographic ------------------------------------------------- +# ExtPict is NOT a break class, so the bucket sweep above cannot see it and an +# over-broad set here is invisible to that check. Probe it through GB11 instead: +# `cp ZWJ emoji` collapses to one grapheme only when cp is pictographic. +lead = Enum.filter(codepoints, &(String.length(<<&1::utf8>> <> zwj <> emoji) == 1)) +follow = Enum.filter(codepoints, &(String.length(emoji <> zwj <> <<&1::utf8>>) == 1)) + +if lead != follow do + IO.puts(:stderr, "WARNING: the two ExtPict probe directions disagree") +end + +File.write!("out/extpict.txt", Enum.map_join(lead, "\n", hex) <> "\n") + +# --- 3. String.trim/1 --------------------------------------------------------- +trimmed = Enum.filter(codepoints, &(String.trim(<<&1::utf8>> <> "x") == "x")) +File.write!("out/trim.txt", Enum.map_join(trimmed, "\n", hex) <> "\n") + +# --- 4. GB11 lookback --------------------------------------------------------- +# Which single intervening character an emoji run survives, between the +# pictograph and the ZWJ. UAX #29 says Extend only; Elixir also allows +# SpacingMark. +lookback = + codepoints + |> Enum.filter(&(String.length(<<0x2764::utf8>> <> <<&1::utf8>> <> zwj <> emoji) == 1)) + |> Enum.map_join("\n", hex) + +File.write!("out/lookback.txt", lookback <> "\n") + +# --- 5. the whole-string corpus ---------------------------------------------- +# Per-codepoint checks cannot see the clusterer's rules: the bucket sweep above +# buckets Hangul and regional indicators to "other", so the GB6-GB8 Hangul +# rules, the GB12/GB13 parity rule and the CR-LF rule are invisible to it. Only +# comparing whole strings, split into graphemes, can catch those. +# +# The shapes are built explicitly rather than sampled from a flat pool. A +# uniform pool is almost all filler — precomposed Hangul and CJK that cluster +# trivially — and the shapes that actually exercise a boundary rule turn up at +# about 1e-6, so a mutant restoring a known bug survives. Each block below is a +# shape that is known to reach a rule, or a shape a name can realistically +# contain. + +bases = [0x41, 0x61, 0x4F, 0x45, 0x55, 0x0995, 0x0B95, 0x0D15, 0x0D9A, 0x0C95, 0x0E01, 0x0915] +marks = [0x0300, 0x0301, 0x0302, 0x0303, 0x0308, 0x030C, 0x0327, 0x0323, 0x0331, 0x0316, 0x0345] +zero_extend = [0x200C, 0x200D, 0xFE00, 0xFE0F, 0x034F, 0x1F3FB, 0x1F3FF, 0xE0067] +two_part_vowels = [ + [0x0995, 0x09C7, 0x09BE], [0x0B95, 0x0BC6, 0x0BBE], [0x0D15, 0x0D46, 0x0D3E], + [0x0D9A, 0x0DD9, 0x0DCF], [0x0C95, 0x0CC6, 0x0CC2], [0x0B15, 0x0B47, 0x0B3E], + [0x11103, 0x11127, 0x1112C] +] +prepends = [0x0600, 0x06DD, 0x0890, 0x0D4E, 0x11A3A, 0x11D46, 0x11F02] + +# Hangul syllables split by whether they carry a trailing consonant: an LV +# syllable decomposes to two jamo and an LVT to three, and the two behave +# differently under a rule that composes onto the cluster lead. +lv_syllables = Enum.take_every(for(s <- 0xAC00..0xD7A3, rem(s - 0xAC00, 28) == 0, do: s), 4) +lvt_syllables = Enum.take_every(for(s <- 0xAC00..0xD7A3, rem(s - 0xAC00, 28) != 0, do: s), 105) + +# What follows the pair. "Nothing" was the only case the corpus had. +# U+11A7 is deliberately included: it sits one below the trailing-consonant +# range, so starting the trailers at U+11A8 never steps across that edge. +trailers = [[], [0x61], [0x0301], [0x11A7], [0x11A8], [0x11FF], [0x1161], [0xAC00], [0x0020, 0x62]] + +two_part_vowel_marks = [0x0CC0, 0x09CB, 0x0BCA, 0x0D4A, 0x0DDC, 0x1B40, 0x0CC7, 0x0D4C] + +# Written by edges.py from the range tables in `services/name_rules.py`, so a +# range added there is swept here without anyone remembering to. +range_edges = + case File.read("out/range_edges.txt") do + {:ok, text} -> + text |> String.split("\n", trim: true) |> Enum.map(&String.to_integer(&1, 16)) + + {:error, _} -> + raise "out/range_edges.txt is missing. Run `python3 edges.py` before probe.exs." + end +regional = [0x1F1E6, 0x1F1EB, 0x1F1F7, 0x1F1FF] +pictographs = [0x00A9, 0x2764, 0x1F469, 0x1F4BB, 0x1F3F4] +ascii_words = [~c"Fetch Data", ~c"step-1", ~c"a_b c", ~c"Verifier letat"] +late_marks = [0x10EFD, 0x11F41, 0x1E08F, 0x1E4EC, 0x1E4EE] + +shapes = + # base + mark + class-zero Extend + mark: the shape OTP and the spec differ + # on, swept exhaustively rather than sampled. + (for b <- bases, m1 <- marks, z <- zero_extend, m2 <- marks, do: [b, m1, z, m2]) ++ + (for b <- bases, z <- zero_extend, m1 <- marks, m2 <- marks, do: [b, z, m1, m2]) ++ + # base + two marks, no separator: canonical ordering with no divergence + (for b <- bases, m1 <- marks, m2 <- marks, do: [b, m1, m2]) ++ + # the two-part vowels, alone and with a mark or a separator after + (for [b, v1, v2] <- two_part_vowels, + tail <- [[], [0x0301], [0x200C], [0x200C, 0x0301]], + do: [b, v1, v2] ++ tail) ++ + # The two halves of a two-part vowel SEPARATED by a class-zero character, + # which is a GB9/GB9a boundary question as well as a normalisation one. + (for [_b, v1, v2] <- two_part_vowels, z <- zero_extend, do: [v1, z, v2]) ++ + (for [b, v1, v2] <- two_part_vowels, z <- zero_extend, do: [b, v1, z, v2]) ++ + (for [b, v1, v2] <- two_part_vowels, z <- zero_extend, m <- [0x0301, 0x0323], + do: [b, v1, z, v2, m]) ++ + # SARA AM, which decomposes + (for b <- [0x0E01, 0x0EA1], v <- [0x0E33, 0x0EB3], tail <- [[], [0x0301], [0x200C]], + do: [b, v] ++ tail) ++ + # the codepoints assigned after the Unicode version Python's tables carry + (for b <- bases, l <- late_marks, m <- marks, do: [b, l, m]) ++ + (for b <- bases, m <- marks, l <- late_marks, do: [b, m, l]) ++ + # Prepend, regional indicators and ZWJ sequences, with marks attached + (for p <- prepends, b <- bases, m <- marks, do: [p, b, m]) ++ + (for a <- regional, b <- regional, m <- marks, do: [a, b, m]) ++ + (for a <- pictographs, b <- pictographs, m <- marks, do: [a, 0x200D, b, m]) ++ + # Hangul: L + precomposed syllable, and L L V adjacency, for GB6-GB8. The + # corpus had no Hangul at all and could not see any of those rules. + # Sampling four syllables here is how a 29,893-row gap read as 949. LV + # (no trailing consonant) and LVT (with one) behave differently, and what + # follows the pair matters too, so all three axes are swept rather than + # sampled on one and fixed on the others. + (for l <- 0x1100..0x115F, sy <- lv_syllables, do: [l, sy]) ++ + (for l <- 0x1100..0x115F, sy <- lvt_syllables, do: [l, sy]) ++ + (for l <- 0x1100..0x1112, sy <- Enum.take_every(lv_syllables, 2), t <- trailers, do: [l, sy | t]) ++ + (for l <- 0x1100..0x1112, sy <- Enum.take_every(lvt_syllables, 2), t <- trailers, do: [l, sy | t]) ++ + # The boundary of every range in `name_rules`, plus one either side, read + # from out/range_edges.txt (written by edges.py). A hand-picked slice of a + # range never steps across its edge, and three rounds running that is exactly + # where the surviving mutant was — U+1160 HANGUL JUNGSEONG FILLER is assigned + # and GCB=V, and the sweep started at U+1161. + (for e <- range_edges, do: [e]) ++ + (for e <- range_edges, v <- [0x1161, 0x0301, 0x61], do: [e, v]) ++ + (for e <- range_edges, l <- [0x1100, 0xAC00], do: [l, e]) ++ + (for e <- range_edges, do: [0x1100, e, 0x11A8]) ++ + # Extended jamo: U+A960-A97C (L), U+D7B0-D7C6 (V), U+D7CB-D7FB (T). The + # corpus contained zero codepoints from all three, so narrowing any of the + # three `_HANGUL_*` ranges in `name_rules` left the harness at exit 0 while + # `U+A960 U+1161` went from one grapheme to two. + (for l <- 0xA960..0xA97C, v <- 0x1161..0x1165, do: [l, v]) ++ + (for l <- 0x1100..0x1105, v <- 0xD7B0..0xD7C6, do: [l, v]) ++ + (for l <- 0x1100..0x1105, v <- 0x1161..0x1163, t <- 0xD7CB..0xD7FB, do: [l, v, t]) ++ + (for l <- 0xA960..0xA97C, v <- 0xD7B0..0xD7B4, t <- [0x11A8, 0xD7CB], do: [l, v, t]) ++ + (for a <- 0x1100..0x1105, b <- 0x1100..0x1105, v <- 0x1161..0x1165, do: [a, b, v]) ++ + (for l <- 0x1100..0x1105, v <- 0x1161..0x1165, t <- 0x11A7..0x11AC, do: [l, v, t]) ++ + # Syllable-block edges, which a stride steps over. + (for l <- 0x1100..0x1112, sy <- [0xAC00, 0xAC01, 0xD7A2, 0xD7A3], t <- trailers, do: [l, sy | t]) ++ + (for v <- 0x1161..0x1175, t <- [0x11A7, 0x11A8], do: [0x1100, v, t]) ++ + (for l <- 0x1100..0x115F, v <- two_part_vowel_marks, do: [l, v]) ++ + (for l <- 0x1100..0x1112, v <- two_part_vowel_marks, t <- trailers, do: [l, v | t]) ++ + # Hangul crossed with Prepend, which the sweep never covered: OTP + # decomposes a precomposed syllable that is not the cluster lead. + (for p <- prepends, sy <- [0xAC00, 0xAE4C, 0xD55C], do: [p, sy]) ++ + (for p <- prepends, l <- 0x1100..0x1105, v <- 0x1161..0x1163, do: [p, l, v]) ++ + # CR and LF, for GB3/GB4/GB5. The corpus had neither. + (for a <- [0x0D, 0x0A], b <- [0x0D, 0x0A, 0x61], do: [a, b]) ++ + (for b <- bases, do: [b, 0x0D, 0x0A, b]) ++ + # Odd-length regional indicator runs, for the GB12/GB13 parity rule. The + # corpus only had pairs, which an implementation with no parity rule also + # gets right. + (for n <- 1..5, do: List.duplicate(0x1F1EB, n)) ++ + (for n <- 1..5, do: List.duplicate(0x1F1EB, n) ++ [0x0301]) ++ + (for a <- regional, b <- regional, c <- regional, do: [a, b, c]) ++ + # The two places Elixir deviates from UAX #29, which the corpus previously + # lacked entirely. `regex` undercounts on both — 100 where Elixir says 200 — + # so without these the corpus shows only the direction that truncates early + # and hides the direction that ships a name over the cap. + (for p <- pictographs, m <- marks, n <- [1, 3, 50], do: List.duplicate([p, 0x200D, m], n) |> List.flatten()) ++ + (for c1 <- [0x0915, 0x0937, 0x0924], c2 <- [0x0915, 0x0937, 0x0924], n <- [1, 3], + do: List.duplicate([c1, 0x094D, c2], n) |> List.flatten()) ++ + [[0x0928, 0x092E, 0x0938, 0x094D, 0x0924, 0x0947]] ++ + # pure ASCII, which must be untouched + Enum.map(ascii_words, & &1) ++ + (for w <- ascii_words, m <- marks, do: w ++ [m]) + +# Cluster boundaries over that corpus. `check.py` compared six things and not +# one of them was a cluster boundary — the clusterer was checked only through +# per-codepoint break classes, which cannot see the regional-indicator parity +# rule or the CR-LF rule at all. +clusters = + Enum.map_join(shapes, "\n", fn cps -> + input = List.to_string(cps) + + boundaries = + input + |> String.graphemes() + |> Enum.map_join(",", fn g -> + g |> :unicode.characters_to_list() |> Enum.map_join("+", &Integer.to_string(&1, 16)) + end) + + Enum.map_join(cps, ",", hex) <> "\t" <> boundaries + end) + +File.write!("out/clusters.txt", clusters <> "\n") + +IO.puts("corpus rows: #{length(shapes)}") + +File.write!( + "out/version.txt", + "elixir #{System.version()}\notp #{System.otp_release()}\n" +) + +IO.puts("wrote out/{classmap,extpict,trim,lookback,clusters,version}.txt")