From dc6a806207841465e30c6e28c7537b6398c5e126 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Sat, 5 Sep 2026 04:33:07 +0000 Subject: [PATCH 1/5] Withhold a workflow we cannot redact, and stop three crashes Three faults in the redaction path, none of which depend on what we decide about job code in Sentry. redact_job_bodies returned the original document whenever anything went wrong, so a workflow it could not parse, or one whose jobs sit deeper than the top level, reached the model with every body intact. It now withholds the document instead. The id walk recursed without tracking what it had entered. A YAML anchor can point at its own container and PyYAML builds that as a real cycle, so such a workflow raised RecursionError, which the bare except then swallowed into the same unredacted fallback. extract_and_preserve_components assumed jobs, triggers and edges were mappings and that a body was a string. A list of jobs, a numeric body or a null entry raised AttributeError or TypeError out of a normal chat request. Split out of #660. --- .../tests/unit/test_redaction_robustness.py | 72 +++++++++++++++++++ services/workflow_chat/workflow_chat.py | 14 ++-- services/yaml_utils.py | 46 ++++++++---- 3 files changed, 114 insertions(+), 18 deletions(-) create mode 100644 services/global_chat/tests/unit/test_redaction_robustness.py diff --git a/services/global_chat/tests/unit/test_redaction_robustness.py b/services/global_chat/tests/unit/test_redaction_robustness.py new file mode 100644 index 00000000..0cf4bc77 --- /dev/null +++ b/services/global_chat/tests/unit/test_redaction_robustness.py @@ -0,0 +1,72 @@ +"""Redaction has to hold back job code even when the document is odd. + +Returning the original on failure hands the model the very bodies redaction +exists to withhold, and a YAML anchor can point at its own container, which +PyYAML builds as a real cycle. +""" + +import pytest +import yaml +from workflow_chat.workflow_chat import AnthropicClient +from yaml_utils import WITHHELD_NOTICE, _remove_ids, redact_job_bodies + +SECRET = "callSecretApi()" + + +def test_a_normal_workflow_is_still_redacted() -> None: + out = redact_job_bodies(f"jobs:\n a:\n id: 123\n body: {SECRET}\n") + + assert SECRET not in out + assert "123" not in out + assert "jobs" in out + + +@pytest.mark.parametrize( + "document", + [ + f'jobs: {{a: {{body: "{SECRET}"}}\n broken', + f"workflows:\n wf:\n jobs:\n a:\n body: {SECRET}\n", + f"- {SECRET}\n", + f"jobs: {SECRET}\n", + ], + ids=["unparseable", "jobs-nested-deeper", "a-list", "jobs-not-a-mapping"], +) +def test_a_document_it_cannot_redact_is_withheld(document: str) -> None: + out = redact_job_bodies(document) + + assert SECRET not in out + assert out == WITHHELD_NOTICE + + +def test_the_id_walk_terminates_on_a_self_referential_anchor() -> None: + data = yaml.safe_load("jobs:\n a: &x\n body: code()\n loop: *x\n") + assert data["jobs"]["a"]["loop"] is data["jobs"]["a"] + + _remove_ids(data) + + assert "id" not in data["jobs"]["a"] + + +def test_redaction_terminates_on_a_self_referential_anchor() -> None: + out = redact_job_bodies(f"jobs:\n a: &x\n body: {SECRET}\n loop: *x\n") + + assert SECRET not in out + + +@pytest.mark.parametrize( + "yaml_data", + [ + {"jobs": [{"body": "code()"}]}, + {"jobs": {"a": {"body": 42}}}, + {"jobs": {"a": None}}, + {"jobs": "not a mapping"}, + {"triggers": {"t": None}}, + {"edges": {"e": "not a mapping"}}, + ], + ids=["jobs-a-list", "numeric-body", "null-job", "jobs-a-string", + "null-trigger", "edge-not-a-mapping"], +) +def test_preserving_components_tolerates_a_shape_it_did_not_expect(yaml_data: dict) -> None: + preserved, _ = AnthropicClient.extract_and_preserve_components(yaml_data) + + assert isinstance(preserved, dict) diff --git a/services/workflow_chat/workflow_chat.py b/services/workflow_chat/workflow_chat.py index 1377ad42..8c601238 100644 --- a/services/workflow_chat/workflow_chat.py +++ b/services/workflow_chat/workflow_chat.py @@ -542,9 +542,11 @@ def extract_and_preserve_components(yaml_data): preserved_values = {} - if "jobs" in yaml_data: + if isinstance(yaml_data.get("jobs"), dict): for job_key, job_data in yaml_data["jobs"].items(): - if "body" in job_data: + if not isinstance(job_data, dict): + continue + 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_BLOCK_{job_key}__" @@ -556,17 +558,17 @@ def extract_and_preserve_components(yaml_data): preserved_values[placeholder] = job_data["id"] job_data["id"] = placeholder - if "triggers" in yaml_data: + if isinstance(yaml_data.get("triggers"), dict): for trigger_key, trigger_data in yaml_data["triggers"].items(): - if "id" in trigger_data: + if isinstance(trigger_data, dict) and "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 "edges" in yaml_data: + if isinstance(yaml_data.get("edges"), dict): for edge_key, edge_data in yaml_data["edges"].items(): - if "id" in edge_data: + if isinstance(edge_data, dict) and "id" in edge_data: placeholder = f"__ID_EDGE_{edge_key}__" preserved_values[placeholder] = edge_data["id"] edge_data["id"] = placeholder diff --git a/services/yaml_utils.py b/services/yaml_utils.py index 17920ec7..1db9b83f 100644 --- a/services/yaml_utils.py +++ b/services/yaml_utils.py @@ -114,6 +114,11 @@ def workflow_has_job_code(yaml_str: str | None) -> bool: return False +#: Sent in place of the document when redaction cannot be completed. Returning +#: the original would hand the model the very bodies this exists to hold back. +WITHHELD_NOTICE = "# workflow withheld: it could not be read well enough to redact" + + def redact_job_bodies(yaml_str: str) -> str: """Return workflow YAML with job bodies replaced by a placeholder and id fields removed. @@ -121,29 +126,46 @@ def redact_job_bodies(yaml_str: str) -> str: This is the read-only structural view shown to the planner and to job_chat in subagent mode. It never round-trips back into a real workflow, so the UUID ids are pure noise to the model — dropping them saves tokens. + + Withholds the document rather than returning it when anything goes wrong. """ try: yaml_data = yaml.safe_load(yaml_str) - if yaml_data and "jobs" in yaml_data: - _remove_ids(yaml_data) - for job_data in yaml_data["jobs"].values(): - if "body" in job_data: - job_data["body"] = "# [use inspect_job_code to view]" - return yaml.dump(yaml_data, sort_keys=False) except Exception: - pass - return yaml_str + return WITHHELD_NOTICE + if not isinstance(yaml_data, dict) or not isinstance(yaml_data.get("jobs"), dict): + return WITHHELD_NOTICE -def _remove_ids(obj: object) -> None: - """Recursively remove 'id' keys from a parsed YAML structure.""" + try: + _remove_ids(yaml_data) + for job_data in yaml_data["jobs"].values(): + if isinstance(job_data, dict) and "body" in job_data: + job_data["body"] = "# [use inspect_job_code to view]" + return yaml.dump(yaml_data, sort_keys=False) + except Exception: + return WITHHELD_NOTICE + + +def _remove_ids(obj: object, seen: set | None = None) -> None: + """Recursively remove 'id' keys from a parsed YAML structure. + + A YAML anchor can refer to its own container, and PyYAML builds that as a + real cycle, so the walk tracks what it has already entered. + """ + if seen is None: + seen = set() + if id(obj) in seen: + return if isinstance(obj, dict): + seen.add(id(obj)) obj.pop("id", None) for value in obj.values(): - _remove_ids(value) + _remove_ids(value, seen) elif isinstance(obj, list): + seen.add(id(obj)) for item in obj: - _remove_ids(item) + _remove_ids(item, seen) def stitch_job_code(yaml_str: str, job_key: str, new_code: str) -> str: From 616e5c17531103c74e09dbaf49d30723c563ba37 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Sat, 5 Sep 2026 05:06:20 +0000 Subject: [PATCH 2/5] Add a changeset --- .changeset/redaction-robustness.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/redaction-robustness.md diff --git a/.changeset/redaction-robustness.md b/.changeset/redaction-robustness.md new file mode 100644 index 00000000..34c29192 --- /dev/null +++ b/.changeset/redaction-robustness.md @@ -0,0 +1,5 @@ +--- +"apollo": patch +--- + +Withhold a workflow that cannot be redacted, rather than passing it to the model in full From 67dbc0849cb5745f785931b6f2af67e9e8a6b50a Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Sat, 5 Sep 2026 05:28:08 +0000 Subject: [PATCH 3/5] Redact a body wherever it sits, and keep a workflow that has none Review found the first pass both too eager and not eager enough. Too eager: a workflow with no job bodies at all, a trigger-only one, was withheld whole. It had nothing to hide, so the planner lost its structure for nothing. A document is now withheld only when it cannot be parsed, or when it does not look like a workflow at all. Not eager enough: redaction only looked at jobs..body, so a body nested under a workflow in a project export, a job written as a list, or one pulled in through a merge key all reached the model intact. The walk now finds a body wherever it sits. Two more of the same shape, found in the same review. The read-only id strip in workflow_chat kept its own copy of the walker, so it still recursed forever on an anchor cycle and then returned the document with the ids in it; it now calls the guarded one. And workflow_has_job_code treated a scalar document as a mapping and raised TypeError. Known limit: code that appears under some other key and is aliased into a body is redacted in the body and left alone elsewhere. That matches main. --- .../tests/unit/test_redaction_robustness.py | 48 ++++++++++++++- services/workflow_chat/workflow_chat.py | 14 +---- services/yaml_utils.py | 58 ++++++++++++++++--- 3 files changed, 99 insertions(+), 21 deletions(-) diff --git a/services/global_chat/tests/unit/test_redaction_robustness.py b/services/global_chat/tests/unit/test_redaction_robustness.py index 0cf4bc77..39f824c0 100644 --- a/services/global_chat/tests/unit/test_redaction_robustness.py +++ b/services/global_chat/tests/unit/test_redaction_robustness.py @@ -8,7 +8,12 @@ import pytest import yaml from workflow_chat.workflow_chat import AnthropicClient -from yaml_utils import WITHHELD_NOTICE, _remove_ids, redact_job_bodies +from yaml_utils import ( + WITHHELD_NOTICE, + _remove_ids, + redact_job_bodies, + workflow_has_job_code, +) SECRET = "callSecretApi()" @@ -25,11 +30,10 @@ def test_a_normal_workflow_is_still_redacted() -> None: "document", [ f'jobs: {{a: {{body: "{SECRET}"}}\n broken', - f"workflows:\n wf:\n jobs:\n a:\n body: {SECRET}\n", f"- {SECRET}\n", f"jobs: {SECRET}\n", ], - ids=["unparseable", "jobs-nested-deeper", "a-list", "jobs-not-a-mapping"], + ids=["unparseable", "a-list", "jobs-not-a-mapping"], ) def test_a_document_it_cannot_redact_is_withheld(document: str) -> None: out = redact_job_bodies(document) @@ -70,3 +74,41 @@ def test_preserving_components_tolerates_a_shape_it_did_not_expect(yaml_data: di preserved, _ = AnthropicClient.extract_and_preserve_components(yaml_data) assert isinstance(preserved, dict) + +@pytest.mark.parametrize( + "document", + [ + f'jobs:\n a:\n body: "ok"\n steps:\n inner:\n body: "{SECRET}"\n', + f'jobs:\n a:\n - body: "{SECRET}"\n', + f'shared: &s\n body: "{SECRET}"\njobs:\n a:\n <<: *s\n', + f"workflows:\n wf:\n jobs:\n a:\n body: {SECRET}\n", + ], + ids=["nested-deeper", "job-is-a-list", "merge-key", "project-export"], +) +def test_a_body_is_redacted_wherever_it_sits(document: str) -> None: + out = redact_job_bodies(document) + + assert SECRET not in out + assert out != WITHHELD_NOTICE + + +def test_a_workflow_with_no_bodies_is_kept_not_withheld() -> None: + """Withholding a document that has nothing to hide loses the planner its + structure for no gain.""" + out = redact_job_bodies("triggers:\n t:\n type: cron\n") + + assert out != WITHHELD_NOTICE + assert "cron" in out + + +def test_the_read_only_id_strip_also_survives_a_cycle() -> None: + document = "jobs:\n a: &x\n id: SECRET-ID\n body: code()\n loop: *x\n" + + out = AnthropicClient.remove_ids_from_yaml(AnthropicClient, document) + + assert "SECRET-ID" not in out + + +def test_a_scalar_document_is_not_treated_as_a_workflow() -> None: + assert redact_job_bodies("jobs") == WITHHELD_NOTICE + assert workflow_has_job_code("jobs") is False diff --git a/services/workflow_chat/workflow_chat.py b/services/workflow_chat/workflow_chat.py index 8c601238..9a5508a6 100644 --- a/services/workflow_chat/workflow_chat.py +++ b/services/workflow_chat/workflow_chat.py @@ -63,6 +63,7 @@ from langfuse import observe, propagate_attributes, get_client as get_langfuse_client from langfuse_util import should_track, build_tags, build_generation_diff, mask_secrets from util import ApolloError, create_logger, add_page_prefix, APOLLO_VERSION +from yaml_utils import _remove_ids from .gen_project_prompt import build_prompt from workflow_chat.available_adaptors import get_available_adaptors from streaming_util import ( @@ -364,17 +365,8 @@ def remove_ids_from_yaml(self, yaml_str): return yaml_str try: yaml_data = yaml.safe_load(yaml_str) - - def remove_ids(obj): - if isinstance(obj, dict): - obj.pop("id", None) - for v in obj.values(): - remove_ids(v) - elif isinstance(obj, list): - for item in obj: - remove_ids(item) - - remove_ids(yaml_data) + # Shared with redact_job_bodies so the cycle guard lives in one place. + _remove_ids(yaml_data) return yaml.dump(yaml_data, sort_keys=False, default_flow_style=False) except Exception as e: logger.warning(f"Could not remove IDs from YAML: {e}") diff --git a/services/yaml_utils.py b/services/yaml_utils.py index 1db9b83f..27c2cb1e 100644 --- a/services/yaml_utils.py +++ b/services/yaml_utils.py @@ -69,7 +69,7 @@ def find_job_in_yaml(yaml_str: str, step_name: str) -> tuple[str | None, dict | except Exception: return None, None - if not yaml_data or "jobs" not in yaml_data: + if not isinstance(yaml_data, dict) or "jobs" not in yaml_data: return None, None jobs = yaml_data["jobs"] @@ -105,7 +105,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") @@ -114,11 +114,56 @@ def workflow_has_job_code(yaml_str: str | None) -> bool: return False +#: What a job body is replaced with in the structural view. +REDACTED_BODY = "# [use inspect_job_code to view]" + #: Sent in place of the document when redaction cannot be completed. Returning #: the original would hand the model the very bodies this exists to hold back. WITHHELD_NOTICE = "# workflow withheld: it could not be read well enough to redact" +#: The sections a workflow document is made of. A document holding none of +#: them is not one we know how to redact, so it is withheld rather than dumped. +WORKFLOW_SECTIONS = ("jobs", "triggers", "edges", "workflows") + + +def _looks_like_a_workflow(yaml_data: object) -> bool: + """A mapping with at least one recognised section, none of them a scalar. + + `jobs: ` has a section we recognise holding something we cannot walk, + so it is not safe to dump back. + """ + if not isinstance(yaml_data, dict): + return False + present = [key for key in WORKFLOW_SECTIONS if key in yaml_data] + if not present: + return False + return all(isinstance(yaml_data[key], (dict, list)) for key in present) + + +def _redact_bodies(obj: object, seen: set | None = None) -> None: + """Replace every `body` string anywhere in the tree, not just jobs.*.body. + + A project export nests its jobs under each workflow, so a walk that only + looks at the top level hands those bodies straight to the model. + """ + if seen is None: + seen = set() + if id(obj) in seen: + return + if isinstance(obj, dict): + seen.add(id(obj)) + for key, value in obj.items(): + if key == "body" and isinstance(value, str): + obj[key] = REDACTED_BODY + else: + _redact_bodies(value, seen) + elif isinstance(obj, list): + seen.add(id(obj)) + for item in obj: + _redact_bodies(item, seen) + + def redact_job_bodies(yaml_str: str) -> str: """Return workflow YAML with job bodies replaced by a placeholder and id fields removed. @@ -127,21 +172,20 @@ def redact_job_bodies(yaml_str: str) -> str: in subagent mode. It never round-trips back into a real workflow, so the UUID ids are pure noise to the model — dropping them saves tokens. - Withholds the document rather than returning it when anything goes wrong. + Withholds the document only when it cannot be read or written back. + Anything it can parse gets every body redacted, wherever they sit. """ try: yaml_data = yaml.safe_load(yaml_str) except Exception: return WITHHELD_NOTICE - if not isinstance(yaml_data, dict) or not isinstance(yaml_data.get("jobs"), dict): + if not _looks_like_a_workflow(yaml_data): return WITHHELD_NOTICE try: _remove_ids(yaml_data) - for job_data in yaml_data["jobs"].values(): - if isinstance(job_data, dict) and "body" in job_data: - job_data["body"] = "# [use inspect_job_code to view]" + _redact_bodies(yaml_data) return yaml.dump(yaml_data, sort_keys=False) except Exception: return WITHHELD_NOTICE From fb64ea429d98f8c6aa9926190640c4a07b3540a0 Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Tue, 8 Sep 2026 18:46:31 +0000 Subject: [PATCH 4/5] Give the two withholding causes their own notice A document that cannot be parsed and one whose shape we cannot redact both returned the same line, so the planner could not tell a workflow it could have read from one that was never valid. --- .../tests/unit/test_redaction_robustness.py | 22 ++++++++++--------- services/yaml_utils.py | 19 +++++++++++----- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/services/global_chat/tests/unit/test_redaction_robustness.py b/services/global_chat/tests/unit/test_redaction_robustness.py index 39f824c0..3d01de86 100644 --- a/services/global_chat/tests/unit/test_redaction_robustness.py +++ b/services/global_chat/tests/unit/test_redaction_robustness.py @@ -9,13 +9,15 @@ import yaml from workflow_chat.workflow_chat import AnthropicClient from yaml_utils import ( - WITHHELD_NOTICE, + WITHHELD_UNPARSEABLE, + WITHHELD_UNREDACTABLE, _remove_ids, redact_job_bodies, workflow_has_job_code, ) SECRET = "callSecretApi()" +WITHHELD = (WITHHELD_UNPARSEABLE, WITHHELD_UNREDACTABLE) def test_a_normal_workflow_is_still_redacted() -> None: @@ -27,19 +29,19 @@ def test_a_normal_workflow_is_still_redacted() -> None: @pytest.mark.parametrize( - "document", + ("document", "notice"), [ - f'jobs: {{a: {{body: "{SECRET}"}}\n broken', - f"- {SECRET}\n", - f"jobs: {SECRET}\n", + (f'jobs: {{a: {{body: "{SECRET}"}}\n broken', WITHHELD_UNPARSEABLE), + (f"- {SECRET}\n", WITHHELD_UNREDACTABLE), + (f"jobs: {SECRET}\n", WITHHELD_UNREDACTABLE), ], ids=["unparseable", "a-list", "jobs-not-a-mapping"], ) -def test_a_document_it_cannot_redact_is_withheld(document: str) -> None: +def test_a_document_it_cannot_redact_is_withheld(document: str, notice: str) -> None: out = redact_job_bodies(document) assert SECRET not in out - assert out == WITHHELD_NOTICE + assert out == notice def test_the_id_walk_terminates_on_a_self_referential_anchor() -> None: @@ -89,7 +91,7 @@ def test_a_body_is_redacted_wherever_it_sits(document: str) -> None: out = redact_job_bodies(document) assert SECRET not in out - assert out != WITHHELD_NOTICE + assert out not in WITHHELD def test_a_workflow_with_no_bodies_is_kept_not_withheld() -> None: @@ -97,7 +99,7 @@ def test_a_workflow_with_no_bodies_is_kept_not_withheld() -> None: structure for no gain.""" out = redact_job_bodies("triggers:\n t:\n type: cron\n") - assert out != WITHHELD_NOTICE + assert out not in WITHHELD assert "cron" in out @@ -110,5 +112,5 @@ def test_the_read_only_id_strip_also_survives_a_cycle() -> None: def test_a_scalar_document_is_not_treated_as_a_workflow() -> None: - assert redact_job_bodies("jobs") == WITHHELD_NOTICE + assert redact_job_bodies("jobs") == WITHHELD_UNREDACTABLE assert workflow_has_job_code("jobs") is False diff --git a/services/yaml_utils.py b/services/yaml_utils.py index 27c2cb1e..93073931 100644 --- a/services/yaml_utils.py +++ b/services/yaml_utils.py @@ -118,8 +118,17 @@ def workflow_has_job_code(yaml_str: str | None) -> bool: REDACTED_BODY = "# [use inspect_job_code to view]" #: Sent in place of the document when redaction cannot be completed. Returning -#: the original would hand the model the very bodies this exists to hold back. -WITHHELD_NOTICE = "# workflow withheld: it could not be read well enough to redact" +#: the original would hand the model the very bodies this exists to hold back: +#: YAML that fails to parse still has its job code sitting in it. +#: +#: Two causes, two notices, so the planner can tell a document it could have +#: read from one that was never valid. +WITHHELD_UNPARSEABLE = ( + "# workflow withheld: the YAML does not parse, so its job code could not be redacted" +) +WITHHELD_UNREDACTABLE = ( + "# workflow withheld: the YAML parsed but no safe view of it could be built" +) #: The sections a workflow document is made of. A document holding none of @@ -178,17 +187,17 @@ def redact_job_bodies(yaml_str: str) -> str: try: yaml_data = yaml.safe_load(yaml_str) except Exception: - return WITHHELD_NOTICE + return WITHHELD_UNPARSEABLE if not _looks_like_a_workflow(yaml_data): - return WITHHELD_NOTICE + return WITHHELD_UNREDACTABLE try: _remove_ids(yaml_data) _redact_bodies(yaml_data) return yaml.dump(yaml_data, sort_keys=False) except Exception: - return WITHHELD_NOTICE + return WITHHELD_UNREDACTABLE def _remove_ids(obj: object, seen: set | None = None) -> None: From 9decd654cdc70f7e5928cd36ac3d476a898cc8fb Mon Sep 17 00:00:00 2001 From: "Elias W. BA" Date: Wed, 9 Sep 2026 13:30:11 +0000 Subject: [PATCH 5/5] Return a document we cannot read, rather than withholding it Job bodies are deferred to save tokens, not kept from the model: it reads any of them with inspect_job_code, and subagents get the whole YAML anyway. So withholding a document it cannot parse only costs it the structure, when it could have said what was wrong with it. --- .changeset/redaction-robustness.md | 2 +- .../tests/unit/test_redaction_robustness.py | 37 ++++++------- services/yaml_utils.py | 53 ++++--------------- 3 files changed, 25 insertions(+), 67 deletions(-) diff --git a/.changeset/redaction-robustness.md b/.changeset/redaction-robustness.md index 34c29192..f73333a7 100644 --- a/.changeset/redaction-robustness.md +++ b/.changeset/redaction-robustness.md @@ -2,4 +2,4 @@ "apollo": patch --- -Withhold a workflow that cannot be redacted, rather than passing it to the model in full +Replace a job body wherever it sits in the workflow, not only at the top level, and stop three crashes on a shape the model did not expect diff --git a/services/global_chat/tests/unit/test_redaction_robustness.py b/services/global_chat/tests/unit/test_redaction_robustness.py index 3d01de86..f2595e02 100644 --- a/services/global_chat/tests/unit/test_redaction_robustness.py +++ b/services/global_chat/tests/unit/test_redaction_robustness.py @@ -1,23 +1,21 @@ -"""Redaction has to hold back job code even when the document is odd. +"""Redaction has to survive an odd document rather than throw or mangle it. -Returning the original on failure hands the model the very bodies redaction -exists to withhold, and a YAML anchor can point at its own container, which -PyYAML builds as a real cycle. +Bodies are deferred to save tokens, not hidden: the model can read any of them +with inspect_job_code. So a document we cannot parse comes back as it came, +where the model can diagnose it. A YAML anchor can point at its own container, +which PyYAML builds as a real cycle. """ import pytest import yaml from workflow_chat.workflow_chat import AnthropicClient from yaml_utils import ( - WITHHELD_UNPARSEABLE, - WITHHELD_UNREDACTABLE, _remove_ids, redact_job_bodies, workflow_has_job_code, ) SECRET = "callSecretApi()" -WITHHELD = (WITHHELD_UNPARSEABLE, WITHHELD_UNREDACTABLE) def test_a_normal_workflow_is_still_redacted() -> None: @@ -29,19 +27,18 @@ def test_a_normal_workflow_is_still_redacted() -> None: @pytest.mark.parametrize( - ("document", "notice"), + "document", [ - (f'jobs: {{a: {{body: "{SECRET}"}}\n broken', WITHHELD_UNPARSEABLE), - (f"- {SECRET}\n", WITHHELD_UNREDACTABLE), - (f"jobs: {SECRET}\n", WITHHELD_UNREDACTABLE), + f'jobs: {{a: {{body: "{SECRET}"}}\n broken', + f"- {SECRET}\n", + f"jobs: {SECRET}\n", ], ids=["unparseable", "a-list", "jobs-not-a-mapping"], ) -def test_a_document_it_cannot_redact_is_withheld(document: str, notice: str) -> None: - out = redact_job_bodies(document) - - assert SECRET not in out - assert out == notice +def test_a_document_it_cannot_redact_comes_back_as_it_came(document: str) -> None: + """Withholding it would only cost the model the structure. It can already + read any body it wants, so there is nothing here to keep from it.""" + assert redact_job_bodies(document) == document def test_the_id_walk_terminates_on_a_self_referential_anchor() -> None: @@ -91,15 +88,11 @@ def test_a_body_is_redacted_wherever_it_sits(document: str) -> None: out = redact_job_bodies(document) assert SECRET not in out - assert out not in WITHHELD -def test_a_workflow_with_no_bodies_is_kept_not_withheld() -> None: - """Withholding a document that has nothing to hide loses the planner its - structure for no gain.""" +def test_a_workflow_with_no_bodies_is_kept() -> None: out = redact_job_bodies("triggers:\n t:\n type: cron\n") - assert out not in WITHHELD assert "cron" in out @@ -112,5 +105,5 @@ def test_the_read_only_id_strip_also_survives_a_cycle() -> None: def test_a_scalar_document_is_not_treated_as_a_workflow() -> None: - assert redact_job_bodies("jobs") == WITHHELD_UNREDACTABLE + assert redact_job_bodies("jobs") == "jobs" assert workflow_has_job_code("jobs") is False diff --git a/services/yaml_utils.py b/services/yaml_utils.py index 93073931..5f057961 100644 --- a/services/yaml_utils.py +++ b/services/yaml_utils.py @@ -117,38 +117,6 @@ def workflow_has_job_code(yaml_str: str | None) -> bool: #: What a job body is replaced with in the structural view. REDACTED_BODY = "# [use inspect_job_code to view]" -#: Sent in place of the document when redaction cannot be completed. Returning -#: the original would hand the model the very bodies this exists to hold back: -#: YAML that fails to parse still has its job code sitting in it. -#: -#: Two causes, two notices, so the planner can tell a document it could have -#: read from one that was never valid. -WITHHELD_UNPARSEABLE = ( - "# workflow withheld: the YAML does not parse, so its job code could not be redacted" -) -WITHHELD_UNREDACTABLE = ( - "# workflow withheld: the YAML parsed but no safe view of it could be built" -) - - -#: The sections a workflow document is made of. A document holding none of -#: them is not one we know how to redact, so it is withheld rather than dumped. -WORKFLOW_SECTIONS = ("jobs", "triggers", "edges", "workflows") - - -def _looks_like_a_workflow(yaml_data: object) -> bool: - """A mapping with at least one recognised section, none of them a scalar. - - `jobs: ` has a section we recognise holding something we cannot walk, - so it is not safe to dump back. - """ - if not isinstance(yaml_data, dict): - return False - present = [key for key in WORKFLOW_SECTIONS if key in yaml_data] - if not present: - return False - return all(isinstance(yaml_data[key], (dict, list)) for key in present) - def _redact_bodies(obj: object, seen: set | None = None) -> None: """Replace every `body` string anywhere in the tree, not just jobs.*.body. @@ -178,26 +146,23 @@ def redact_job_bodies(yaml_str: str) -> str: fields removed. This is the read-only structural view shown to the planner and to job_chat - in subagent mode. It never round-trips back into a real workflow, so the - UUID ids are pure noise to the model — dropping them saves tokens. + in subagent mode. Bodies are deferred rather than hidden: the model reads + any of them with inspect_job_code, so this is about tokens, not secrecy. + The UUID ids never round-trip back into a real workflow, so dropping them + saves tokens too. - Withholds the document only when it cannot be read or written back. - Anything it can parse gets every body redacted, wherever they sit. + A document we cannot read is returned as it came. The model can say what is + wrong with it, which is more use than telling it there is no workflow. """ try: yaml_data = yaml.safe_load(yaml_str) - except Exception: - return WITHHELD_UNPARSEABLE - - if not _looks_like_a_workflow(yaml_data): - return WITHHELD_UNREDACTABLE - - try: + if not isinstance(yaml_data, dict): + return yaml_str _remove_ids(yaml_data) _redact_bodies(yaml_data) return yaml.dump(yaml_data, sort_keys=False) except Exception: - return WITHHELD_UNREDACTABLE + return yaml_str def _remove_ids(obj: object, seen: set | None = None) -> None: