From 3ca229aed43fb8072bdf5d79b85df9a0e90437df Mon Sep 17 00:00:00 2001 From: Hsuehtan <296098438+Hsuehtan@users.noreply.github.com> Date: Wed, 7 Oct 2026 19:25:56 +0800 Subject: [PATCH 1/2] refactor(lark): decide the application id shape in one owner Four sites compiled the cli_-prefixed application id themselves -- the transport hub, bot_scopes, event_collector_runtime and goal_channel_delivery_contract, the last with its own anchors on the same body -- and eight more modules import the transport hub's name, so a bound fix had three possible homes. The shape now lives in identity_shapes.LARK_APP_ID_PATTERN, which already owns the other four Lark identifier bodies and states why anchored and unanchored spellings stay distinct. Every caller applies fullmatch, where the anchors are redundant, so no product answer changes; the guard's tricky-value table pins that. event_collector_runtime is declared by file and count instead of converted, because #5248 restructures that file. Signed-off-by: Hsuehtan <296098438+Hsuehtan@users.noreply.github.com> --- loopx/extensions/lark/bot_scopes.py | 3 +- .../lark/goal_channel_delivery_contract.py | 5 +- .../extensions/lark/goal_channel_transport.py | 2 +- loopx/extensions/lark/identity_shapes.py | 11 +++ .../test_lark_identity_shape_owner.py | 90 ++++++++++++++++--- 5 files changed, 93 insertions(+), 18 deletions(-) diff --git a/loopx/extensions/lark/bot_scopes.py b/loopx/extensions/lark/bot_scopes.py index 1ab9d8d9f3..1f82d929ce 100644 --- a/loopx/extensions/lark/bot_scopes.py +++ b/loopx/extensions/lark/bot_scopes.py @@ -9,9 +9,8 @@ from __future__ import annotations -import re +from .identity_shapes import LARK_APP_ID_PATTERN as APP_ID_PATTERN -APP_ID_PATTERN = re.compile(r"cli_[A-Za-z0-9_-]+") _OFFICIAL_SCOPE_APPLY_HOSTS = ("open.feishu.cn", "open.larkoffice.com") # 核心：发消息、建群/查群/群成员管理 —— goal-channel、reviewer、kanban 必需 diff --git a/loopx/extensions/lark/goal_channel_delivery_contract.py b/loopx/extensions/lark/goal_channel_delivery_contract.py index ea12b3e820..90c2b90b50 100644 --- a/loopx/extensions/lark/goal_channel_delivery_contract.py +++ b/loopx/extensions/lark/goal_channel_delivery_contract.py @@ -6,10 +6,9 @@ from collections.abc import Callable, Mapping from typing import Any -from .identity_shapes import LARK_CHAT_ID_PATTERN +from .identity_shapes import LARK_APP_ID_PATTERN, LARK_CHAT_ID_PATTERN _GOAL_ID_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._:-]{0,159}$") -_LARK_APP_ID_RE = re.compile(r"^cli_[A-Za-z0-9_-]+$") _LARK_PROFILE_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]{0,99}$") @@ -45,7 +44,7 @@ def goal_channel_delivery_route( or not LARK_CHAT_ID_PATTERN.fullmatch(chat_id) or not _LARK_PROFILE_RE.fullmatch(sender_profile) or sender_profile.lower() == "default" - or not _LARK_APP_ID_RE.fullmatch(bot_app_id) + or not LARK_APP_ID_PATTERN.fullmatch(bot_app_id) or not bot_display_name or not cli_bin ): diff --git a/loopx/extensions/lark/goal_channel_transport.py b/loopx/extensions/lark/goal_channel_transport.py index e09f8a68ba..39ea408d94 100644 --- a/loopx/extensions/lark/goal_channel_transport.py +++ b/loopx/extensions/lark/goal_channel_transport.py @@ -10,13 +10,13 @@ # Re-exported for the existing callers of this module; the shapes themselves are # decided once, in ``identity_shapes``. from .identity_shapes import ( # noqa: F401 + LARK_APP_ID_PATTERN as APP_ID_PATTERN, LARK_CHAT_ID_SEARCH as CHAT_ID_PATTERN, LARK_MESSAGE_ID_SEARCH as MESSAGE_ID_PATTERN, LARK_OPEN_ID_SEARCH as OPEN_ID_PATTERN, ) from .presentation.kanban import CommandRunner -APP_ID_PATTERN = re.compile(r"cli_[A-Za-z0-9_-]+") SAFE_PROFILE_PATTERN = re.compile(r"[A-Za-z0-9][A-Za-z0-9_.-]{0,99}") REQUIRED_GOAL_TOPIC_SCOPES = ("im:message", "im:message:readonly") REQUIRED_BOT_GROUP_HISTORY_SCOPES = ( diff --git a/loopx/extensions/lark/identity_shapes.py b/loopx/extensions/lark/identity_shapes.py index 8aa2eb8632..34b8e16e9b 100644 --- a/loopx/extensions/lark/identity_shapes.py +++ b/loopx/extensions/lark/identity_shapes.py @@ -14,6 +14,9 @@ ``goal_channel_contracts`` and ``goal_channel_notification`` ``search()`` for an id inside payload text, where ``^`` and ``$`` would change the answer -- so both spellings stay distinct and one module decides both. + +The fifth shape is the application id, ``cli_``-prefixed. It has only the +whole-value spelling because every caller applies ``fullmatch``. """ from __future__ import annotations @@ -26,6 +29,14 @@ LARK_MESSAGE_ID_PATTERN = re.compile(r"^om_[A-Za-z0-9_-]+$") LARK_CHAT_ID_PATTERN = re.compile(r"^oc_[A-Za-z0-9_-]+$") LARK_OPEN_ID_PATTERN = re.compile(r"^ou_[A-Za-z0-9_-]+$") +# The application a bot belongs to. Four sites decided this themselves -- the +# transport hub, ``bot_scopes``, ``event_collector_runtime`` and +# ``goal_channel_delivery_contract``, the last one with its own anchors on the +# same body -- and eight more modules reach the decision by importing the +# transport hub's name, so a fix to the body had three possible homes. +# Every caller applies ``fullmatch``, which is why only the whole-value spelling +# is stated here. +LARK_APP_ID_PATTERN = re.compile(r"^cli_[A-Za-z0-9_-]+$") LARK_MESSAGE_ID_SEARCH = re.compile(r"om_[A-Za-z0-9_-]+") LARK_CHAT_ID_SEARCH = re.compile(r"oc_[A-Za-z0-9_-]+") diff --git a/tests/architecture/test_lark_identity_shape_owner.py b/tests/architecture/test_lark_identity_shape_owner.py index acea4b8a76..d00536e56e 100644 --- a/tests/architecture/test_lark_identity_shape_owner.py +++ b/tests/architecture/test_lark_identity_shape_owner.py @@ -11,7 +11,7 @@ makes the decision is still an offender. 3. Are the two *uses* of a shape kept apart? A whole-value check and a search for an id inside larger text are different questions, so the owner states - both spellings: four anchored patterns and three unanchored ones. Either + both spellings: five anchored patterns and three unanchored ones. Either spelling anywhere else is an offender, and so is converting a declared site without retiring its declaration. """ @@ -20,6 +20,7 @@ import ast import pathlib +import re from types import ModuleType import pytest @@ -50,6 +51,7 @@ "message_id": r"om_[A-Za-z0-9_-]+", "chat_id": r"oc_[A-Za-z0-9_-]+", "operator_id": r"ou_[A-Za-z0-9_-]+", + "app_id": r"cli_[A-Za-z0-9_-]+", } # A module has to import the regex machinery before it can decide a shape, and # both spellings this scan accepts (``re.X(...)`` and a name from @@ -58,13 +60,16 @@ # single body, which is one of the probe cases below. PRESCREEN_TOKENS = ("import re", "from re import") # Only these three bodies have a search spelling. An event id is never looked -# for inside larger text. +# for inside larger text, and neither is an application id: every ``cli_`` site +# this slice gathered applies ``fullmatch``, so the whole-value spelling alone is +# the complete contract there. SEARCH_IDENTIFIERS = {"chat_id", "message_id", "operator_id"} ANCHORED_EXPORTS = { "event_id": "LARK_EVENT_ID_PATTERN", "message_id": "LARK_MESSAGE_ID_PATTERN", "chat_id": "LARK_CHAT_ID_PATTERN", "operator_id": "LARK_OPEN_ID_PATTERN", + "app_id": "LARK_APP_ID_PATTERN", } SEARCH_EXPORTS = { "message_id": "LARK_MESSAGE_ID_SEARCH", @@ -94,17 +99,20 @@ "oc_", "om_", "ou_", + "cli_", "chat_id", "message_id", "operator_id", "event_id", + "app_id", ) DECLARED_INDIVIDUAL_SITES: dict[str, int] = { - # ``re.fullmatch(r"[A-Za-z0-9._:-]{1,240}", ...)`` against an event id. The - # in-flight goal-channel claim work restructures this file, so the site is - # declared here instead of racing that branch. - "loopx/extensions/lark/event_collector_runtime.py": 1, + # ``re.fullmatch(r"[A-Za-z0-9._:-]{1,240}", ...)`` against an event id, plus + # this file's own ``cli_`` compile. The in-flight goal-channel claim work + # restructures this file, so both sites are declared here instead of racing + # that branch; converting either one deletes its share of the count. + "loopx/extensions/lark/event_collector_runtime.py": 2, } SHAPE_CONSUMERS: dict[str, tuple[ModuleType, str]] = { @@ -134,7 +142,7 @@ INBOX_HUB: (event_inbox, ("CHAT_ID_PATTERN", "MESSAGE_ID_PATTERN")), TRANSPORT_HUB: ( goal_channel_transport, - ("CHAT_ID_PATTERN", "MESSAGE_ID_PATTERN", "OPEN_ID_PATTERN"), + ("APP_ID_PATTERN", "CHAT_ID_PATTERN", "MESSAGE_ID_PATTERN", "OPEN_ID_PATTERN"), ), } _RE_MODULE = "re" @@ -497,6 +505,9 @@ def test_owner_states_each_shape_as_the_recorded_whole_value_body() -> None: assert identity_shapes.LARK_OPEN_ID_PATTERN.pattern == ( "^" + WHOLE_VALUE_BODIES["operator_id"] + "$" ) + assert identity_shapes.LARK_APP_ID_PATTERN.pattern == ( + "^" + WHOLE_VALUE_BODIES["app_id"] + "$" + ) def test_the_owner_is_the_only_module_that_defines_any_of_them() -> None: @@ -575,6 +586,53 @@ def test_inbox_callers_still_receive_one_object_through_the_chain() -> None: assert module.MESSAGE_ID_PATTERN is identity_shapes.LARK_MESSAGE_ID_PATTERN, name +# The application id reaches most of its callers through the transport hub, so the +# identity assertion is the wiring proof that none of them kept a private copy. +APP_ID_CALLERS = ( + "bot_scopes", + "goal_channel_setup", + "goal_channel_runtime", + "goal_channel_blocked_notice", + "goal_channel_targets", + "goal_topic_connections", + "private_conversations", + "event_inbox", +) + + +@pytest.mark.parametrize("name", sorted(APP_ID_CALLERS)) +def test_every_app_id_caller_holds_the_owners_object(name: str) -> None: + module = __import__(f"loopx.extensions.lark.{name}", fromlist=["APP_ID_PATTERN"]) + assert module.APP_ID_PATTERN is identity_shapes.LARK_APP_ID_PATTERN, name + assert "APP_ID_PATTERN" not in module_level_bindings(module), name + + +def test_the_delivery_contract_holds_the_owners_object_too() -> None: + # It had its own anchored compile of the same body rather than a hub import. + assert ( + goal_channel_delivery_contract.LARK_APP_ID_PATTERN + is identity_shapes.LARK_APP_ID_PATTERN + ) + + +@pytest.mark.parametrize( + "value", + ["cli_ok1", "cli_ok1\n", "\ncli_ok1", "cli_a\nb", "", "cli_", "cli_\u00e9", + "cli_a b", "xcli_a", "cli_a-b_1.C", "cli_a" + "z" * 200], +) +def test_the_app_id_answer_is_the_same_anchored_or_not(value: str) -> None: + """Why gathering these sites cannot change a product answer. + + All three defining sites applied ``fullmatch`` to an unanchored body, and the + fourth applied an anchored one; ``re.fullmatch`` already requires the whole + string, so both spellings accept and reject exactly the same values. + """ + + anchored = bool(identity_shapes.LARK_APP_ID_PATTERN.fullmatch(value)) + unanchored = bool(re.fullmatch(r"cli_[A-Za-z0-9_-]+", value)) + assert anchored == unanchored, value + + def test_callback_validators_do_not_restate_the_identity_table() -> None: for module in (goal_channel_operation, team_plan_confirmation): tree = parse_module(module) @@ -755,6 +813,14 @@ def test_anchored_and_unanchored_fullmatch_agree_on_every_tricky_value( "operator id restated under an unrelated name", 'import re\n\nOPERATOR = re.compile(r"^ou_[A-Za-z0-9_-]+$")\n', ), + ( + "app id restated unanchored, the spelling three sites used", + 'import re\n\nAPP = re.compile(r"cli_[A-Za-z0-9_-]+")\n', + ), + ( + "app id restated anchored, the spelling the fourth site used", + 'import re\n\nAPP = re.compile(r"^cli_[A-Za-z0-9_-]+$")\n', + ), ( "event id inside a function body, not at module level", 'import re\n\n\ndef pick(value):\n' @@ -771,14 +837,14 @@ def test_anchored_and_unanchored_fullmatch_agree_on_every_tricky_value( "a chat id embedded in a larger route grammar", 'import re\n\nROUTE = re.compile(r"^route:oc_[A-Za-z0-9_-]+:v1$")\n', ), - ( - "an app id, which this slice does not own", - 'import re\n\nAPP = re.compile(r"^cli_[A-Za-z0-9_-]+$")\n', - ), ( "a pattern built from runtime data, which cannot be folded", 'import re\n\nCHAT = re.compile(prefix + "[A-Za-z0-9_-]+")\n', ), + ( + "a sender profile shape, which is a different decision", + 'import re\n\nPROFILE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]{0,99}$")\n', + ), ( "a module constant rebound locally before use, which is not trusted", 'import re\n\nCHAT_ID = "^oc_[A-Za-z0-9_-]+$"\n\n\ndef pick(CHAT_ID):\n' @@ -818,7 +884,7 @@ def test_declared_site_count_is_enforced_in_both_directions() -> None: assert offender_rows(rows) == [] retired = [item for item in rows if item["file"] != declared_file] assert offender_rows(retired) == [ - f"{declared_file} declares 1 individual shape site(s), found 0" + f"{declared_file} declares 2 individual shape site(s), found 0" ] From 3ebb47000163fd0f33701af46fa837582b29c09c Mon Sep 17 00:00:00 2001 From: Hsuehtan <296098438+Hsuehtan@users.noreply.github.com> Date: Wed, 7 Oct 2026 20:18:41 +0800 Subject: [PATCH 2/2] fix(lark): keep the app-id guard's gates honest Three couplings showed up when the shape was actually run, not assumed: - The unfoldable layer is gated on identifier field names. Keying it on the cli_ prefix instead dragged in a setup-URL pattern and a markdown heading built from data, exactly the noise the layer's comment warns about, so the token is the field name app_id. - An alias import does create a module-level binding, so the consumer assertion that rejected one was wrong; object identity is the wiring proof. - The declared-site fixture in the staleness test carried the old budget of one construction; with the declared file now holding two, it has to state both. Signed-off-by: Hsuehtan <296098438+Hsuehtan@users.noreply.github.com> --- .../test_lark_identity_shape_owner.py | 33 ++++++++++++++++--- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/tests/architecture/test_lark_identity_shape_owner.py b/tests/architecture/test_lark_identity_shape_owner.py index d00536e56e..dfe96c195e 100644 --- a/tests/architecture/test_lark_identity_shape_owner.py +++ b/tests/architecture/test_lark_identity_shape_owner.py @@ -99,11 +99,14 @@ "oc_", "om_", "ou_", - "cli_", "chat_id", "message_id", "operator_id", "event_id", + # The application id is gated on its field name, not on the ``cli_`` prefix: + # ``cli_`` also occurs in this package as a binary name in data (``lark-cli``, + # ``cli_bin``), which drags in regexes built from URLs and markdown headings + # and turns the declaration layer into the noise its comment warns about. "app_id", ) @@ -602,9 +605,11 @@ def test_inbox_callers_still_receive_one_object_through_the_chain() -> None: @pytest.mark.parametrize("name", sorted(APP_ID_CALLERS)) def test_every_app_id_caller_holds_the_owners_object(name: str) -> None: + # Identity is the wiring proof: an alias import hands on the owner's object, + # while a module that recompiled the body would hold a distinct one even though + # the pattern text matched. module = __import__(f"loopx.extensions.lark.{name}", fromlist=["APP_ID_PATTERN"]) assert module.APP_ID_PATTERN is identity_shapes.LARK_APP_ID_PATTERN, name - assert "APP_ID_PATTERN" not in module_level_bindings(module), name def test_the_delivery_contract_holds_the_owners_object_too() -> None: @@ -617,8 +622,19 @@ def test_the_delivery_contract_holds_the_owners_object_too() -> None: @pytest.mark.parametrize( "value", - ["cli_ok1", "cli_ok1\n", "\ncli_ok1", "cli_a\nb", "", "cli_", "cli_\u00e9", - "cli_a b", "xcli_a", "cli_a-b_1.C", "cli_a" + "z" * 200], + [ + "cli_ok1", + "cli_ok1\n", + "\ncli_ok1", + "cli_a\nb", + "", + "cli_", + "cli_\u00e9", + "cli_a b", + "xcli_a", + "cli_a-b_1.C", + "cli_a" + "z" * 200, + ], ) def test_the_app_id_answer_is_the_same_anchored_or_not(value: str) -> None: """Why gathering these sites cannot change a product answer. @@ -897,6 +913,15 @@ def test_a_converted_declared_site_is_not_silently_reintroduced() -> None: "identifier": "event_id", "anchored": False, }, + # The declared budget is two sites here (the event-id match plus this file's + # own ``cli_`` compile), so a fixture that clears the budget has to carry + # both; dropping either one is the failure this test is about. + { + "file": declared_file, + "line": 33, + "identifier": "app_id", + "anchored": False, + }, { "file": "loopx/extensions/lark/other_module.py", "line": 9,