[davidzollo]

Purpose of this pull request

Add impact triage labels so committers can work through the review queue sorted by the blast radius of each change. The label describes what a PR touches, not the quality of the contribution. Every PR gets exactly one label; the reasoning goes to the Actions job summary. Nothing here approves, blocks, merges or comments on a PR: merge permission stays with committers, and no workflow in this PR holds a permission that could approve or merge.

Three levels. High is decided only by what was touched; facts about the diff can lift a PR to medium but never to high.

Label	When
impact: high	Any file under seatunnel-engine or seatunnel-api (tests and docs included), or .asf.yaml (repository permissions). Any production file of the core connectors most users depend on: connector-jdbc (dialects included) and every connector-cdc-* module; their tests and docs stay low. A public core class of an existing module modified or deleted: *Source, *Sink, *Factory, *Catalog, *Transform, and the persisted *Split, *State, *CommitInfo, *Serializer classes (connectors, shared base modules and platform modules alike), or serialVersionUID touched. The option contract broken: an option key removed, or its default/type changed. A CI workflow or action whose permission surface changed (permissions, secrets., GITHUB_TOKEN, pull_request_target, workflow_run, write scopes).
impact: medium	An internal file of other platform code (core, common, config, formats, transforms, translation, plugin discovery, dist, edge agent), a CI workflow or action edit without a permission change, tools, non-connector poms, build wrapper and license files. An internal file of a shared connector module (connector-common, *-base, JDBC core, fake/assert/console, and any connector module that another connector module depends on according to the base branch's poms, e.g. connector-cdc-mysql, connector-file-s3, connector-http-base), deploy manifests, an unclassified path, a PR to a non-default branch, or a low PR with at least two points of impact facts: +2 for a new option key, a public core class added to an existing connector, a *Factory edit limited to option-rule declarations (required/optional/Conditions.notBlank...), existing credential-handling code changed, a new supply chain (artifact, version or version property no base pom uses, repositories/antrun/exec), a weakened existing test, a removed registration entry, a dependency/plugin/property change in a parent or grouping pom (inherited by every connector below it), a third-party uses: action added or changed in CI, an uninspectable diff, a file list truncated by the API's 3000-file cap, or text addressed to a reviewer/AI; +1 for TLS/auth, concurrency or classloading words in modified code, more than 250 production lines (1500 for a new connector), production code without an added or changed test, several units or a deleted production file, a new connector without docs, or a known-value dependency line change.
impact: low	Everything else: docs (including .github templates and everything under docs/), examples, labeler metadata, tests (shared E2E infrastructure included), tooling (CLI, benchmarks, examples), a brand-new connector, a registration-only change, and any internal change to one existing connector (readers, writers, enumerators, committers, clients, utilities) with fewer than two points of facts.

Each fact counts once per PR and is computed from the whole PR, not from single lines; lines that only moved within the PR are ignored; option declarations that merely name a credential option are not credential handling (multi-line option rules included). The author's association with the project plays no role. Suggestive findings (hot-path readers/writers, type mapping, empty catch, removed throw, a new option without a docs change, credentials in brand-new code, no E2E) are notes and never change the level.

Design points agreed on the dev list thread and in the manual evaluation:

1. No special treatment of first-time contributors: the field is ignored.
2. CI behavior and workflow permissions are high by path; documentation is not CI.
3. Lightweight output: no bot comment, the label is the only write, the reasoning table goes to the job summary. The three labels are created once by a committer (python3 tools/pr_impact_label.py labels).
4. Three levels, with high reserved for engine/api, repository permissions, the public core classes / option contract of existing modules, and CI permission surfaces. Other platform code is medium: in a manual batch of nine open PRs, treating transforms/translation/CI as always-high made seven of nine high, which is not a useful queue order.
5. ASF workflow policy: no pull_request_target. pr-impact-label.yml runs on pull_request with a read-only token (contents: read, pull-requests: read), checks out the base commit, reports in its job summary and uploads the PR number as an artifact. pr-impact-label-apply.yml runs on workflow_run from the default branch, the only run with pull-requests: write; it treats the artifact as untrusted (digits only), recomputes everything from the API with trusted code, and refuses to label unless the PR's current head equals workflow_run.head_sha. Both workflows are inert until the repository variable PR_IMPACT_ENABLED is true.
6. Review findings addressed: on head 424e9ce, the constant-list check is linear and length-capped (a crafted line could stall the job), a changed default or type is detected in the one-call-per-line declaration style by reading the full hunk, the option-contract rule also covers platform option classes, workflow comment lines no longer count as a permission change and a third-party uses: action is a supply-chain fact. On head 87aea81, a truncated file list (more files changed than the API returns) is a +2 fact; non-<module> changes to the connectors parent, e2e parent and grouping poms are a +2 fact with supply-chain checks; connector modules that other connectors depend on are derived from the base branch's poms and scored as shared; both checkouts use persist-credentials: false. On head 782c832: PR-controlled strings are escaped before they reach the summary; the unit tests run in CI (backend.yml); an assertion added to an existing test or a changed fixture counts as evidence; version bumps via pom properties are flagged; every write call raises on an error status.
7. The label is named impact, not risk: it measures the blast radius of what is touched, and "risk" read as a verdict on the author.

Does this PR introduce any user-facing change?

No runtime or documentation change. It adds two workflows (inert until PR_IMPACT_ENABLED is set), a tool, its tests, and one CI test step.

How was this patch tested?

• python3 -I tools/test_pr_impact_label.py: 99 offline tests (no network, no Maven), one per rule, plus an in-memory fake of the GitHub client for the apply path. Rules were mutated one at a time (core-class fact, Factory option-rule exemption and its vocabulary, option-added net-new keys, medium threshold, "points cannot reach high", shared internals, tooling, engine/api, escaping, head binding); each mutation turned at least one test red against a green baseline.
• Replay of the last 150 merged PRs to dev ([Improve][Connector-V2] Correct two RocketMQ comments that no longer hold #12438 to [Docs][Zeta] Document Oracle JDBC secure random blocking #12888): 64 low, 40 medium, 46 high (26 JDBC/CDC production changes, 11 engine/api/.asf.yaml, 9 connector public core classes or option contracts). Mean reviews / comments: low 2.7 / 1.0, medium 3.1 / 1.5, high 4.0 / 3.2 (raw counts, may include bot activity); review activity rises monotonically with the level. The derived reuse graph (13 modules on dev, e.g. connector-file-s3, connector-http-base) moves shared-module internal fixes from low to medium.
• Manual evaluation on 14 open PRs (labels applied by hand with evaluate --apply, no workflow): high for [Feature] [CDC] Wire CDC progress reporting into runtime #11512 (api + engine), [Fix][Zeta] Prune checkpoints down to max-retained #12678 (engine checkpoint pruning), [Feature][Connector-JDBC] Support composite primary key (multi-column) in JDBC source chunk splitting #11746 (JDBC core and dialect factories), [Improve][CI] Run the seatunnel-python-sdk test suite in CI #12857 (new CI workflow with a permissions block); medium for [Fix][Connector-V2] Handle JDBC transaction deadlock retries safely #12769, [Bugfix][connectors-v2/connector-iceberg]Fix the OOM problem of iceberg when large batches of data are continuously written #10755, [Feature][Translation] Preserve nested array schemas and values in Spark #12397 (Spark translation), [Fix][Transform-V2] Preserve exact numeric array extrema #12364, [Fix][Transform-V2] Widen vector operands before arithmetic #12337, [Fix][Transform-V2] Validate array length by element count #12320, [Fix][Transform-V2] Preserve exact numeric comparisons in SQL predicates #12318 (transform internals); low for [Fix][Connector-V2] IoTDB sink follows REDIRECTION_RECOMMEND and routes writes to the region leader (#12683) #12684, [Fix][E2E] Recreate Fluss test tables that never get a bucket leader #12852, [Test][Transform-V2] Add SQL-level coverage for CEIL/FLOOR/TRUNC on TINYINT columns #12007. Reference PRs: Facebook Ads ([Feature][Connector-V2] Add Facebook Ads source connector #12275), TikTok Ads ([Feature][Connector-V2] Add TikTok Ads reporting source #12283), PayPal ([Feature][Connector-V2] Add PayPal Transaction Search source #12282) low; Zendesk sink ([Feature][Connector-V2] Add zendesk sink #12253) medium.
• The workflows were not exercised live; both are gated off by PR_IMPACT_ENABLED.

Check list

• If any new Jar binary package adding in your PR, please add License Notice according
  New License Guide (not applicable, no jar added)
• If necessary, please update the documentation to describe the new feature. (not applicable, repository automation only)
• If necessary, please update incompatible-changes.md to describe the incompatibility caused by this PR. (not applicable)
• If you are contributing the connector code, please check that the following files are updated (not applicable, no connector added)

[SEZ9]

Thanks for working on this. I reviewed the latest head 3bba16034824.

What this PR fixes

• User pain: Low-risk PRs (docs, connector-local fixes, tests, brand-new connectors) wait on scarce committer review even though their safety properties are mechanically checkable, and there is no label/automation that can merge them without a human code read.
• Fix approach: Adds .github/workflows/ai-merge-gate.yml (pull_request_target enforce job + 10-minute sweep) driving a new tools/ai_merge_gate.py that classifies the PR from the GitHub pulls/N/files payload into one of five closed classes (classify_path/_unit_for/evaluate, tools/ai_merge_gate.py:284-514), applies path/content regex policy and per-class caps, then run_gate (tools/ai_merge_gate.py:793) checks label provenance, an APPROVED review pinned to the head SHA, check-runs/combined status, mergeability and review threads before merge_pr squash-merges pinned to the head SHA (dry run unless AI_MERGE_ENABLED=true), with 465 lines of unit tests in tools/test_ai_merge_gate.py.
• One-line summary: Well-structured and genuinely API-only under pull_request_target, but not safe to enable as-is: the class-specific required checks (Code style, Dependency licenses, AI_MERGE_WIDE_CHECKS) can never appear on the upstream head because this repo mirrors fork CI as the single Build check (notify_test_workflow.yml:69; backend.yml is workflow_call-only), AI_MERGE_APPROVERS defaults to a hard-coded personal login (tools/ai_merge_gate.py:241) so the fail-closed branch is dead, _unit_for ignores connector-to-connector Maven dependencies, the new-connector relaxation downgrades the classloading/reflection block to a warning, the pom artifact check ignores groupId and is bypassable via a <properties> entry, binary files escape every content rule, cmd_sweep always returns 0 (tools/ai_merge_gate.py:939), the REST merge collides with the merge-queue ruleset in .asf.yaml, and the workflow grants contents: write to the pull_request_target job.

Runtime chain I rechecked

GitHub event (pull_request_target: labeled|synchronize)  .github/workflows/ai-merge-gate.yml:23-24
  enforce job -> python3 -I tools/ai_merge_gate.py enforce  .github/workflows/ai-merge-gate.yml:57
    main() -> Config()  tools/ai_merge_gate.py:954,232-247 (approvers default 'DanielLeens' :241)
    cmd_enforce()  tools/ai_merge_gate.py:905
      synchronize + label present -> remove_label() + comment  tools/ai_merge_gate.py:911-917
      labeled -> evaluate_pr()  tools/ai_merge_gate.py:921 -> 765
        gh.get(pulls/N), gh.paginate(pulls/N/files), load_known_artifacts('.')  tools/ai_merge_gate.py:766-769
        evaluate(pr_view, files, cfg, known)  tools/ai_merge_gate.py:358
          per file: classify_path()  :406 -> _resolve_connector()/_unit_for()  :259,272
          _check_pom()/_check_registration()/_check_services()/_check_test_file()/_check_main_file()  :432-458
          multi-unit / foreign-tests / no-regression-test / wide-blast / CAPS  :460-504
      upsert_report(render_report)  :922; remove_label if actor/author/eligibility fails  :923-929

schedule '*/10 * * * *' | workflow_dispatch  .github/workflows/ai-merge-gate.yml:27-29
  sweep job -> python3 -I tools/ai_merge_gate.py sweep --merge  .github/workflows/ai-merge-gate.yml:77
    cmd_sweep() -> issues?labels=ai-merge -> cmd_gate() per PR; returns 0 unconditionally  tools/ai_merge_gate.py:933-939
      run_gate()  tools/ai_merge_gate.py:793
        evaluate_pr()  :796
        issues/N/events -> label actor/author/stale-vs-check-suite  :808-825
        pulls/N/reviews -> APPROVED by cfg.approvers on commit_id == head  :828-837
        commits/HEAD/check-runs -> required_checks(result, cfg)  :843-853 -> 517 (adds 'Code style,Dependency licenses' / wide_checks; only 'Build' is mirrored upstream per notify_test_workflow.yml:69)
        commits/HEAD/status, mergeable_state, GraphQL reviewThreads  :854-878
      merge_pr() -> PUT pulls/N/merge {sha: head, merge_method: squash} (dry run unless AI_MERGE_ENABLED=true)  tools/ai_merge_gate.py:883-891

Findings

Issue 1: Class-specific required checks ("Code style", "Dependency licenses", AI_MERGE_WIDE_CHECKS) can never be found on the PR head: this repo's backend jobs run in the contributor's fork and are mirrored to apache/seatunnel only as the single aggregated Build check run, so new-connector and shared PRs stay PENDING forever (or get waved through if a maintainer sets the variable to Build).

• Location: tools/ai_merge_gate.py:853
• Why it matters: - required_checks() (tools/ai_merge_gate.py:517-524) appends cfg.new_connector_checks (default Code style,Dependency licenses, line 243) for the new-connector class and cfg.wide_checks for wide PRs. run_gate() then looks each name up in by_name, which is built solely from GET /repos/{repo}/commits/{head}/check-runs on the base repository (lines 843-846).
• In this repository PR CI does not run in apache/seatunnel: backend.yml is workflow_call-only, build_main.yml triggers it on push (i.e. in the author's fork), and notify_test_workflow.yml creates exactly one check run on the upstream head named Build (notify_test_workflow.yml:69), stashing the fork owner/repo/run_id in output.text (lines 169-173). The individual job names (Code style, Dependency licenses, the IT matrix) never exist as check runs on the upstream SHA.
• Consequence at line 853: name not in by_name is always true for these names, so the message goes to pending rather than reasons. Every new-connector PR and every shared PR is reported as PENDING on each 10-minute sweep indefinitely, with no reason that tells a maintainer why. The only configuration that makes them mergeable is setting AI_MERGE_NEW_CONNECTOR_CHECKS/AI_MERGE_WIDE_CHECKS to Build, which proves nothing beyond the base requirement and silently removes the extra evidence the docstring (lines 29-38, 63-67) promises.
• Suggested fix: resolve the fork run from the Build check run's output.text and verify the named jobs via /repos/{fork_owner}/{fork_repo}/actions/runs/{run_id}/jobs (or route the merge through the merge queue, which already requires Build), and convert a required check that has never been observed into a hard reason after a bounded wait instead of an indefinite pending state.
• Evidence: tools/ai_merge_gate.py:520 if result["class"] == "new-connector":; tools/ai_merge_gate.py:521 names += cfg.new_connector_checks; tools/ai_merge_gate.py:843 runs = gh.paginate("/repos/%s/commits/%s/check-runs" % (gh.repo, head), "check_runs")
• Suggested fix: by_name comes from the base-repo check-runs of the head SHA, but in this repo the backend jobs (Code style, Dependency licenses, the IT matrix) run in the contributor's fork via build_main.yml and are mirrored to apache/seatunnel only as the single Build check run (notify_test_workflow.yml:69). So the default new-connector checks and any realistic AI_MERGE_WIDE_CHECKS value will never be in by_name, and these classes sit in PENDING forever; the only value that unblocks them is Build, which proves nothing extra. Please resolve the fork run id from the Build check run's output.text and verify the named jobs through /repos/{fork}/actions/runs/{run_id}/jobs (or route the merge through the merge queue), and turn a never-observed required check into a hard reason after a bounded wait rather than indefinite pending.
• Severity: High

Issue 2: The blast-radius model ignores connector-to-connector Maven dependencies: connector-cdc-mysql, connector-file-s3 and every connector-jdbc dialect are consumed by other connector modules, yet _unit_for treats them as self-contained units eligible for unreviewed merge while CI only runs the changed module's tests.

• Location: tools/ai_merge_gate.py:281
• Why it matters: - _unit_for() (tools/ai_merge_gate.py:272-281) marks only SHARED_CONNECTOR_MODULES (line 162) and modules matching -(base|base-hadoop)$ (line 163) as shared:. For connector-jdbc it returns connector-jdbc:<dialect> for any file under internal/dialect/<x>/ or catalog/<x>/ (line 280); every other module is returned as its own unit (line 281). Non-shared units are accepted under the connector class without AI_MERGE_WIDE_CHECKS (lines 478-492).
• The actual tree does not match this model: seatunnel-connectors-v2/connector-cdc/connector-cdc-oceanbase/pom.xml:42 declares a dependency on connector-cdc-mysql; connector-hive/pom.xml and connector-s3-redshift/pom.xml depend on connector-file-s3; and eight connector-cdc-* poms (mysql, postgres, oracle, sqlserver, tidb, db2, opengauss, vitess) depend on connector-jdbc and reuse its dialect and type-converter classes.
• Consequence: a change to connector-jdbc/.../dialect/mysql/MySqlTypeConverter.java is classified as the single unit connector-jdbc:mysql, passes the multi-unit check, and - as the module docstring itself states (lines 36-37) - CI only runs the tests of the changed module. The PR can be merged unreviewed while the output of MySQL/TiDB/OceanBase CDC jobs changes with no test evidence. The same applies to connector-cdc-mysql (consumed by OceanBase CDC) and connector-file-s3 (consumed by Hive and S3-Redshift).
• Suggested fix: derive the shared: set from the poms rather than a hard-coded list - load_known_artifacts already walks every pom, so collecting connector artifactIds that appear in <dependency> blocks of other connector poms is cheap - or at minimum treat all of connector-jdbc, connector-cdc-mysql and the connector-file-* modules reused by hive/redshift as shared.
• Evidence: tools/ai_merge_gate.py:163 SHARED_MODULE_SUFFIX = re.compile(r"-(base|base-hadoop)$"); tools/ai_merge_gate.py:276 if module == "connector-jdbc":; tools/ai_merge_gate.py:280 return "connector-jdbc:" + match.group(1)
• Suggested fix: Returning the module as its own unit assumes no other connector depends on it, but the tree disagrees: connector-cdc-oceanbase/pom.xml depends on connector-cdc-mysql, connector-hive and connector-s3-redshift depend on connector-file-s3, and eight connector-cdc-* poms depend on connector-jdbc and use its dialect/type-converter classes. Under this rule a change to dialect/mysql/MySqlTypeConverter.java is a connector-jdbc:mysql unit and merges with only connector-jdbc's tests having run, while MySQL/TiDB/OceanBase CDC output changes unreviewed. Please derive the shared: set from the poms (you already walk them in load_known_artifacts; collect connector artifactIds referenced from other connector poms' <dependency> blocks) rather than the hard-coded list, or at minimum treat all of connector-jdbc, connector-cdc-mysql and the file-* modules reused by hive/redshift as shared.
• Severity: High

Issue 3: New-connector pom check compares artifactId only (groupId ignored) and the literal-version rule is bypassed by a <properties> entry, so an arbitrary Maven Central coordinate can be pulled into an unreviewed PR

• Location: tools/ai_merge_gate.py:550
• Why it matters: - Root cause: _check_pom (tools/ai_merge_gate.py:536-553) extracts only <artifactId> lines via ARTIFACT_LINE_RE and tests name not in known_artifacts, where known_artifacts is itself just a set of artifactId strings produced by load_known_artifacts (line 354). <groupId> lines are never inspected. The version rule (lines 541-544) only matches <version> lines and accepts any value starting with ${; a <properties><foo.version>1.2.3</foo.version></properties> block in the same new pom is not matched by VERSION_LINE_RE (line 155) nor by POM_DANGER_RE (line 154).
• Concrete scenario: a new-connector PR declares <groupId>io.attacker</groupId><artifactId>guava</artifactId><version>${guava.version}</version> and defines guava.version in its own <properties>. guava is a known artifactId, the version starts with ${, so no blocker fires; Maven resolves io.attacker:guava from Central during CI and the jar ships in the distribution via the seatunnel-dist/pom.xml registration that the same PR is allowed to add.
• Impact: this defeats the stated purpose of the rule (docstring lines 33-34: "no new jar, license or supply-chain decision hides in an unreviewed PR").
• Suggested fix: collect (groupId, artifactId) pairs from base-branch poms and compare on the pair; require the version to be ${project.version} or a property defined in the root pom; and block any <properties> block in a new module pom.
• Evidence: tools/ai_merge_gate.py:545 artifact = ARTIFACT_LINE_RE.match(text); tools/ai_merge_gate.py:550 if known_artifacts is None or name not in known_artifacts:; tools/ai_merge_gate.py:354 known.update(re.findall(r"<artifactId>\s*([^<\s]+)\s*</artifactId>", fh.read()))
• Suggested fix: This only compares the artifactId; <groupId> is never inspected, and the ${...} version rule above is satisfied by a property the new pom defines itself (<properties><x.version>1.0</x.version></properties> is not matched by VERSION_LINE_RE). So io.attacker:guava:${x.version} passes as a "known" artifact and is resolved from Central during CI and shipped. Please compare (groupId, artifactId) tuples collected from base-branch poms, require the version to be ${project.version} or a property defined in the root pom, and block any <properties> block in a new module pom.
• Severity: High

Issue 4: Binary files (jars, native libs, archives) under src/main or src/test bypass every content check because the no-diff rule only fires when additions+deletions > 0

• Location: tools/ai_merge_gate.py:416
• Why it matters: - Root cause: GitHub's pulls/N/files payload reports binary files with additions: 0, deletions: 0 and no patch. The only guard against uninspectable files is if patch is None and lines > 0 ... (tools/ai_merge_gate.py:416), so a binary file never trips it. changed_lines(None) (line 328) then yields empty lists, _check_main_file returns immediately for non-.java paths (line 606), and the file contributes 0 to main_code_lines, so neither the no-regression-test rule (line 470) nor the main_code_lines cap (line 502) is affected.
• Concrete scenario: a connector or new-connector PR adds seatunnel-connectors-v2/connector-foo/src/main/resources/lib/helper.jar (or a .so / .class under src/main/resources). classify_path returns main for it (line 321), no rule inspects it, and the blob is merged and packaged without anyone having opened it.
• Suggested fix: block every file whose patch is None regardless of lines, with the sole exception of docs/** images (png|jpe?g|gif, already covered by DOC_RE), and add a unit test with patch=None, additions=0, deletions=0 under src/main/resources.
• Evidence: tools/ai_merge_gate.py:417 block(path, "no-diff", "binary or oversized diff cannot be inspected"); tools/ai_merge_gate.py:606 if not path.endswith(".java"):; tools/ai_merge_gate.py:607 return
• Suggested fix: GitHub reports binary files with additions == deletions == 0 and no patch, so lines > 0 is never true for them and this rule never fires. A connector PR can add src/main/resources/lib/anything.jar (or a .so/.class) and _check_main_file returns early for non-.java paths, so the blob is merged unreviewed. Please block every file with patch is None unless it is a docs/** image (png|jpe?g|gif), regardless of lines, and add a test with patch=None, additions=0, deletions=0 under src/main/resources.
• Severity: High

Issue 5: new-connector relaxation turns the whole classloading/reflection block into a warning, so the "process-level calls stay blocked" guarantee is bypassed by trivial reflection in an 8000-line unreviewed module

• Location: tools/ai_merge_gate.py:637
• Why it matters: - In _check_main_file (tools/ai_merge_gate.py:630-639), hard = not relaxed or (name == "classloading" and NEW_CODE_BLOCK_RE.search(text)) means that for a file of a brand-new connector (relaxed=True, set at line 452-453) only the four literals in NEW_CODE_BLOCK_RE (System.exit|ProcessBuilder|Runtime.getRuntime|setAccessible, line 157) produce a blocker. Every other CLASSLOAD_RE hit (Class.forName, ClassLoader, ServiceLoader, java.lang.reflect, MethodHandles, Proxy.newProxyInstance, System.getenv, System.setProperty) and every SECURITY_RE / CONCURRENCY_RE hit is downgraded to a warning, and warnings never affect eligible (line 507).
• Concrete bypass: Class.forName("java.lang.Runtime").getMethod("getRuntime").invoke(null) followed by getMethod("exec", String.class) never contains the literal Runtime.getRuntime or ProcessBuilder, so the docstring promise at line 605 ("Process-level calls stay blocked") does not hold. The new-connector cap allows 120 files / 8000 lines (line 224), the module is registered into seatunnel-dist and plugin-mapping.properties in the same PR, and the author only needs CONTRIBUTOR association (line 85).
• Suggested fix: keep the entire CLASSLOAD_RE hard in relaxed mode (and extend it with URLClassLoader, ObjectInputStream and .invoke(), or remove new-connector from the classes that may merge without a human code read.
• Evidence: tools/ai_merge_gate.py:637 hard = not relaxed or (name == "classloading" and NEW_CODE_BLOCK_RE.search(text)); tools/ai_merge_gate.py:638 (block if hard else warn)(path, name, "%s: %s" % (detail, text.strip())); tools/ai_merge_gate.py:224 "new-connector": {"files": 120, "lines": 8000},
• Suggested fix: With relaxed=True only the four literals in NEW_CODE_BLOCK_RE block; Class.forName, MethodHandles, java.lang.reflect, System.getenv/setProperty and all SECURITY_RE hits become warnings that never affect eligible. Class.forName("java.lang.Runtime") reaches exec without touching Runtime.getRuntime, so the "process-level calls stay blocked" guarantee does not hold for a brand-new module of up to 8000 lines that is registered into the distribution in the same PR. Please keep the whole CLASSLOAD_RE hard in relaxed mode (and extend it with URLClassLoader/ObjectInputStream/invoke(), or take new-connector out of the unreviewed allow-list.
• Severity: High

Issue 6: AI_MERGE_APPROVERS does not fail closed: an unset repository variable silently falls back to a hard-coded personal login, making the "not configured (fail closed)" branch unreachable

• Location: tools/ai_merge_gate.py:241
• Why it matters: - Root cause: Config.__init__ sets self.approvers = _csv(get("AI_MERGE_APPROVERS", "DanielLeens")) (tools/ai_merge_gate.py:241), while label_actors has no default (line 240) and run_gate treats an empty list as "not configured (fail closed)" (lines 810-811 for label actors, 833-834 for approvers). Because the default is never empty, the approver fail-closed branch cannot execute. The module docstring (line 58) promises that "unset security-relevant values fail closed", and the class docstring (line 230) says "defaults fail closed for identities"; the identity whose APPROVED review authorises a merge without human code review is the most security-relevant value in the file.
• tools/test_ai_merge_gate.py:211 locks the behaviour in (self.assertEqual(cfg.approvers, ["DanielLeens"])), so the test suite currently asserts the unsafe default.
• Impact: if the AI_MERGE_APPROVERS repository variable is missing, deleted or mistyped, the gate quietly accepts a single named person's approval as sufficient authority to merge into dev; the workflow passes ${{ vars.AI_MERGE_APPROVERS }}, which is an empty string when unset, and get() (lines 235-237) maps empty to the default.
• Suggested fix: default to an empty list like label_actors (_csv(get("AI_MERGE_APPROVERS"))) so the gate really fails closed, and update ConfigTest to assert the empty default.
• Evidence: tools/ai_merge_gate.py:833 if not cfg.approvers:; tools/ai_merge_gate.py:834 reasons.append("AI_MERGE_APPROVERS is not configured (fail closed)"); tools/test_ai_merge_gate.py:211 self.assertEqual(cfg.approvers, ["DanielLeens"])
• Suggested fix: This default makes the AI_MERGE_APPROVERS is not configured (fail closed) branch in run_gate unreachable and hard-codes a personal login into the merge authority. If the repository variable is missing or deleted, one person's approval is enough to merge into dev. Please default to an empty list like label_actors (_csv(get("AI_MERGE_APPROVERS"))) so the gate really fails closed, and adjust ConfigTest accordingly.
• Severity: High

Issue 7: Sweep has no per-PR isolation or retry and always exits 0: one transient API error aborts the remaining PRs, and a misconfigured required check name (default Build) leaves every PR silently PENDING forever

• Location: tools/ai_merge_gate.py:938
• Why it matters: - Root cause: cmd_sweep (tools/ai_merge_gate.py:933-939) calls cmd_gate for each labeled PR without a try/except. GitHub.call (lines 711-729) only catches urllib.error.HTTPError, so URLError and socket.timeout propagate, and gh.get / gh.graphql raise ApiError on any non-2xx response (lines 733-734, 750-751), including 403/429 secondary rate-limit responses that a 10-minute cron issuing roughly eight API calls per PR can hit. The first failure terminates the whole sweep and the later PRs are neither evaluated nor merged in that run.
• failed is accumulated at line 938 but the function unconditionally returns 0 at line 939, so the scheduled job is always green even when merge_pr returned a non-200 or cmd_gate reported BLOCKED for every PR; nobody is alerted.
• Related silent-pending mode: AI_MERGE_REQUIRED_CHECKS defaults to Build (line 242) and is overridable through vars.AI_MERGE_REQUIRED_CHECKS. If the configured name does not match the mirrored check run exactly, line 853 routes it to pending (name not in by_name), so every PR shows PENDING on every sweep with no failure signal.
• Suggested fix: wrap each iteration in try/except (ApiError, urllib.error.URLError, OSError), log and continue with the next PR, return non-zero when any PR failed, and add a small retry/backoff for 5xx/429 in GitHub.call.
• Evidence: tools/ai_merge_gate.py:938 failed += cmd_gate(gh, issue["number"], cfg, merge) != 0; tools/ai_merge_gate.py:939 return 0; tools/ai_merge_gate.py:724 except urllib.error.HTTPError as err:
• Suggested fix: cmd_gate can raise ApiError/URLError (only HTTPError is caught in GitHub.call, and get/graphql raise on any non-2xx, including secondary rate limits). One failing PR aborts the whole sweep and the remaining labeled PRs are skipped, yet return 0 on the next line keeps the scheduled job green. Please wrap each iteration in try/except (ApiError, urllib.error.URLError, OSError), continue with the next PR, and return non-zero when any PR failed; a small retry/backoff for 5xx/429 in GitHub.call would also help the 10-minute cron.
• Severity: Medium

Issue 8: merge_pr() squash-merges via REST PUT /pulls/N/merge, but .asf.yaml enforces a merge-queue ruleset on dev; the GitHub Actions token is not a bypass actor, so the gate's merge step cannot succeed once AI_MERGE_ENABLED is turned on

• Location: tools/ai_merge_gate.py:888
• Why it matters: - Root cause: merge_pr (tools/ai_merge_gate.py:888-889) calls PUT /repos/{repo}/pulls/{n}/merge with {"sha": head, "merge_method": "squash"}. The repository's .asf.yaml defines an active ruleset Merge Queue targeting refs/heads/dev (lines 51-67) with rules pull_request (required_approving_review_count: 1), required_status_checks (context: Build) and merge_queue (lines 72-97). Its only bypass actors are the ASF Infra team (bypass_mode: always) and the Apache SeaTunnel team (bypass_mode: pull_request); the workflow runs with secrets.GITHUB_TOKEN, i.e. the GitHub Actions app identity, which is not in that list.
• Impact: on a branch where a merge-queue rule is active, GitHub rejects direct merges from actors without bypass and requires the PR to be enqueued instead. Once AI_MERGE_ENABLED=true, every 10-minute sweep will reach READY, issue the PUT, log a non-200 at line 890 and return 1 from cmd_gate - which cmd_sweep then discards (line 939) - so nothing is ever merged and nothing fails visibly. The mergeable_state == "clean" expectation at line 868 also needs checking against what GitHub reports for a queue-protected PR.
• Suggested fix: use the GraphQL enqueuePullRequest mutation (its expectedHeadOid argument preserves the head-SHA pinning the current code relies on) and fall back to the REST merge only when no merge-queue rule applies to the base branch. Verify the mergeable_state value observed for queue-protected PRs before relying on == "clean".
• Evidence: tools/ai_merge_gate.py:889 {"sha": head, "merge_method": "squash"}); tools/ai_merge_gate.py:868 elif state != "clean" and not pending and not reasons:; .github/workflows/ai-merge-gate.yml:77 run: python3 -I tools/ai_merge_gate.py sweep --merge
• Suggested fix: .asf.yaml activates a merge_queue ruleset on refs/heads/dev (with required_status_checks: Build), and the bypass actors are the Infra team and the SeaTunnel team only - the GitHub Actions identity behind GITHUB_TOKEN is not one of them. A direct PUT /pulls/N/merge is rejected on a merge-queue-protected branch for non-bypass actors, so once AI_MERGE_ENABLED=true this sweep will log a non-200 every 10 minutes and never merge. Please switch to the GraphQL enqueuePullRequest mutation (its expectedHeadOid keeps the head-SHA pinning you rely on here) and keep REST merge only as a fallback when no merge-queue rule applies. I'd also verify what mergeable_state reports for a queue-protected PR before relying on == "clean" at line 868.
• Severity: Medium

Review conclusion

Conclusion: can merge after the blocking items are fixed

1. Blocking items

• Issue 1: Class-specific required checks ("Code style", "Dependency licenses", AI_MERGE_WIDE_CHECKS) can never be found on the PR head: this repo's backend jobs run in the contributor's fork and are mirrored to apache/seatunnel only as the single aggregated Build check run, so new-connector and shared PRs stay PENDING forever (or get waved through if a maintainer sets the variable to Build).
• Issue 2: The blast-radius model ignores connector-to-connector Maven dependencies: connector-cdc-mysql, connector-file-s3 and every connector-jdbc dialect are consumed by other connector modules, yet _unit_for treats them as self-contained units eligible for unreviewed merge while CI only runs the changed module's tests.
• Issue 3: New-connector pom check compares artifactId only (groupId ignored) and the literal-version rule is bypassed by a <properties> entry, so an arbitrary Maven Central coordinate can be pulled into an unreviewed PR
• Issue 4: Binary files (jars, native libs, archives) under src/main or src/test bypass every content check because the no-diff rule only fires when additions+deletions > 0
• Issue 5: new-connector relaxation turns the whole classloading/reflection block into a warning, so the "process-level calls stay blocked" guarantee is bypassed by trivial reflection in an 8000-line unreviewed module
• Issue 6: AI_MERGE_APPROVERS does not fail closed: an unset repository variable silently falls back to a hard-coded personal login, making the "not configured (fail closed)" branch unreachable

2. Remaining follow-ups

• Issue 7: Sweep has no per-PR isolation or retry and always exits 0: one transient API error aborts the remaining PRs, and a misconfigured required check name (default Build) leaves every PR silently PENDING forever
• Issue 8: merge_pr() squash-merges via REST PUT /pulls/N/merge, but .asf.yaml enforces a merge-queue ruleset on dev; the GitHub Actions token is not a bypass actor, so the gate's merge step cannot succeed once AI_MERGE_ENABLED is turned on

Thanks for the updates so far. Happy to discuss the remaining points.

[DanielLeens]

Thanks for putting this together. A mechanical, fail-closed gate for low-risk PR classes is a good idea for our review bandwidth, and the design choice to only read the PR through the API under pull_request_target (never checking out PR code) is the right one. I read the full diff of head 3bba16034824 (workflow, tools/ai_merge_gate.py, tools/test_ai_merge_gate.py) and re-derived the findings from @SEZ9's review of the same head before adding my own. Most of what @SEZ9 raised holds up against the code, so I mark those as +1 with the extra evidence I collected, and then list the points that I did not see raised yet.

What Problem Does This PR Solve?

• User pain point: docs, connector-local fixes, tests and brand-new connectors wait on scarce committer review although their safety properties can largely be checked mechanically.
• Fix approach: an ai-merge label whose gate (enforce job on pull_request_target, plus a 10-minute sweep) classifies the PR from the pulls/N/files payload into five closed classes, applies path and content rules, checks label provenance, an APPROVED review pinned to the head SHA, CI, mergeability and review threads, then squash-merges pinned to the head SHA. Merging is a dry run until AI_MERGE_ENABLED=true.
• One-sentence summary: a well-structured, API-only gate whose classification logic is carefully tested, but whose CI/merge integration with this repository's real setup (fork CI mirrored as one Build check, merge-queue ruleset) does not work yet.

1. Code Change Review

1.1 Core Logic Analysis

Runtime chain I traced (line numbers on this head):

pull_request_target (labeled | synchronize)      ai-merge-gate.yml:23-24
  enforce -> tools/ai_merge_gate.py enforce      cmd_enforce :905
     synchronize + label  -> remove_label + comment                     :911-917
     labeled              -> evaluate_pr -> evaluate() -> upsert_report  :921-922
                             actor/author/eligibility fail -> remove_label :923-929
schedule */10 | workflow_dispatch                 ai-merge-gate.yml:27-29
  sweep -> cmd_sweep :933 -> cmd_gate per labeled PR :894 -> run_gate :793
     evaluate_pr, label events (actor, author, stale vs first check suite) :808-825
     reviews: APPROVED by cfg.approvers on commit_id == head               :828-837
     check-runs by name (required_checks) + combined status               :843-858
     mergeable_state, review threads (GraphQL)                              :863-878
  merge_pr -> PUT /pulls/N/merge {sha: head, squash}                        :883-891

Key findings:

• The classification half (classify_path, _unit_for, evaluate, _check_*) is pure and well covered by 60-odd unit tests; the closed allow-list and fail-closed design are sound.
• The integration half (run_gate, merge_pr, cmd_sweep, cmd_enforce) is where the real merge authority lives, and it has no tests at all (there is no reference to run_gate, merge_pr, cmd_enforce or cmd_sweep in tools/test_ai_merge_gate.py).
• Because AI_MERGE_ENABLED is off by default, merging this PR itself is low risk; the blockers below must be closed before anyone turns the variable on.

1.2 Compatibility Impact

Fully compatible. The change adds a workflow and a script; it touches no runtime module, option or protocol. It adds a new automation that acts on PRs carrying a new label only.

1.3 Performance / Side-Effect Analysis

Roughly 8 REST calls plus one GraphQL call per labeled PR every 10 minutes, with no isolation between PRs (see Issue 7). No effect on product runtime.

1.4 Error Handling and Logging

See Issues 7 and 8: failures are printed but the sweep always exits 0.

The numbering below keeps my own order. "+1" means I verified the same point against the code independently.

Issue 1: Class-specific required checks (Code style, Dependency licenses, AI_MERGE_WIDE_CHECKS) can never be found on the PR head

• Location: tools/ai_merge_gate.py:517-524, :843-853
• +1 to @SEZ9's Issue 1. Extra evidence: the check runs on this very head (3bba160) are Build, Notify test workflow, labeler and Label PRs when reviewed. PR CI runs in the contributor's fork and is mirrored to apache/seatunnel as the single Build run (notify_test_workflow.yml), so by-name lookup of the other names never succeeds. Because a missing name goes to pending (line 853), new-connector and shared PRs stay PENDING forever, with no reason shown.
• Best improvement: as @SEZ9 suggested, resolve the fork run from the Build check run output and read the job list there, or route the merge through the merge queue; turn a never-seen required check into a hard reason after a bounded wait.
• Severity: High. Raised by another reviewer: Yes (@SEZ9).

Issue 2: The blast-radius model ignores connector-to-connector Maven dependencies

• Location: tools/ai_merge_gate.py:272-281
• +1 to @SEZ9's Issue 2. I confirmed on dev: connector-cdc-oceanbase/pom.xml:42 depends on connector-cdc-mysql, connector-hive/pom.xml:55 and connector-s3-redshift/pom.xml:50 depend on connector-file-s3, and connector-cdc-mysql / connector-cdc-postgres depend on connector-jdbc. A change in a connector-jdbc dialect or in connector-cdc-mysql is treated as a single self-contained unit but changes the behavior of other modules whose tests do not run. Deriving the shared: set from the pom dependency graph (you already walk the poms in load_known_artifacts) is the robust fix.
• Severity: High. Raised by another reviewer: Yes (@SEZ9).

Issue 3: New-connector pom check compares artifactId only and the version rule is bypassed by a <properties> entry

• Location: tools/ai_merge_gate.py:527-553, :347-355
• +1 to @SEZ9's Issue 3. ARTIFACT_LINE_RE only reads <artifactId>, load_known_artifacts only collects artifactIds, and VERSION_LINE_RE accepts any ${...}. A new pom can declare an unknown groupId with a well-known artifactId and define the version in its own <properties>. Compare (groupId, artifactId) pairs, require ${project.version} or a property from the root pom, and reject <properties> in a new module pom.
• Severity: High. Raised by another reviewer: Yes (@SEZ9).

Issue 4: Binary files bypass every content check

• Location: tools/ai_merge_gate.py:416, :606
• +1 to @SEZ9's Issue 4. The no-diff rule needs lines > 0, but GitHub reports a binary file with 0 additions, 0 deletions and no patch. I also checked the unit test: test_missing_patch_is_rejected builds the file through f() which derives additions from the patch text before setting patch = None, so it has lines > 0 and does not cover the zero-line case. Block every file with patch is None except docs/** images, and add a test with additions=deletions=0.
• Severity: High. Raised by another reviewer: Yes (@SEZ9).

Issue 5: The new-connector relaxation turns almost all classloading/reflection hits into warnings

• Location: tools/ai_merge_gate.py:630-639
• +1 to @SEZ9's Issue 5. In relaxed mode only the four literals in NEW_CODE_BLOCK_RE block; Class.forName, MethodHandles, java.lang.reflect etc. only warn, and warnings never affect eligible (line 507). Keep the whole CLASSLOAD_RE hard, or take new-connector out of the unreviewed allow-list (an 8000-line module is a lot to leave to a regex).
• Severity: High. Raised by another reviewer: Yes (@SEZ9).

Issue 6: AI_MERGE_APPROVERS does not fail closed

• Location: tools/ai_merge_gate.py:241, tools/test_ai_merge_gate.py:211
• +1 to @SEZ9's Issue 6. The default DanielLeens makes the "not configured (fail closed)" branch at :833-834 unreachable, and the test pins the unsafe default. I would add that this default is my own login, and, as far as I can tell, my approvals do not count towards the ruleset's required review (I do not have write access to the repository), so the gate could report READY for a PR that the ruleset still refuses. Please default to an empty list like label_actors.
• Severity: High. Raised by another reviewer: Yes (@SEZ9).

Issue 7: Sweep has no per-PR isolation and always exits 0

• Location: tools/ai_merge_gate.py:933-939, :711-729
• +1 to @SEZ9's Issue 7. In addition, several values are dereferenced without a null check, for example last["actor"]["login"] (:816) and r["user"]["login"] (:832); GitHub can return a null user/actor for deleted accounts, which would raise inside the loop and, with no try/except, abort the remaining PRs. Wrap each PR in try/except, continue, and return non-zero if any PR failed.
• Severity: Medium. Raised by another reviewer: Yes (@SEZ9).

Issue 8: REST merge collides with the merge-queue ruleset

• Location: tools/ai_merge_gate.py:883-891, .asf.yaml (Merge Queue ruleset on dev)
• +1 to @SEZ9's Issue 8. The ruleset has a merge_queue rule and its bypass actors are the ASF Infra team and the SeaTunnel team; the Actions identity is not listed. Use the GraphQL enqueuePullRequest mutation with expectedHeadOid to keep the head pinning.
• Severity: Medium. Raised by another reviewer: Yes (@SEZ9).

Issue 9: Merges made with GITHUB_TOKEN will not trigger the post-merge push CI on dev

• Location: .github/workflows/ai-merge-gate.yml:31-77 (token), .github/workflows/build_main.yml (on: push)
• Problem description: GitHub does not start new workflow runs for events created with the repository GITHUB_TOKEN. build_main.yml runs the full build on every push to dev (for example run 37927712604 on the current head). A squash merge performed by merge_pr through the Actions token would therefore land code, which by design nobody read, without the post-merge build on dev.
• Potential risk: a regression from an unreviewed merge is only noticed by the next unrelated push or by the nightly run.
• Best improvement: going through the merge queue (Issue 8) solves this, because the queue/merge is performed by GitHub itself. Otherwise dispatch build_main.yml explicitly after the merge (workflow_dispatch is the documented exception) or merge with a dedicated app identity.
• Severity: Medium
• Raised by another reviewer: No

Issue 10: Test code of the tests class is merged unreviewed and then runs in privileged upstream CI

• Location: tools/ai_merge_gate.py:650-665 (_check_test_file only checks weakening), tools/test_ai_merge_gate.py:168 (test_security_words_in_tests_do_not_block)
• Problem description: src/test additions are not scanned for process, network, file-system or reflection use, and tests / connector PRs can add arbitrary test code. After the merge, that code runs in build_main.yml on pushes to dev in apache/seatunnel, in a job with packages: write and a checkout token. The pre-merge fork run is not the privileged context; the post-merge run is.
• Best improvement: apply the process/network/reflection patterns (NEW_CODE_BLOCK_RE, CLASSLOAD_RE, plus Files.write, HttpClient, Socket, exec) to added test lines as hard blockers (tests that need a container already go through Testcontainers, so legitimate hits should be rare), or limit the tests class to changes that only add assertions to existing tests.
• Severity: Medium
• Raised by another reviewer: No

Issue 11: The 465 lines of unit tests never run in CI, and the merge-authority half has no tests

• Location: .github/workflows/backend.yml:78 (only tools/update_modules_check is discovered), tools/test_ai_merge_gate.py
• Problem description: nothing runs python3 -m unittest for this file, so a later edit to a policy regex can silently turn a "pinned" test red without anyone noticing. Separately, run_gate / cmd_enforce / cmd_sweep / merge_pr are untested, although they decide label provenance, stale labels, approval pinning, pending-versus-blocked and the merge call itself. Issues 1, 6, 7 and 8 are all in that untested half.
• Best improvement: add a step to the existing "Check CI scope rules" job (for example python3 -m unittest discover -s tools -p 'test_ai_merge_gate.py') and add a small fake GitHub class to test the five or six decision rules (wrong label actor, label older than head, approval for an old SHA, required check absent, CHANGES_REQUESTED, dry run).
• Severity: Medium
• Raised by another reviewer: No

Issue 12: Workflow-level contents: write is granted to the pull_request_target job

• Location: .github/workflows/ai-merge-gate.yml:31-36
• Problem description: permissions is declared once at workflow level, so the enforce job (which only needs issues and pull-requests write) also receives contents: write. The script never checks out PR code, so this is defense in depth, but it is the one job that runs on events from forks.
• Best improvement: declare permissions per job: enforce with issues: write and pull-requests: write; sweep with whatever the chosen merge mechanism needs.
• Severity: Low
• Raised by another reviewer: Partly (@SEZ9 mentioned it in the summary but not as a numbered issue)

2. Code Quality Assessment

2.1 Coding Standards

The module docstring and the per-rule comments are very good. Two small things: the # !/usr/bin/python line in tools/ai_merge_gate.py is not a valid shebang (harmless), and AI_MERGE_REPO_ROOT is read in two places with the same default, fine.

2.2 Test Coverage and Test Stability

Classification tests are deterministic and pure (no network, no clock), so stability is fine. Rating: Stable for what exists; coverage gaps are in Issues 4 and 11.

2.3 Documentation Updates

This is repository tooling, so no docs/en / docs/zh page is required. The PR description is thorough; once the Build-only reality (Issue 1) is fixed, please update the description of the required checks accordingly.

3. Architectural Soundness

• 3.1 The classification approach is a solid long-term foundation; the CI/merge integration is a workaround that does not fit the fork-CI plus merge-queue setup yet.
• 3.2 Maintainability: one 980-line script is OK for now; the pure half is easy to extend.
• 3.3 Extensibility: adding a class means adding to CAPS, evaluate and tests, which is clear.
• 3.4 Historical compatibility: not applicable.

4. Issue Summary

#	Issue	Location	Severity
1	Required class checks never present on the head (fork CI mirrored as Build)	ai_merge_gate.py:517-524, 843-853	High (@SEZ9)
2	Connector-to-connector dependencies ignored in blast radius	ai_merge_gate.py:272-281	High (@SEZ9)
3	Pom check ignores groupId; <properties> bypasses version rule	ai_merge_gate.py:527-553	High (@SEZ9)
4	Binary files bypass all content rules	ai_merge_gate.py:416, 606	High (@SEZ9)
5	New-connector relaxation downgrades reflection/classloading	ai_merge_gate.py:630-639	High (@SEZ9)
6	Approver default is a hard-coded login, fail-closed branch unreachable	ai_merge_gate.py:241	High (@SEZ9)
7	Sweep has no per-PR isolation, always exits 0, null dereferences	ai_merge_gate.py:933-939, 816, 832	Medium (@SEZ9 + addition)
8	REST merge vs merge-queue ruleset	ai_merge_gate.py:883-891	Medium (@SEZ9)
9	GITHUB_TOKEN merges do not trigger post-merge push CI on dev	ai-merge-gate.yml, build_main.yml	Medium
10	Unreviewed test code runs later in privileged upstream CI	ai_merge_gate.py:650-665	Medium
11	Unit tests not wired into CI; run_gate/merge path untested	backend.yml:78, test_ai_merge_gate.py	Medium
12	contents: write on the pull_request_target job	ai-merge-gate.yml:31-36	Low

CI and merge state (head 3bba16034824)

• Build is red in the fork run (davidzollo/seatunnel run 38022100908): 81 jobs passed, 11 skipped and only elasticsearch-connector-it (11) and (17) failed, with ElasticsearchIT PIT/Scroll slice assertions. The same test methods fail on the dev head itself (push run 37927712604), so this is not caused by this PR. A sync is not needed (the branch is 0 commits behind dev, no conflicts); re-running only the failed jobs will not help until dev is fixed.
• The PR is a draft; mergeStateStatus is BLOCKED (review required). There is an existing CHANGES_REQUESTED review from @SEZ9 on this head.

5. Merge Recommendation

Conclusion: Ready to merge after fixes

1. Blockers (must be fixed before the gate is enabled; the merge step is a dry run until AI_MERGE_ENABLED=true): Issues 1 to 6.
2. Recommended: Issues 7 and 8 (they decide whether the gate can merge at all), then 9, 10, 11 and 12.

Overall: the pure classification layer is the strongest part and the direction is right. I would like the integration half to get the same care and tests as the classification half before it gets merge authority. Happy to re-review after an update.

[DanielLeens]

Thanks for the update, and sorry for the churn on my side: the head moved from the ai-merge gate to a triage-only labeler, so I re-read this as a brand-new PR at head 782c83247 (workflow, tools/pr_risk_label.py, tools/test_pr_risk_label.py, full diff against dev). The new design removes the merge authority, the contents: write grant, the sweep and the merge-queue conflict, so my twelve issues on 3bba160 and @SEZ9's eight issues on the same head no longer apply and I am not carrying them over. The direction is much safer. I found two things worth fixing before this leaves draft, plus three smaller items.

What Problem Does This PR Solve?

• User pain point: committers have no cheap way to order the review queue by blast radius, so a docs fix and an engine change wait in the same line.
• Fix approach: a pull_request_target workflow runs tools/pr_risk_label.py, which reads the PR only through the GitHub API, scores it (base level from paths, plus bounded adjustments from diff facts, platform code fixed at 5, everything else capped at 4), upserts one bot comment that shows the arithmetic, and keeps exactly one risk: N-name label.
• One-sentence summary: an advisory, API-only risk triage label that cannot approve, block or merge.

1. Code Change Review

1.1 Core Logic Analysis

pull_request_target (opened|reopened|synchronize|ready_for_review|edited)   pr-risk-label.yml:23-24
  concurrency per PR number, cancel-in-progress                              :33-35
  actions/checkout@v4  (base branch, no PR code)                             :46
  python3 -I tools/pr_risk_label.py evaluate --pr N --apply                  :51
    evaluate_pr: GET pulls/N, GET pulls/N/files (paginated), load_known(base poms)   pr_risk_label.py:781
      evaluate(): classify_path per file -> base level; _pom_facts/_main_facts/_test_facts -> adjustments
                  compute_level: min(4, base + points), platform fixed 5; non-default base floor 3
    ensure_labels (create 5 labels) -> upsert_report (PATCH or POST bot comment) -> apply_label (add new, then remove old)

Key findings:

• The security model holds: the workflow checks out the base branch (same pattern as add-label.yml), the only interpolated expression is the integer PR number, permissions are contents: read, issues: write, pull-requests: write, and the script never executes PR content. No approval, review or merge call exists (test_no_merge_path_exists).
• Classification looks careful and conservative: unknown paths are medium, platform paths are fixed at 5, test-only changes in platform modules are trivial, moved lines are neutral, and the option-key / persisted-class / registration-removal / test-weakening rules count once per PR.
• Because tools/ and .github/ are platform paths, any later edit of this script will itself be risk: 5-critical, which is appropriate.

1.2 Compatibility Impact

Fully compatible. No runtime, API, option or format change. It adds one workflow and one label family. Interaction with add-label.yml (actions/labeler@v5, sync-labels: true): to my understanding v5 only removes labels that are defined in its own config, so risk: labels should survive, but please confirm it on the first live run as your description already says.

1.3 Performance / Side-Effect Analysis

One short job per PR event plus 5 label-create calls, 2 to 3 list calls and 2 to 3 writes. edited re-runs on every title or body edit, which is fine because the comment is PATCHed in place. os.walk over all poms of a depth-1 checkout is cheap. Concurrency groups cancel older runs, and adding the new label before removing the old one keeps the PR from ever having zero risk labels.

1.4 Error Handling and Logging

Issue 1 (Medium, fix before undrafting): attacker-controlled text is written unescaped into a bot-authored comment.

• Location: tools/pr_risk_label.py:662 and :620 (details embed text.strip() of a PR source line), :694-702 (file paths are placed in a backtick span), :696 (table row), plus option keys, artifact names and versions from poms.
• Problem description: the comment is posted by the repository bot, so readers will treat it as authoritative. Any contributor, including a first-time fork PR, can choose the content of these strings: a changed line such as String s = "@apache/seatunnel-committers | ok"; that also contains a word like token is echoed verbatim into a table cell; | breaks the table; @user or @org/team notifies people from the bot; a backtick, <, or a newline (a file path may contain one) breaks out of the code span and can forge extra rows or links.
• Potential risk: mention spam and a spoofable "risk" report on public PRs. It cannot change the label, but it changes what reviewers read.
• Best improvement: one _md() helper applied to every interpolated value in render_report: collapse whitespace and newlines, truncate (about 120 characters), escape |, backtick and <, and neutralize @ (for example insert a zero-width space or wrap the value in a code span after stripping backticks). Add a test that a line with @x|y and a path with a newline render as one inert row.
• Raised by another reviewer: No

Issue 2 (Medium, fix before undrafting): the 59 unit tests are not run by any CI job.

• Location: tools/test_pr_risk_label.py, .github/workflows/backend.yml (the existing python3 -m unittest discover -s tools/update_modules_check -p 'test_*.py' step is the pattern to follow)
• Problem description: nothing in the diff invokes tools/test_pr_risk_label.py, and the new workflow only runs the script. Deleting or weakening a rule would not turn anything red, which defeats the mutation-checked tests described in the PR. This is the same class of gap I raised on the earlier head.
• Best improvement: add python3 -m unittest discover -s tools -p 'test_pr_risk_label.py' next to the other tools test step in backend.yml, and mention the step in the PR description.
• Raised by another reviewer: No

Issue 3 (Medium, non-blocking): a changed existing test does not count as test evidence, so honest fixes get +1 no-regression-test.

• Location: tools/pr_risk_label.py:455-456 (added_test_annotation is set only when an added line matches @Test/@TestTemplate/...), message at :525 ("no test method was added or changed")
• Problem description: the project guideline is to extend an existing test class, and a typical fix adds assertions to an existing method or edits a .conf/resource of an existing E2E. In that diff no @Test line is added, so the PR is told it has no test and is pushed up one level; the message also says "or changed", which the code does not implement.
• Best improvement: treat any added line matching ASSERT_RE in a kind == "test" file, or any change to a test resource file of the same module, as evidence. Add one test for "assertion added to an existing method".
• Raised by another reviewer: No

Issue 4 (Medium, non-blocking): version bumps defined through pom properties are invisible to the supply-chain rule.

• Location: tools/pr_risk_label.py:153 (PAIR_RE skips ${...} versions), :589 (dependency-change only fires on <artifactId> or a literal <version>)
• Problem description: connector poms declare most versions as properties (for example connector-activemq/pom.xml:33 <activemq.version>5.15.16</activemq.version>, and 26 connector poms have two or more such lines). A PR that only bumps such a property touches no <artifactId> and no <version> line, so the whole PR is risk: 2-low with zero adjustments, even though it changes a third-party jar that ships in the distribution. This is the PR class the rule is meant to catch.
• Best improvement: count an added/removed line that matches <[A-Za-z0-9_.-]*version[A-Za-z0-9_.-]*> inside a pom as dependency-change (+1), and as new-supply-chain (+2) when the value is not used by any base pom. One test with <activemq.version> bumped.
• Raised by another reviewer: No

Issue 5 (Low, non-blocking): write calls ignore the HTTP status.

• Location: tools/pr_risk_label.py:802, :804, :812, :814 (gh.call results are discarded; only ensure_labels checks the status)
• Problem description: if a comment PATCH/POST or a label add fails (permission change, secondary rate limit, deleted comment), the job still ends green and the PR silently has a stale or missing label.
• Best improvement: raise ApiError for status >= 300, tolerating 404 on removing an already-absent label, so the failure shows up in the Actions run.
• Raised by another reviewer: No

2. Code Quality Assessment

2.1 Coding Standards

The file header and the module docstring explain the scoring model and the security model well, and the rules are table-driven. The ASF license header is present on all three files.

2.2 Test Coverage and Test Stability

The offline tests cover every rule with positive and negative cases (moved options, equivalent-assertion replacement, polling helper, disabled test on a brand-new test, label plan, no merge path). Gaps: Issues 1, 3 and 4 have no test, and Issue 2 means none of them run in CI. Stability rating: Stable. They are pure in-memory unittest cases with no network, clock or ordering dependency.

2.3 Documentation Updates

No user-facing docs are needed. A short note on the label meanings for contributors (for example in the PR template or the contributing guide) would help people who see the bot comment for the first time. Optional.

3. Architectural Soundness

A precise, advisory-only solution. Keeping scoring pure (evaluate) and GitHub I/O separate is what makes it testable and keeps the pull_request_target surface small. The heuristics are line-level, as the description says, so the label is a triage hint, and your replay on 150 merged PRs honestly shows that levels 1 to 4 do not separate strongly on review effort. Historical compatibility: not applicable.

4. Issue Summary

#	Issue	Location	Severity
1	Untrusted text rendered unescaped in a bot comment	pr_risk_label.py:620,662,694-702	Medium
2	Unit tests not run by CI	backend.yml, test_pr_risk_label.py	Medium
3	Edited existing test is not counted as test evidence	pr_risk_label.py:455,525	Medium
4	Property-defined version bumps are not flagged	pr_risk_label.py:153,589	Medium
5	Write calls ignore HTTP status	pr_risk_label.py:802-814	Low

CI and merge state

Head 782c83247: the apache-side Build check is FAILURE, mirrored from the fork run (https://github.com/davidzollo/seatunnel/actions/runs/38055369411) where three jobs failed: engine-v2-it (11), elasticsearch-connector-it (11) and (17). This PR changes only .github/ and tools/, so I do not see a way for those jobs to be caused by it, and similar Elasticsearch IT failures have been seen on the dev push run itself (run 37927712604, as I noted on #12705). I could not tie the engine-v2-it failure to this diff either, so a rerun of just the failed jobs is the cheapest next step. The branch is 3 ahead / 0 behind dev, mergeable=true, still a draft (BLOCKED).

5. Merge Recommendation

Conclusion: Ready to merge after fixes

1. Blockers (before undrafting): Issue 1 (escape untrusted text in the comment) and Issue 2 (run the tests in CI), plus a green or explained CI run.
2. Recommended, non-blocking: Issues 3, 4 and 5.

No other reviewer has commented on this head yet (@SEZ9's earlier CHANGES_REQUESTED review is on the previous design). Since this adds a bot comment to every PR, it would be good for a maintainer to confirm project-wide acceptance of that before it goes live; the first rollout can stay read-only as your description suggests. Thanks for the quick and substantial redesign, and happy to re-review after your next push.

[DanielLeens]

Thanks for the substantial rework, @davidzollo! I went through the full diff of this head (5 files: two new workflows, tools/pr_risk_label.py, its tests, and one backend.yml step), re-read all of my previous findings against the code, and traced both workflows end to end. The five findings from my last review are all addressed, and the new pull_request + workflow_run design is much safer. I have a few smaller notes below, none of which blocks.

What Problem Does This PR Solve?

User pain point: Committers have no cheap way to sort the review queue by risk, so low-risk PRs wait behind high-risk ones and nothing flags the PRs that touch credentials, CI, shared modules or the supply chain.

Fix approach: A tool (tools/pr_risk_label.py) classifies a PR from the pulls/N/files payload into one of five levels, using path policy plus changed-line facts. Two workflows run it: pr-risk-label.yml (pull_request, read-only token, report in the job summary, uploads the PR number) and pr-risk-label-apply.yml (workflow_run on the default branch, the only run with pull-requests: write, recomputes everything from the API and refuses to label unless the PR head still equals workflow_run.head_sha). Both are inert until the repository variable PR_RISK_ENABLED is true.

One-sentence summary: An advisory, ASF-policy-compliant risk label per PR; nothing approves, blocks, merges or comments.

Example: a Kafka sink fix with a new assertion in an existing test scores base 2 (one connector unit), no adjustments, label risk: 2-low. The same fix touching password handling in existing code becomes risk: 5-critical through the credential fact.

1. Code Change Review

1.1 Core Logic Analysis

Before (previous head 782c832): a single pull_request_target-style workflow wrote a bot comment containing PR-controlled text. After (this head 14d3ba7): no comment, report only in the job summary, two-stage workflows.

Runtime path:

PR opened/synchronize/edited (fork or branch)
  -> pr-risk-label.yml (pull_request, contents:read, pull-requests:read)
       checkout BASE sha (trusted script + base poms)
       python3 -I tools/pr_risk_label.py evaluate --pr N     -> report in job summary
       upload artifact pr-risk/number
  -> pr-risk-label-apply.yml (workflow_run, default-branch code, pull-requests:write)
       if repo == apache/seatunnel && PR_RISK_ENABLED && event == pull_request && conclusion == success
       download artifact -> NUMBER = digits only (tr -cd '0-9')
       python3 -I tools/pr_risk_label.py evaluate --pr NUMBER --expect-head <workflow_run.head_sha> --apply
         -> GET pulls/N, pulls/N/files (paginated), recompute with trusted code
         -> head != expect-head ? stale, nothing written
         -> apply_label: add new risk label first, then remove other "risk: " labels

The normal PR path reaches all of this code. I checked the trust boundary specifically:

• Because pull_request workflows take the workflow file from the PR, a malicious PR can upload any artifact content. That is harmless here: only digits are kept, the level is recomputed by default-branch code from the API, and the label is applied only if that PR's head equals the head of the run that produced the artifact. Pointing the number at another PR fails the head check.
• No PR code is checked out or executed; the first run checks out base.sha, and the second run uses the default branch. No secrets beyond GITHUB_TOKEN are used, and github.event.pull_request.number (an integer) is the only expression interpolated into a shell command.
• Escaping: _md() collapses whitespace, truncates to 120 characters, and escapes backslash, backtick, pipe, [, <, > and neutralizes @; every PR-controlled value in the report goes through it (details, file paths, units, head). test_pr_controlled_text_is_inert_in_the_report covers a hostile line and path.

Findings from my last review, status on this head:

1. Unescaped untrusted text in a bot comment: fixed (no comment any more, _md on every value).
2. Tests not run in CI: fixed (backend.yml step python3 -I tools/test_pr_risk_label.py; the test file inserts its own directory into sys.path, so -I works).
3. Edited existing test not counted as evidence: fixed (ASSERT_RE on added lines or a changed test resource counts; test_assertion_added_to_an_existing_test_method_counts_as_evidence).
4. Property-defined version bumps invisible: fixed (VERSION_PROPERTY_RE, new-supply-chain when unknown, dependency-change otherwise; test_version_property_bump_is_flagged).
5. Write calls ignoring HTTP status: fixed (GitHub.write raises on status >= 300, 404 tolerated only for removing an absent label).

1.2 Compatibility Impact

Fully compatible. No runtime, API, option or format change. It adds two workflows (skipped until PR_RISK_ENABLED is set; the two Evaluate risk jobs on this head show as skipped, which is the intended inert state), a tool, tests, and one CI test step. It adds a risk: label family and does not touch other labels (plan_label_changes only removes names starting with risk: ), so labeler with sync-labels is not affected.

1.3 Performance / Side-Effect Analysis

One short job per PR event: 1 + ceil(files/100) GETs, 1 label list, up to 2 writes. os.walk over the base poms is cheap. Concurrency groups cancel superseded first-stage runs; the second stage is keyed by head SHA without cancellation. Adding the new label before removing the old one avoids a window with no risk label.

1.4 Error Handling and Logging

Failures raise ApiError and fail the run visibly. No tokens are printed. Issues:

Issue 1: A PR with more than 3000 changed files is evaluated on a truncated file list without any signal

• Location: tools/pr_risk_label.py:847-854 (evaluate_pr)
• Problem: GET pulls/N/files returns at most 3000 files (sorted by path). evaluate_pr already fetches the PR object but does not compare pr["changed_files"] with len(files). A PR whose first 3000 files are, say, docs under docs/ and whose runtime changes sort later (seatunnel-*) is classified on the visible part only and can come out as risk: 1-trivial.
• Potential risk: the triage label under-reports exactly for the oversized PRs; a contributor can also create this shape on purpose. It is advisory, so a human still reviews, but the label is the thing used to order the queue.
• Best improvement: if pr["changed_files"] > len(files), add the existing uninspectable-diff adjustment (or a critical fact) with a detail such as "only N of M files could be listed", and add one unit test with a fake client.
• Severity: Medium (non-blocking)
• Raised by another reviewer: No

Issue 2: Connector modules that other connectors depend on are scored as ordinary single units

• Location: tools/pr_risk_label.py:271-280 (_unit_for), SHARED_CONNECTOR_MODULES
• Problem: Only connector-common/fake/assert/console, *-base, *-base-hadoop and JDBC core are shared:. On dev, connector-cdc-oceanbase depends on connector-cdc-mysql, and connector-hive and connector-s3-redshift depend on connector-file-s3, and the CDC connectors depend on connector-jdbc dialect classes. A change to connector-cdc-mysql or connector-file-s3 therefore scores base 2 with no dependents-not-tested note. I raised the same coverage gap against the earlier merge-gate design; for a triage label it is much less serious, hence Low.
• Potential risk: slight under-triage of a few widely reused connectors.
• Best improvement: add these modules to SHARED_CONNECTOR_MODULES now, or derive the shared set from connector-to-connector <dependency> entries in the base poms (the script already walks them in load_known).
• Severity: Low
• Raised by another reviewer: No

Issue 3: Apply and first-stage runs are not exercised before merge, and first-time contributors' runs need approval

• Location: .github/workflows/pr-risk-label.yml, .github/workflows/pr-risk-label-apply.yml
• Problem: workflow_run workflows only run from the default branch, so the apply stage cannot be tried on this PR, and the PR description says so honestly. Separately, pull_request runs of first-time contributors in this repository wait for a maintainer to approve workflows, so the PRs that most need a triage label may get none until approved.
• Potential risk: low; labels missing on some PRs, and the first live run is the real test.
• Best improvement: keep the staged rollout (create labels, set PR_RISK_ENABLED, watch the first runs on a few PRs) and mention the approval caveat in the rollout note.
• Severity: Low
• Raised by another reviewer: No

2. Code Quality Assessment

2.1 Coding Standards

Good. The module docstring documents the scoring model and the security model; rules are table-driven; the ASF license header is present on all new files. The ASF actions/*@v4 actions used are already used elsewhere in the repository. Minor: test_no_merge_comment_or_label_creation_path_exists asserts that certain function names do not exist, which a rename would bypass; it is harmless as a tripwire but is not real protection, the permission scoping in the workflows is.

2.2 Test Coverage and Test Stability

Coverage is broad and each rule has positive and negative cases (moved options, equivalent-assertion replacement, polling helper, disabled test on a new test, credential declarations vs handling, version properties, head moved, failed write, hostile text). They are pure in-memory unittest cases with fixed inputs, no network, clock, ports or ordering dependency. Stability rating: Stable. The Issue 1 truncation case has no test (it has no code yet). I did not run the tests locally; CI is the evidence. At the time of this review the apache-side checks for this head were not finished (labeler and Notify test workflow queued, no Build result yet), so I cannot say the new backend.yml step is green.

2.3 Documentation Updates

No user-facing docs needed. The PR description documents the labels, the activation switch and the one-time label creation step (labels sub-command). I could not independently verify the dev-list agreement the description refers to; I reviewed the implementation against the description only.

3. Architectural Soundness

3.1 Elegance

Precise and conservative: pure scoring (evaluate) separate from GitHub I/O, advisory only, label is the single write.

3.2 Maintainability

Rule table plus tests make changes cheap. Residual risk: the keyword rules are line-level heuristics. For example CREDENTIAL_RE includes broad words such as token and principal, so an unrelated existing line mentioning a token can lift a connector PR to level 5; your replay shows 3 hits in 150 merged PRs, so this looks acceptable, and the report names the line so a reviewer can discount it.

3.3 Extensibility

Adding a path or fact is one table or regex entry plus a test.

3.4 Historical-Version Compatibility

Not applicable (repository automation only).

4. Issue Summary

Number	Issue	Location	Severity
1	Files list capped at 3000 is evaluated silently; compare with changed_files	tools/pr_risk_label.py:847-854	Medium
2	Reused connector modules (cdc-mysql, file-s3, jdbc dialects) scored as single units	tools/pr_risk_label.py:271-280	Low
3	Apply stage cannot be tested pre-merge; first-time contributor runs need approval	.github/workflows/pr-risk-label*.yml	Low

5. Merge Recommendation

Conclusion: Ready to merge

Blockers

• Source: none found.
• State: the PR is still a draft, the apache-side checks for this head had not completed when I looked, and the branch is 93 commits behind dev (mergeable_state is blocked, diverged, no conflict reported). Please wait for a green Build before undrafting; syncing dev first is reasonable since the PR edits backend.yml.

Recommended fixes (non-blocking)

• Issue 1: detect truncated file lists.
• Issue 2: widen the shared module set.
• Issue 3: keep the staged rollout and note the approval caveat.

Overall assessment: the redesign removes the main attack surface (bot comment, pull_request_target), keeps write permission in a default-branch-only run that recomputes from the API and binds to the commit, and is well covered by tests. Merge permission stays with committers, and the whole thing is inert until a committer flips PR_RISK_ENABLED. Better alternative: none needed; GitHub rulesets or CODEOWNERS cannot express this kind of content-based triage.

[DanielLeens]

Thanks again, @davidzollo, for the two follow-up pushes (9bf1ba69 and 1f384227). I re-read the full current head 1f384227183f (the five files: two workflows, tools/pr_risk_label.py, its tests, and the backend.yml step) and also the delta since my last review at 14d3ba77. The workflows are byte-for-byte unchanged since that head, so the trust-boundary analysis from my previous review still holds; the new work is the "everything under docs/ is documentation" rule and the reduction from five levels to three (risk: low|medium|high). Both look correct to me. Two items from my last review are still open (they were non-blocking then and still are), plus two tiny new notes.

What Problem Does This PR Solve?

• User pain point: committers cannot cheaply order the review queue by blast radius, so a docs fix and a credential or engine change wait in the same line.
• Fix approach: pr-risk-label.yml (pull_request, read-only token) reports the level in the job summary and uploads only the PR number; pr-risk-label-apply.yml (workflow_run, default-branch code, the only run with pull-requests: write) recomputes the level from the API and writes exactly one risk: low|medium|high label, but only if the PR head still equals workflow_run.head_sha. Both stay inert until the repository variable PR_RISK_ENABLED is true.
• One-sentence summary: an advisory, API-only triage label that cannot approve, block, merge or comment.

1. Code Change Review

1.1 Core Logic Analysis

Delta since my last review (14d3ba77 -> 1f384227), both only in tools/pr_risk_label.py and its tests:

1. DOC_RE is now (\.md$)|(^docs/) (tools/pr_risk_label.py:157), so images, sidebars.js and i18n JSON under docs/ are documentation instead of "other", and GITHUB_CI_RE still keeps .github/workflows|actions|scripts out of the docs class. ALWAYS_HIGH_RE is checked before DOC_RE in classify_path, so a Markdown file under seatunnel-engine/ or seatunnel-api/ stays high. test_every_file_under_docs_is_documentation covers the new rule.
2. Levels collapse to LOW, MEDIUM, HIGH = 1, 2, 3; compute_level is HIGH for a critical fact, otherwise min(HIGH, base + points). Because the old 4 and 5 both map to high and a single +2 on a low base also reaches 3, test weakening, a removed option, a persisted-state class change or a new supply-chain artifact each lift a connector PR straight to high, while one +1 fact (missing regression test, several units, sensitive word) gives medium. That matches the documented table and the test updates.

Runtime path (unchanged, re-verified on this head):

PR opened/synchronize/edited/ready_for_review
  -> pr-risk-label.yml   (pull_request, contents:read + pull-requests:read, checkout of base.sha)
       python3 -I tools/pr_risk_label.py evaluate --pr N      -> job summary only
       upload artifact pr-risk/number
  -> pr-risk-label-apply.yml (workflow_run, default-branch code, pull-requests:write)
       gate: repo == apache/seatunnel && PR_RISK_ENABLED && event == pull_request && conclusion == success
       NUMBER = digits only from the artifact
       evaluate --pr NUMBER --expect-head <workflow_run.head_sha> --apply
         -> GET pulls/N + pulls/N/files, recompute, head mismatch => "stale", nothing written
         -> add the new risk label first, then remove other "risk: " labels

Workflow security review (this is a CI workflow PR, so I re-checked it point by point against the YAML):

• No pull_request_target. The only privileged trigger is workflow_run, which always runs the default-branch workflow file and script, and no PR-controlled code is checked out or executed in that run (the checkout has no ref:, so it is the default branch).
• Token scope: the first workflow has contents: read and pull-requests: read; the second has pull-requests: write, contents: read, actions: read (needed for download-artifact with run-id). No other secret is used, GITHUB_TOKEN is passed only through env:.
• Script injection: the only ${{ }} expression inside a run: body is github.event.pull_request.number (an integer) in pr-risk-label.yml. PR title, branch name and body are never interpolated into a shell command; the apply stage passes EXPECT_HEAD through env: and reads the PR number from the artifact with tr -cd '0-9' | head -c 12, so a hostile artifact cannot carry shell syntax.
• Artifact poisoning: a PR can change pr-risk-label.yml itself (it runs from the PR), so it can upload any number. The apply stage only trusts the digits, recomputes from the API, and refuses unless that PR's head equals the commit the producing run was triggered for, so pointing at a different PR fails the head check. The worst case is a wrong-but-advisory label on a PR the attacker's own commit SHA belongs to.
• Markdown injection into the job summary: _md() bounds to 120 chars and escapes backslash, backtick, pipe, [, <, > and @; every PR-controlled value in render_report goes through it.
• Concurrency: first stage cancels superseded runs per PR number; apply stage is keyed by head SHA without cancellation, so a label write is never cut off halfway.

1.2 Compatibility Impact

Fully compatible. No runtime, API, option or file-format change. The risk: label family is new and plan_label_changes only touches names starting with risk: , so the existing labeler config with sync-labels is unaffected. Note the label names changed from the earlier risk: N-name form to risk: low|medium|high; the one-time labels sub-command prints the matching gh label create commands, and nothing has been created in the repository yet, so there is no migration concern.

1.3 Performance / Side-Effect Analysis

One short job per PR event: 1 + ceil(files/100) GETs, one label list, at most two label writes. No hot-path cost. The ordering "add new label, then remove stale ones" avoids a window with no risk label.

1.4 Error Handling and Logging

Write calls raise on any status >= 300 (404 tolerated only when removing an absent label), tokens are never printed. Two earlier items are still open:

Issue 1 (still open from my last review): a PR with more than 3000 changed files is evaluated on a truncated file list without any signal

• Location: tools/pr_risk_label.py:843-850 (evaluate_pr)
• Problem: GET pulls/N/files returns at most 3000 files. evaluate_pr already loads the PR object (pr) but still never compares pr["changed_files"] with len(files) (I grepped the head for changed_files: no match). If the visible 3000 files are docs and the runtime changes sort later, the PR can be labelled risk: low.
• Potential risk: the label under-reports exactly for oversized PRs, which is the shape an attacker or a mass-refactor would produce. It stays advisory, but the label drives queue order.
• Best improvement: if pr["changed_files"] > len(files), add a critical fact or the existing uninspectable-diff adjustment ("only N of M files could be listed") and add one unit test with a fake client.
• Severity: Medium (non-blocking)
• Raised by another reviewer: No

Issue 2 (still open, Low): reused connector modules are still scored as ordinary single units

• Location: tools/pr_risk_label.py:174 (SHARED_CONNECTOR_MODULES), _unit_for at :305
• Problem: only connector-common/fake/assert/console, *-base, *-base-hadoop and JDBC core are shared:. Modules such as connector-cdc-mysql (used by connector-cdc-oceanbase) and connector-file-s3 (used by connector-hive, connector-s3-redshift) are not, so a change there is risk: low with no note about dependents. With only three levels the difference between low and the single-unit baseline now matters slightly more, because a low label is the "skim" bucket.
• Best improvement: add these modules to the set, or derive the set from connector-to-connector <dependency> entries in the base poms (the script already walks them in load_known).
• Severity: Low (non-blocking)
• Raised by another reviewer: No

Issue 3 (new, Low): the module docstring places "examples" in two different levels

• Location: tools/pr_risk_label.py:29-31 (docstring base table)
• Problem: the low row lists "examples" while the medium row lists "CLI, benchmarks, examples, deploy". In code seatunnel-examples/.../examples/*.conf is metadata (low) and seatunnel-(cli|benchmarks|examples)/ otherwise is MEDIUM, so both are true, but a reader of the table cannot tell which is which.
• Best improvement: say "example .conf files" in the low row and "example project code" in the medium row.
• Severity: Low (non-blocking)
• Raised by another reviewer: No

Issue 4 (new, Low): optional hardening for the apply job checkout

• Location: .github/workflows/pr-risk-label-apply.yml (actions/checkout@v4 step)
• Problem: the job holds pull-requests: write and leaves the default persist-credentials: true, so the token is stored in .git/config for the rest of the job. No PR code runs there, so this is not exploitable today.
• Best improvement: add with: persist-credentials: false to the checkout in both workflows (the scripts use the API only), which keeps the token out of the working tree if someone later adds a step.
• Severity: Low (non-blocking)
• Raised by another reviewer: No

Previous Issue 3 (the apply stage cannot be exercised before merge, and first-time contributor pull_request runs need maintainer approval) remains a rollout note only; the staged rollout through PR_RISK_ENABLED is the right mitigation and needs no code change.

2. Code Quality

2.1 Coding Standards

Good: the ASF license header is on all new files, rules are table-driven, the security model is documented in the module docstring, and the actions/*@v4 versions match what the repository already uses.

2.2 Test Coverage and Test Stability

I compared tools/test_pr_risk_label.py before and after the three-level change: the number of assertion lines is identical (63 removed, 63 added), every level constant is mapped consistently (5 -> HIGH, 4 -> HIGH, 3 -> MEDIUM, 2 and 1 -> LOW), and the previous test_non_default_base_branch_has_a_floor_of_three became test_non_default_base_branch_is_at_least_medium with the same two assertions. I found no weakened test. New tests (test_every_file_under_docs_is_documentation, test_author_association_plays_no_role, test_any_change_under_engine_or_api_is_high_including_tests_and_docs) fail if the corresponding rule is removed. They are pure in-memory unittest cases with fixed inputs, no network, clock or ordering dependency. Stability rating: Stable (tools/test_pr_risk_label.py). The step Run pr risk label tests in backend.yml ran and passed inside Run / Code style on this head. The 3000-file case (Issue 1) has no test yet.

2.3 Documentation Updates

No user-facing docs needed. The PR description and module docstring document the labels, the activation variable and the one-time label creation. Please refresh the description if it still mentions the five-level risk: N-name labels.

3. Architectural Soundness

The structure is still the right one: pure scoring (evaluate) separate from GitHub I/O, a read-only first stage, a default-branch second stage, one advisory write. Three levels are easier for committers to act on than five, at the cost of folding "shared connector" and "platform" into one high bucket, which is a policy choice the dev-list thread decides, not a code problem. I did not find a better location for the logic; GitHub rulesets or CODEOWNERS cannot express content-based triage.

4. Issue Summary

Number	Issue	Location	Severity
1	Files list capped at 3000 is evaluated silently (carried over)	tools/pr_risk_label.py:843-850	Medium
2	Reused connector modules scored as single units (carried over)	tools/pr_risk_label.py:174	Low
3	Docstring lists "examples" in two levels	tools/pr_risk_label.py:29-31	Low
4	Optional persist-credentials: false on the apply job checkout	.github/workflows/pr-risk-label-apply.yml	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers

• Source: none found. The workflows are safe as written (no pull_request_target, minimal token scope, no PR text in shell, commit-bound label write).
• State: the PR is still a draft, Build is red on this head, and the branch is 93 commits behind dev (diverged, no conflict reported). The red Build is not caused by the diff as far as I can tell: in the fork run the Code style job, which contains the new Run pr risk label tests step, and Dependency licenses and License header all passed. unit-test (11, ubuntu-latest) failed with RestApiSubmitJobStartWithSavePointTest.setUp IllegalState Node failed to ... (a Hazelcast node-start timeout in seatunnel-engine-server, a module this PR does not touch), and elasticsearch-connector-it (11 and 17) failed and all-connectors-it-6 was cancelled when the run hit the time limit. These are inferences from the job names and the touched files, not proof. Please sync with dev (the PR edits backend.yml) and re-run the failed jobs before undrafting.

Recommended fixes (non-blocking)

• Issue 1: detect a truncated file list and raise the level, with one unit test.
• Issue 2: widen the shared module set.
• Issues 3 and 4: small docstring and checkout hardening tweaks.

Overall, the design is good and the three-level simplification is clean and well tested. Thank you for the careful iteration; once Build is green on a synced head this is in good shape.

solved

[DanielLeens]

Thank you, @davidzollo, for the thorough iteration on this one. The branch has moved from 1f384227 (my last review) to 87aea81a: I re-read the whole current head again (the two workflows, tools/pr_impact_label.py, its 80 tests, and the backend.yml step), not only the delta. This is a static source review; I did not run anything locally. The push contains the risk to impact rename, the rule that high is decided only by what was touched, platform production lines counted in the report, and a label-description length fix.

What Problem Does This PR Solve?

• User pain point: committers cannot cheaply order the review queue by blast radius, so a docs fix and an engine or credential change wait in the same line.
• Fix approach: pr-impact-label.yml (pull_request, read-only token) writes the level into the job summary and uploads only the PR number. pr-impact-label-apply.yml (workflow_run, default-branch code, the only run with pull-requests: write) recomputes the level from the API and writes exactly one impact: low|medium|high label, only if the PR head still equals workflow_run.head_sha. Both are inert until the repository variable PR_IMPACT_ENABLED is true.
• One-sentence summary: an advisory, API-only triage label that cannot approve, block, merge or comment.

1. Code Change Review

1.1 Core Logic Analysis

What changed since my last review (1f384227 to 87aea81a, checked in the files at this head):

1. Rename risk to impact. I searched all four files at this head for leftover risk strings and found none; the label prefix is impact: (tools/pr_impact_label.py, LABEL_PREFIX), the variables are PR_IMPACT_ENABLED and PR_IMPACT_DEFAULT_BASE, and the backend.yml step runs tools/test_pr_impact_label.py. Consistent.
2. High by what was touched. compute_level (tools/pr_impact_label.py:618) now returns high only for a high base or a critical fact; facts add up to medium at most (sum(points) >= 2). The critical facts are core-class, core-class-removed, persisted-format, option-removed and option-changed, all emitted from _main_facts and the option-contract block. This matches the table in the description.
3. Platform production lines are counted in the report (evaluate, platform branch), without changing the level because the base is already high.
4. Label descriptions are 77, 93 and 82 characters (checked by loading LEVEL_META), so they fit GitHub's 100-character limit.

Runtime path (unchanged, re-verified against the YAML on this head):

PR opened/synchronize/edited/ready_for_review/reopened
  -> pr-impact-label.yml   (pull_request, contents:read + pull-requests:read, checkout of base.sha)
       python3 -I tools/pr_impact_label.py evaluate --pr N   -> job summary only
       upload artifact pr-impact/number  (retention 1 day)
  -> pr-impact-label-apply.yml (workflow_run, default-branch code, pull-requests:write)
       gate: repo == apache/seatunnel && PR_IMPACT_ENABLED && event == pull_request && conclusion == success
       NUMBER = digits only from the artifact (tr -cd '0-9' | head -c 12)
       evaluate --pr NUMBER --expect-head <workflow_run.head_sha> --apply
         -> GET pulls/N + pulls/N/files, recompute, head mismatch => "stale", nothing written
         -> add the new label first, then remove other "impact: " labels (404 tolerated)

Workflow security: no pull_request_target; the only privileged trigger is workflow_run, which runs the default-branch file and script, and no PR code is checked out or executed in that run. The only ${{ }} in a run: body is the integer github.event.pull_request.number; EXPECT_HEAD goes through env:. Token scopes are minimal. The Markdown summary goes through _md(). I found no new security issue on this head.

1.2 Compatibility Impact

Fully compatible. No runtime, API, option, serialization or file-format change; the label family is new and plan_label_changes only touches names that start with impact: , so the existing labeler with sync-labels is unaffected.

1.3 Performance / Side-Effect Analysis

One short job per PR event: 1 + ceil(files/100) GETs, one label list, at most two label writes. The add-before-remove order avoids a window with no impact label.

1.4 Error Handling and Logging

Write calls raise on any status of 300 or more (404 tolerated only when removing an absent label), and tokens are never printed.

Issue 1 (carried over from my last two reviews, still open): more than 3000 changed files are evaluated on a truncated list without any signal

• Location: tools/pr_impact_label.py:868 (GitHub.paginate) and its use in evaluate_pr
• Problem: pulls/N/files returns at most 3000 files. paginate stops when a page has fewer than 100 items, so a 3000-file PR looks complete, and nothing raises the level.
• Risk: a very large PR, e.g. a mass rename that includes platform files beyond the cut-off, can be labelled low or medium.
• Best improvement: if len(files) >= 3000 (or changed_files from the PR object is larger than len(files)), add a fact such as uninspectable-diff (+2) or force at least medium, and add one unit test.
• Severity: Medium (advisory label only)
• Raised by another reviewer: No

Issue 2 (new): changes to the connector parent poms are scored as "registration", so adding a dependency, plugin or property there is low

• Location: tools/pr_impact_label.py:147 (REGISTRATION_FILES), :656 (_registration_facts), and the pom branch in classify_path (:334)
• Problem: seatunnel-connectors-v2/pom.xml and seatunnel-e2e/seatunnel-connector-v2-e2e/pom.xml are classified as registration, and _registration_facts only looks at removed lines (plus _supply_facts for seatunnel-dist/pom.xml). The connectors parent pom is not only a module list: on dev it declares <dependencies>, <dependencyManagement> and a <build> section with maven-shade-plugin and maven-dependency-plugin that every connector inherits. A PR that only adds a dependency, a plugin or a <repositories> block there has no removed line, so the result is low with no fact. Editing an existing line there, in contrast, counts as "registration entry removed" (+2), so the outcome depends on whether a line was edited or added. The description says non-connector poms are high, and the tests I found only pin <module> additions to these files (the new-connector fixture) and a <module> removal (test_removing_a_registration_entry_is_medium).
• Risk: the label under-reports the blast radius of the one pom that affects the shading and dependencies of every connector. The same concern applies, at lower scale, to the grouping poms (for example connector-cdc/pom.xml), which MODULE_POM_RE also matches as a connector pom.
• Best improvement: treat only <module> lines as registration for these parent poms (the same filter _pom_facts already uses), and run _supply_facts plus the dependency-change fact on every other added or removed line; or classify the connectors parent pom as a shared unit (medium). Add one test that adds a <dependency> or <plugin> to seatunnel-connectors-v2/pom.xml and expects at least medium.
• Severity: Medium (advisory label only)
• Raised by another reviewer: No

Issue 3 (carried over, Low): reused connector modules are scored as single units. SHARED_CONNECTOR_MODULES (:179) still lists only four modules plus the -base suffix. Modules that other connectors depend on (for example the CDC base, file base or JDBC dialect modules in use by other connectors) are treated as self-contained units. A short list in the docstring of what is deliberately not covered would be enough if you prefer to keep it simple.

Issue 4 (carried over, Low): add persist-credentials: false to the checkout step in pr-impact-label-apply.yml:50 (and pr-impact-label.yml:45). The apply job holds pull-requests: write and stores the token in .git/config for the rest of the job. No PR code runs there, so it is not exploitable today; this keeps it that way if a step is added later.

Issue 5 (new, Low): PR description typos. Design point 7 reads "named impact, not impact" (the first word should presumably be risk), and the numbering runs 1-4, 7, 5, 6. Please fix it before the description is used as the dev-list reference.

2. Code Quality

2.1 Coding Standards

ASF license headers are present on all new files, the rules are table-driven, the security model is documented in the module docstring, and the action versions match the rest of the repository.

2.2 Test Coverage and Test Stability

tools/test_pr_impact_label.py has 80 test_ methods; they are pure in-memory unittest cases with fixed inputs (no network, clock or ordering dependency), and the apply path uses an in-memory GitHub fake. Several tests fail if the rule they name is removed (for example the docs rule, the three-level thresholds, the 250-line size boundary, head binding). The Run pr impact label tests step in backend.yml is in Code style. I could not compare against an earlier version of every assertion at this head, but the delta I checked in the previous round (63 assertion lines removed, 63 added in the level collapse) had no weakening. The two gaps are the 3000-file case (Issue 1) and the parent-pom case (Issue 2). Stability rating: Stable (tools/test_pr_impact_label.py).

CI for head 87aea81a: apache-side Build is pending; the fork run (https://github.com/davidzollo/seatunnel/actions/runs/39540199487) is queued and has not started. Notify test workflow and labeler pass; Evaluate impact is skipping because PR_IMPACT_ENABLED is unset, which is expected. So there is no finished CI result for this head yet, and I cannot say whether the Code style step that runs the new tests passes here.

2.3 Documentation Updates

No user-facing docs are needed. Please refresh the PR description for Issue 5 and, if the label rule table changes after Issue 2, keep it in sync with the module docstring.

3. Architectural Soundness

The structure is still the right one: pure scoring separated from GitHub I/O, a read-only first stage, a default-branch second stage, and one advisory write. I did not find a better location for this logic; rulesets or CODEOWNERS cannot express content-based triage. The three levels with "high only from what was touched" are easier for committers to act on than accumulated points.

4. Issue Summary

Number	Issue	Location	Severity
1	Files list capped at 3000 is evaluated silently (carried over)	tools/pr_impact_label.py:868	Medium
2	Parent poms (seatunnel-connectors-v2/pom.xml, e2e parent) scored as registration only	tools/pr_impact_label.py:147, :656	Medium
3	Reused connector modules scored as single units (carried over)	tools/pr_impact_label.py:179	Low
4	Optional persist-credentials: false on checkouts	pr-impact-label-apply.yml:50	Low
5	PR description typo and numbering	PR description	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers

• Source: none. The workflows are safe as written, and the scoring logic is consistent with the description except the Medium items above, which affect an advisory label that stays off until PR_IMPACT_ENABLED is set.
• CI: the fork Build for this head is still queued, so there is no finished result yet. Please let it complete and check that Code style (with the new test step) is green before merging.

Recommended fixes (before enabling the variable)

• Issue 1 and Issue 2, each with one small test.
• Issues 3 to 5 as small follow-ups.

Overall, a careful and well-tested design, and the rename and three-level simplification are clean. Thank you for the continued iteration!

[DanielLeens]

Thank you, @davidzollo, for another careful round. The branch has moved from 87aea81a (my last review) to 424e9ce1. I re-read the whole current head from scratch (the two workflows, tools/pr_impact_label.py, its 89 tests, and the backend.yml step), not only the delta. This is a static source review; I did not run the workflows or the test suite. The only things I executed were two small regex/scoring probes against the script's own functions with synthetic input (described under Issues 1 and 2).

All five items from my last review are fixed, and I found two new Medium issues that the earlier rounds missed. Sorry for missing these in the previous reviews; they are in code that was already there, so I label them as carryovers of my own oversight rather than regressions from your last push.

What Problem Does This PR Solve?

• User pain point: committers cannot cheaply order the review queue by blast radius, so a docs fix and an engine or credential change wait in the same line.
• Fix approach: pr-impact-label.yml (pull_request, read-only token) writes the level to the job summary and uploads only the PR number. pr-impact-label-apply.yml (workflow_run, default-branch code, the only run with pull-requests: write) recomputes the level from the API and writes exactly one impact: low|medium|high label, only if the PR head still equals workflow_run.head_sha. Both stay inert until the repository variable PR_IMPACT_ENABLED is true.
• One-sentence summary: an advisory, API-only triage label that cannot approve, block, merge or comment.

1. Code Change Review

1.1 Core Logic Analysis

Status of my previous findings (head 87aea81a -> 424e9ce1)

Previous finding	Status	Evidence at this head
Issue 1 (Medium): 3000-file cap evaluated silently	Resolved	evaluate adds truncated-file-list (+2) when changed_files > len(files) (tools/pr_impact_label.py:485-487); evaluate_pr passes changed_files (:957-958); test_truncated_file_list_is_medium pins it.
Issue 2 (Medium): connector parent poms scored as registration only	Resolved	_registration_facts now calls _parent_pom_facts for the two parent poms (:730), and grouping poms derived from the base checkout go through the same path (:548-549); non-<module> lines give parent-pom-change (+2) plus supply-chain checks. Tests at test_pr_impact_label.py:393-423.
Issue 3 (Low): reused connector modules treated as single units	Resolved	load_known derives reused from connector-to-connector <dependency> entries (:327-335) and evaluate promotes them to shared: (:568-569). I ran the same extraction over the poms of origin/dev: it yields 13 modules (connector-cdc-base, connector-cdc-mysql, connector-cdc-pg-base, connector-cdc-postgres, connector-common, connector-file-base, connector-file-base-hadoop, connector-file-bos, connector-file-cos, connector-file-oss, connector-file-s3, connector-http-base, connector-jdbc), matching the description.
Issue 4 (Low): persist-credentials: false	Resolved	Present in both checkouts (pr-impact-label.yml:49, pr-impact-label-apply.yml:52).
Issue 5 (Low): description typo and numbering	Resolved	Design point 7 now reads "named impact, not risk" and the list runs 1 to 7.

The earlier review by SEZ9 (dismissed) was written against the removed ai-merge-gate design and no longer applies to this head.

What changed since my last review

1. 6ee61cd5: the fixes above.
2. 42f1a9d2: MODULE_POM_RE now also matches split E2E sub-module poms (connector-jdbc-e2e/connector-jdbc-e2e-part-3).
3. 424e9ce1: compute_level (:676-684) returns high only for a high base (engine/api/.asf.yaml) or a critical fact (core-class, core-class-removed, persisted-format, option-removed, option-changed, workflow-permissions); facts alone reach medium at most. This matches the description table. I traced classify_path for the main shapes (engine/api tests and docs, docs/, .md, labeler metadata, registration files, connector main/test, platform test suites, shared E2E infrastructure, workflows) and each lands where the table says.

Runtime path (re-verified against the YAML on this head):

PR opened/reopened/synchronize/ready_for_review/edited
  -> pr-impact-label.yml  (pull_request, contents:read + pull-requests:read, checkout of base.sha, persist-credentials:false)
       python3 -I tools/pr_impact_label.py evaluate --pr N     -> job summary only
       upload artifact pr-impact/number (retention 1 day)
  -> pr-impact-label-apply.yml (workflow_run, default-branch code, pull-requests:write)
       if: repo == apache/seatunnel && PR_IMPACT_ENABLED == 'true' && event == pull_request && conclusion == success
       NUMBER = digits only from the artifact (tr -cd '0-9' | head -c 12)
       evaluate --pr NUMBER --expect-head <workflow_run.head_sha> --apply      (pr_impact_label.py:990-1004)
         -> GET pulls/N + pulls/N/files, evaluate(), head mismatch => "stale", nothing written
         -> apply_label: add the new label first, then remove other "impact: " labels (404 tolerated)

Workflow security: no pull_request_target; the only privileged trigger is workflow_run, which runs the default-branch file and script, and no PR code is checked out or executed in that run. The only ${{ }} inside a run: body is the integer PR number, and EXPECT_HEAD goes through env:. Token scopes are minimal. I found no new security issue in the YAML.

Does the normal path reach the changed logic? Yes: every PR event goes through evaluate once the variable is set. Is the reported problem real? Yes, the review queue has no blast-radius ordering today. The new problems below are in the scoring code, not the workflow structure.

1.2 Compatibility Impact

Fully compatible. No runtime, API, option, serialization or file-format change. The impact: label family is new, and plan_label_changes only touches names with that prefix, so the existing labeler with sync-labels is unaffected.

1.3 Performance / Side-Effect Analysis

Normally one short job per PR event: 1 + ceil(files/100) GETs, one label list, at most two label writes, plus an os.walk over the checkout for poms. The one exception is Issue 1 below, where a crafted line makes the regex run for minutes.

1.4 Error Handling and Logging

Write calls raise on any status of 300 or more (404 tolerated only when removing an absent label), and tokens are never printed.

Issue 1 (new): catastrophic regex backtracking in CONSTANT_LIST_RE lets a PR author stall the evaluation job and so suppress the label

• Location: tools/pr_impact_label.py:255-257 (CONSTANT_LIST_RE), reached through is_option_rule_line (:220-221) and is_declaration (:260-261), called from _main_facts (:796, :805, :814, :818).
• Problem: the pattern is (?:[\w.]*\b[A-Z][A-Z0-9_]+\b\s*,?\s*)+ followed by an anchored tail. The [\w.]* prefix inside a repeated group lets the engine split a run like AB.AB.AB. into groups in 2^n ways when the line does not end the expected way. I measured it with the script's own regex: "AB." * 16 + "token x" (55 chars) took 0.04 s, * 20 took 0.58 s, * 22 took 2.3 s, * 24 took 9.5 s, so the time doubles per extra token. I also called evaluate() with one added line in a Java file under a connector's src/main (a line of AB. repeated plus token x, so it passes the CREDENTIAL_RE gate): 18 repeats took 0.21 s, 20 took 0.70 s, 22 took 2.88 s. A line of about 100 characters would run for hours.
• Potential risk: the line is PR-controlled data, and both workflows have timeout-minutes: 10. The evaluation job then fails (the apply job only runs on conclusion == success), so the PR gets no impact label at all, and each rerun burns up to 10 runner minutes. The level is advisory, but the author of a risky PR can decide whether it is labelled. The workflow isolation is fine; this is a robustness problem of the scorer itself.
• Best improvement: remove the ambiguity instead of tuning it. For example, strip an optional wrapper and test each comma-separated element with a linear fullmatch such as [\w.]*[A-Z][A-Z0-9_]+, or skip the constant-list check for lines longer than a small bound (for example 200 characters), since real option constant lists are short. Add one regression test that feeds a 200-character "AB." * 60 + "token x" line through is_declaration and is_option_rule_line and requires a prompt result (a signal.alarm guard or a generous wall-clock bound keeps it from hanging the suite if it regresses).
• Severity: Medium
• Raised by another reviewer: No

Issue 2 (new): a changed default or type of an existing option is missed for the declaration style the connectors actually use

• Location: tools/pr_impact_label.py:224-225 (OPTION_KEY_RE, OPTION_TOKEN_RE), the collection at :593-599, and the check at :607-609.

• Problem: option-changed (critical, therefore high) only fires when a key appears on a removed line and an added line (both is the intersection of keys on the changed lines) and the default/type tokens differ. In the connectors, declarations are written one call per line, for example in KafkaBaseOptions:

  public static final Option<MessageFormat> FORMAT =
          Options.key("format")
                  .enumType(MessageFormat.class)
                  .defaultValue(MessageFormat.JSON)

  Changing only .defaultValue(...) (or .intType() to .longType()) leaves the Options.key(...) line as unchanged context, so both is empty and nothing fires. I confirmed it by calling evaluate() on a modified FooOptions.java under a connector with -.defaultValue(1000) / +.defaultValue(2000) and the key line as context: the result is impact: low, with only no-regression-test. The tests that pin this rule (test_pr_impact_label.py:274-281 and :324-327) use the single-line form Options.key("size").intType().defaultValue(10);, which is not how the repository writes options.

• Potential risk: the description lists "default/type changed" as a high trigger, and it is exactly the change that silently alters existing jobs. With the common formatting, it is labelled low.

• Best improvement: associate each changed line with its option by walking the patch hunk, context lines included, and remembering the most recent Options.key("...") line; flag option-changed when a defaultValue/*Type token on a removed line differs from the matching added line under the same key. Hunks carry three lines of context, which covers the usual key/type/default distance. Add a test with the multi-line form (and one where only .intType() changes).

• Severity: Medium

• Raised by another reviewer: No

Issue 3 (new, Low): the option-contract rule only runs for connector files, not for platform option classes

• Location: tools/pr_impact_label.py:565-599 (option collection lives in the main/shared branch); the platform branch (:527-536) only calls _main_facts.
• Problem: removing or changing an option key in a transform config class (seatunnel-transforms-v2) is never evaluated as option-removed/option-changed, so it stays medium. The description states the rule without that restriction.
• Best improvement: either run the same option collection for platform Java files, or state in the description and docstring that the option contract is checked for connector modules only.
• Severity: Low
• Raised by another reviewer: No

Issue 4 (new, Low): CI edits that add or change an action reference, or edit a script a privileged workflow runs, are only medium

• Location: tools/pr_impact_label.py:150-154 (WORKFLOW_SURFACE_RE).
• Problem: the surface regex looks for permissions, secrets., GITHUB_TOKEN, pull_request_target, workflow_run, id-token and : write. Replacing an uses: reference with a third-party action, or changing a file under .github/scripts or tools/ that a workflow with secrets executes, is scored medium. It also fires on comment lines that merely mention "permissions", which over-reports slightly.
• Best improvement: optional. Treat an added or changed uses: line that points outside actions/ as a supply-chain fact (+2, as for poms), and ignore # comment lines in the surface check.
• Severity: Low
• Raised by another reviewer: No

Issue 5 (new, Low): small layout nit in the script

• Location: tools/pr_impact_label.py:220-222 and :260-262
• Problem: is_option_rule_line and is_declaration sit between constant definitions with no blank lines around them (SERIAL_RE, DISABLE_RE follow immediately), which makes the constants block hard to scan.
• Best improvement: move the two helpers below the constants, or add the usual two blank lines.
• Severity: Low
• Raised by another reviewer: No

2. Code Quality Assessment

2.1 Coding Standards

ASF license headers are on all new files, the rules are table-driven, the security model is documented in the module docstring, and action versions match the rest of the repository. The new core functions (compute_level, _parent_pom_facts, load_known, evaluate) have docstrings that explain the why.

2.2 Test Coverage and Test Stability

tools/test_pr_impact_label.py has 89 test_ methods (matching the description). They are in-memory unittest cases with fixed inputs, and the apply path uses an in-memory GitHub fake. The file inserts its own directory into sys.path (:33), so python3 -I tools/test_pr_impact_label.py can import the script. The new tests for the truncated list, parent poms and derived reuse fail if the rule is removed. The gaps are the two new issues: nothing pins the multi-line option form (Issue 2) or a pathological line (Issue 1). Stability rating: Stable (tools/test_pr_impact_label.py, no network, clock or ordering dependency; this is a new file, so there is no earlier version to weaken).

CI for head 424e9ce1 (apache-side Build is failure; the real run is https://github.com/davidzollo/seatunnel/actions/runs/39600527455):

• Run / Code style (which contains the new Run pr impact label tests step), Check Helm Chart, labeler and Notify test workflow passed. Evaluate impact is skipped, as expected while PR_IMPACT_ENABLED is unset.
• Failed: Run / unit-test on 11 and 17, ubuntu and windows, all with LLMBooleanOutputTest.readsLegacyFactoryWrapperWithValidationDisabled:248 InvalidClass in seatunnel-transforms-v2; Run / Dependency licenses failed while downloading the Maven wrapper (ERROR 403: Forbidden, retried four times); mysql-cdc-connector-it (11, ubuntu-latest) was cancelled.
• Related to this PR: no, as far as I can tell. The PR touches no file in seatunnel-transforms-v2 or the Maven wrapper path, and the licenses job failed before any check ran. This is an inference from the job logs and the file list. I did not find a commit on dev that fixes the LLMBooleanOutputTest error, and the branch is 7 commits behind dev, so syncing with the latest dev and re-running the failed jobs is a reasonable next step (the re-run needs the licenses job to get a successful wrapper download).

2.3 Documentation Updates

No user-facing docs are needed (repository automation only). The PR description is accurate for the rules as implemented; if Issue 3 is resolved by documenting a connector-only scope, a sentence in the description and docstring is enough.

3. Architectural Soundness

3.1 Elegance

Long-term solution. Pure scoring is separated from GitHub I/O, the first stage is read-only, the second runs trusted default-branch code, and "high only from what was touched" is simple for committers to reason about.

3.2 Maintainability

Good, apart from the heuristics growing (regex tables plus facts). Keeping a test per rule, as the PR does, is the right mitigation. Issue 1 shows that regexes applied to PR-controlled lines deserve a bounded-time check.

3.3 Extensibility

New rules are table or fact additions with a matching test, so extension is cheap.

3.4 Historical-Version Compatibility

No impact: no runtime, API, option, serialization or checkpoint change, and the labels are new.

4. Issue Summary

Number	Issue	Location	Severity
1	Catastrophic regex backtracking in CONSTANT_LIST_RE on a PR-controlled line stalls the job and suppresses the label	tools/pr_impact_label.py:255-257 (via :221, :261)	Medium
2	Changed default/type of an existing option is missed for the multi-line declaration style	tools/pr_impact_label.py:224-225, :593-599, :607-609	Medium
3	Option-contract rule not applied to platform option classes	tools/pr_impact_label.py:565-599	Low
4	New or changed third-party uses: and workflow-run scripts are only medium; comment lines trigger the surface regex	tools/pr_impact_label.py:150-154	Low
5	Helper functions wedged between constant blocks	tools/pr_impact_label.py:220-222, :260-262	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

1. Blockers - must be fixed

• Issue 1: bound the constant-list check (rewrite without ambiguous nested quantifiers, or cap the line length) and add a regression test. It is a small change, and it keeps a PR author from deciding whether their PR gets labelled.
• Issue 2: detect default/type changes in the multi-line option style, with a test using the repository's real formatting. Without it the "default/type changed" high trigger in the description does not fire for the common case.
• CI: the failures above look unrelated to the diff. Please sync with the latest dev and re-run the failed jobs; the merge is gated on a green Build.

2. Recommended fixes - non-blocking

• Issues 3, 4 and 5.

Overall assessment: the five findings from the last round are all properly fixed and covered by tests, the workflow pair is still the right ASF-compliant shape, and the three-level rule now matches the description. The two Medium items are in scoring details that earlier reviews (mine included) did not exercise. The label stays off until PR_IMPACT_ENABLED is set, so there is time to fix them first.

Alternatives considered: I did not find a better place for this logic; rulesets or CODEOWNERS cannot express content-based triage, and a single-stage pull_request_target workflow would violate the ASF policy that this design deliberately follows.

Thank you again for the steady iteration on this!

[DanielLeens]

Thank you, @davidzollo, for the quick turnaround. The branch moved from 424e9ce1 (my last review) to c8e8b3a3. I re-read the delta in tools/pr_impact_label.py and tools/test_pr_impact_label.py in full and re-checked the two workflows (unchanged since my last review, still pull_request read-only plus workflow_run apply). This is a static review; the only things I executed were small probes that call the script's own evaluate() and is_constant_list() with synthetic input. I did not run the test suite.

All five items from my last review are fixed, and I found one new Medium issue in the newest rule (JDBC/CDC is high) plus two Low notes.

What Problem Does This PR Solve?

• User pain point: committers cannot cheaply order the review queue by blast radius, so a docs fix and an engine or credential change wait in the same line.
• Fix approach: an advisory impact: low|medium|high label. pr-impact-label.yml (pull_request, read-only token) writes the level to the job summary; pr-impact-label-apply.yml (workflow_run, default-branch code, the only run with pull-requests: write) recomputes it from the API and writes exactly one label if the head still matches. Both stay inert until PR_IMPACT_ENABLED is true.
• One-sentence summary: a label-only triage aid that cannot approve, block, merge or comment.

1. Code Change Review

1.1 Core Logic Analysis

Status of my previous findings (head 424e9ce1 -> c8e8b3a3)

Previous finding	Status	Evidence at this head
Issue 1 (Medium): exponential regex backtracking in CONSTANT_LIST_RE	Resolved	Replaced by is_constant_list (tools/pr_impact_label.py:288-300): length cap of 200 (:263), optional wrapper strip, then a non-repeating fullmatch per comma-separated element (:262). I fed "AB." * 66 + "token x" through it and it returned immediately. A regression test pins it (test_pathological_constant_list_line_is_decided_promptly).
Issue 2 (Medium): changed default/type missed in the one-call-per-line style	Resolved	option_changes (:324-351) walks the hunk, remembers the latest Options.key(...) and attributes defaultValue/*Type tokens to it. I probed a hunk with the key line as context and only .defaultValue(1000) -> .defaultValue(2000) changed: option-changed fires (level high). The same for .intType() -> .longType(). A description-only edit stays low.
Issue 3 (Low): option rule only for connectors	Resolved	collect_options is now also called for platform Java files (evaluate, platform branch).
Issue 4 (Low): third-party uses: and comment lines	Resolved	third_party_actions (:315) adds a +2 supply-chain fact for non-actions/, non-./ references; workflow_surface_changed (:310) skips # lines.
Issue 5 (Low): layout nit	Resolved	The helpers moved below the constants block.

What else changed: 683c1abf makes every production file of connector-jdbc and every connector-cdc-* module high (CORE_CONNECTOR_RE, :205; assigned at :694-698), tests and docs stay low. The description table was updated to match. I treat this as an agreed design decision (the replay numbers in the description are consistent with it) and only review its implementation. The merge of dev (02e54233) brought in python-sdk.yml and a codeql.yaml bump; neither is part of the PR's own five files (git diff over the PR file list shows the workflow pair, backend.yml, the script and its tests only).

Runtime path (unchanged, re-verified):

pull_request -> pr-impact-label.yml (read-only token, base checkout, persist-credentials:false)
  -> python3 -I tools/pr_impact_label.py evaluate --pr N   -> job summary + PR-number artifact
workflow_run -> pr-impact-label-apply.yml (default-branch code, pull-requests:write)
  -> evaluate --pr N --expect-head <head_sha> --apply  -> one "impact: *" label, stale head => nothing written

I found no new security issue in the workflows.

1.2 Compatibility Impact

Fully compatible. No runtime, API, option, serialization or file-format change; the impact: label family is new and does not touch the existing labeler labels.

1.3 Performance / Side-Effect Analysis

One short job per PR event. The earlier unbounded-time risk is gone: the constant-list check is linear and capped.

1.4 Error Handling and Logging

Unchanged: write calls raise on status 300 or more, tokens are never printed.

Issue 1 (new, Medium): a PR that touches JDBC/CDC production code plus any medium platform file is labelled medium, not high

• Location: tools/pr_impact_label.py:694-698 (base level selection), with core_connectors filled at :667.
• Problem: the base level is chosen by if platform: ... elif core_connectors: .... As soon as the PR contains one platform file whose level is medium (a root pom.xml, a file under tools/, seatunnel-core, a workflow without a permission change), the platform branch wins and core_connectors is never consulted. I called evaluate() with the same synthetic JDBC production change in four PRs: alone it gives high (core-connector); together with tools/foo.py, with the root pom.xml, or with a seatunnel-core main file it gives medium (platform). The description says "any production file of the core connectors ... is high", and a JDBC fix that also bumps a root pom property or touches a tooling script is a very ordinary PR shape.
• Potential risk: the label under-reports exactly the PRs the new rule was added for, depending on an unrelated extra file, and it is the kind of mismatch between the documented table and the behavior that committers will notice only after trusting the label.
• Best improvement: compute the base as the maximum of the applicable bases, for example base = max(platform_base, HIGH if core_connectors else LOW, ...), or move the core_connectors check ahead of platform (keeping the class naming consistent). Add one test with JDBC production plus a tools/ or root-pom.xml file that expects high.
• Severity: Medium
• Raised by another reviewer: No

Issue 2 (new, Low): a non-option edit after an option inside the same hunk can be attributed to that option

• Location: tools/pr_impact_label.py:324-351 (option_changes, current is only reset at a hunk header).
• Problem: after Options.key("a") the walker keeps a as the current key until the next key line. A changed non-option declaration further down the same hunk that contains .stringType() or defaultValue(...) (for example a helper constant in an options class) is counted as a default/type change of option a. I probed a hunk where only Option<String> B = X; became B = Y.stringType(); and got option-changed (high).
• Potential risk: occasional false high on option classes; advisory only.
• Best improvement: also reset current on a line that begins a new top-level declaration (for example public static final at declaration indentation) or on a blank line followed by a declaration. Optional.
• Severity: Low
• Raised by another reviewer: No

Issue 3 (new, Low): is_constant_list is slightly looser than the pattern it replaced

• Location: tools/pr_impact_label.py:262
• Problem: the old pattern required a word boundary before the upper-case run; [\w.]*[A-Z][A-Z0-9_]+ accepts a lone camel-case identifier that merely ends in two capitals (secretKeyAB returns True in my probe). Such a line would be treated as a declaration and not as credential handling. Realistic collisions are rare.
• Best improvement: require the final segment after the last . to be fully upper-case, e.g. (?:[A-Za-z_]\w*\.)*[A-Z][A-Z0-9_]+.
• Severity: Low
• Raised by another reviewer: No

2. Code Quality Assessment

2.1 Coding Standards

ASF headers are on all new files; the new functions (is_constant_list, option_changes, third_party_actions, workflow_surface_changed) have docstrings that explain why (the backtracking history, why the hunk is walked).

2.2 Test Coverage and Test Stability

Tests were added for the new behavior (ReviewFindingsOn424e9ceTest: pathological line, constant-list recognition, multi-line default and type change, description-only edit, key attribution across hunks, platform option classes, comment lines, third-party actions). Two existing assertions were changed in 683c1abf (the JDBC dialect and the shared CDC-mysql cases moved from low/medium to high) and one shared-CDC test was removed. I checked that these follow from the agreed rule change (dialects and CDC production are now high by design) rather than from loosening, and the new JDBC/CDC test pins both the high production case and the low test/docs case. The gap is Issue 1: no test combines a core-connector change with a medium platform file. Stability rating: Stable (in-memory unittest, no network or ordering dependence).

CI for head c8e8b3a3 (the apache-side Build check is still in progress; the real run is the fork run https://github.com/davidzollo/seatunnel/actions/runs/39714596631, completed with failure):

• Failed: unit-test on 11/17 ubuntu and windows with LLMBooleanOutputTest.readsLegacyFactoryWrapperWithValidationDisabled:248 InvalidClass in seatunnel-transforms-v2; engine-k8s-it on 11 and 17 with curl: (35) SSL_ERROR_SYSCALL ... update.k3s.io (a download failure).
• Unrelated to this PR: this PR touches no file of seatunnel-transforms-v2, and the head does not contain cc54bb43 ([Fix][Transform-V2] Restore archived LLM wrapper deserialization, #12938), which is on dev and addresses that test. The branch is 3 commits behind dev.

2.3 Documentation Updates

No user-facing docs are needed. The PR description matches the code except for Issue 1.

3. Architectural Soundness

Unchanged from my last review: scoring is separated from GitHub I/O, the read-only stage and the privileged apply stage are correctly split, and the rules are table-driven with a test each. Issue 1 is a precedence slip in the base-level selection, not a structural problem.

4. Issue Summary

Number	Issue	Location	Severity
1	JDBC/CDC production change plus a medium platform file is labelled medium	tools/pr_impact_label.py:694-698	Medium
2	Non-option edit after an option in the same hunk attributed to that option	tools/pr_impact_label.py:324-351	Low
3	is_constant_list accepts identifiers ending in capitals	tools/pr_impact_label.py:262	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

1. Blockers - must be fixed

• Issue 1: make the core-connector base combine with the platform base instead of being skipped, and add the mixed-PR test.
• CI: the failures above are unrelated to the diff and are already fixed on dev (#12938) or environmental (k3s download). Please sync with the latest dev and re-run; the merge is gated on a green Build.

2. Recommended fixes - non-blocking

• Issues 2 and 3.

Overall assessment: all five earlier findings are properly fixed and tested, and the new JDBC/CDC rule is implemented as described except for the precedence slip in Issue 1. The label stays off until PR_IMPACT_ENABLED is set, so there is time to fix it first.

Thank you again for the steady iteration!