Skip to content

fix(test): accept Slack HTTP connector proxy flows [MST-9564]#657

Merged
tmatup merged 1 commit into
mainfrom
fix-MST-9564
May 13, 2026
Merged

fix(test): accept Slack HTTP connector proxy flows [MST-9564]#657
tmatup merged 1 commit into
mainfrom
fix-MST-9564

Conversation

@tmatup

@tmatup tmatup commented May 8, 2026

Copy link
Copy Markdown
Member

Summary

  • Teach the shared flow checker to accept connector-backed HTTP v2 proxy nodes when they target the expected connector and carry real connection/folder bindings.
  • Keep manual HTTP requests from satisfying connector-required eval checks.
  • Update the Slack channel description and Slack weather pipeline checks to use the connector-target assertion.
  • Add focused tests for native connector nodes, HTTP proxy connector bindings, and manual HTTP rejection.

Validation

  • uv run python -m py_compile tests/tasks/uipath-maestro-flow/_shared/flow_check.py tests/tasks/uipath-maestro-flow/_shared/test_flow_check.py tests/tasks/uipath-maestro-flow/multi_node/slack_channel_description/check_channel_description.py tests/tasks/uipath-maestro-flow/multi_node/slack_weather_pipeline/check_slack_weather_pipeline.py
  • uv run --with pytest pytest tests/tasks/uipath-maestro-flow/_shared/test_flow_check.py — 22 passed (post-rebase)
  • Captured-run structural probe against run 2026-05-08_04-46-07 accepted the Slack connector target in both Slack .flow artifacts.
  • Re-confirmed against run 2026-05-13_03-38-18 during the cross-ticket triage on 2026-05-13: skill-flow-slack-channel-description/00 and skill-flow-slack-weather-pipeline/00 both still fail on the legacy assert_flow_has_node_type(["uipath.connector"]) shape because the agent correctly produces a core.action.http.v2 proxy node; this PR's assertion accepts that shape and resolves both failures.
  • git diff --check

Rebase note

Rebased onto current origin/main on 2026-05-13 to clear unrelated extra_forbidden Pydantic errors in Run skill smoke tests. Those errors came from origin/main's run_limits migration to tests/experiments/default.yaml (PR #721, merged after this PR was opened) and were unrelated to this PR's changes. New head: 950e5ee0. All 22 unit tests still pass.

Related

Surfaced and triaged alongside five sibling PRs from run 2026-05-13_03-38-18, all addressing checker/skill drift against the same e2e suite:

Issue

MST-9564

@github-actions

github-actions Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @tmatup's task in 2m 28s —— View job


Review: PR #657 — accept Slack HTTP connector proxy flows

Summary

Teaches the shared flow_check.py test helper to recognize Slack connector calls that arrive as core.action.http.v2 proxy nodes (with real connector bindings), not just native uipath.connector.* nodes. Adds assert_flow_uses_connector_target(), refactors node iteration into _iter_flow_nodes(), updates two eval checks to use the new assertion, and adds focused unit tests.

Change-by-Change Review

1. tests/tasks/uipath-maestro-flow/_shared/flow_check.py (+64/−8)

Severity: Low

Clean, well-structured change. Observations:

  • _iter_flow_nodes extraction (line 259–263): Good DRY refactor — assert_flow_has_node_type and the new function both benefit.
  • assert_flow_uses_connector_target (line 88–133): Logic is sound. Two acceptance paths (native connector node, HTTP proxy with real bindings) and a clear rejection path. The five-part conjunction on lines 121–127 is strict enough to reject manual HTTP without being fragile.
  • _non_empty_binding_value (line 267–268): Correctly rejects empty strings, whitespace-only, and the "ImplicitConnection" sentinel. One observation: the code at line 117 checks body.get("targetConnector") or body.get("connectorKey") but no test exercises the connectorKey fallback — if this alternate key name exists in real payloads, a test would prevent future regressions. Low priority.
  • Missing return type annotation on _iter_flow_nodes (line 259) — rest of the module is reasonably typed. Minor style nit.

2. tests/tasks/uipath-maestro-flow/_shared/test_flow_check.py (+71/−1)

Severity: OK

Good test coverage. Three new tests cover the three important paths: native connector node acceptance, HTTP proxy binding acceptance, manual HTTP rejection. The _write_flow helper update to accept dicts is clean.

One missing edge case: a test where connectionId or connectionFolderKey is an empty string (as opposed to "ImplicitConnection") — _non_empty_binding_value handles this, but it's untested. Low priority.

3. tests/tasks/uipath-maestro-flow/multi_node/slack_channel_description/check_channel_description.py (+6/−5)

Severity: OK

Straightforward swap from assert_flow_has_node_type(["uipath.connector"]) to assert_flow_uses_connector_target("uipath-salesforce-slack"). The comment accurately explains the two representation paths.

4. tests/tasks/uipath-maestro-flow/multi_node/slack_weather_pipeline/check_slack_weather_pipeline.py (+7/−4)

Severity: Low

The combined check assert_flow_has_node_type(["uipath.connector", "core.action.http"]) is now split into assert_flow_uses_connector_target("uipath-salesforce-slack") + assert_flow_has_node_type(["core.action.http"]). Note that when the Slack call is an HTTP proxy node, it also satisfies the core.action.http substring hint — so a flow with only the Slack proxy node and no separate weather API call would still pass the HTTP type check. In practice the output assertion ("warm office today" / "cold office today") catches this shortcut, so no functional gap. Just worth being aware of.

What's Missing

  • connectorKey fallback test: flow_check.py:117 falls back to body.get("connectorKey") but no test exercises this path. If this key appears in real payloads, add a test. (Low)
  • Empty-binding edge case test: _non_empty_binding_value rejects "" and " ", but only "ImplicitConnection" is tested via the manual HTTP rejection test. (Low)

Area Ratings

Area Status Notes
Frontmatter N/A No skill SKILL.md changed
E2E Tests OK Eval checks updated; unit tests added
Skill Body N/A No skill docs changed
References & Assets N/A No references changed
Repo Hygiene OK No secrets, no cross-skill refs, changes scoped correctly

Issues for Manual Review

  • Verify "ImplicitConnection" is the only sentinel value that needs rejection — if other provisioning-placeholder strings exist in real .flow files, _non_empty_binding_value may need extending.
  • Confirm the connectorKey fallback on line 117 reflects a real payload variant vs. speculative future-proofing.

Conclusion

Well-structured change with clear motivation and good test coverage. The new assertion correctly distinguishes connector-proxied HTTP from manual HTTP. No blocking issues — the two low-severity test gaps are optional improvements. Approve.

@tmatup tmatup closed this May 13, 2026
@tmatup tmatup reopened this May 13, 2026
@tmatup tmatup changed the title fix(MST-9564): accept Slack HTTP connector proxy flows fix(test): accept Slack HTTP connector proxy flows [MST-9564] May 13, 2026
Allow connector-backed HTTP v2 proxy nodes to satisfy Slack connector eval checks when they carry the expected targetConnector plus real connection and folder bindings. Keep manual HTTP requests from satisfying connector checks, and add focused helper coverage.
@tmatup

tmatup commented May 13, 2026

Copy link
Copy Markdown
Member Author

Review Response

The Claude Code Review bot (auto-run on PR open) approved this PR with two low-priority observations and two "issues for manual review". Addressing each:

1. connectorKey fallback at flow_check.py:117 has no test coverage (bot, Low)

Noted; keeping the fallback uncovered for now. The fallback was speculative future-proofing — I haven't observed connectorKey in any real .flow artifact from runs to date. If/when we see it in production payloads, adding a test is a one-liner. Removing the fallback now would create a regression risk if the alternate key is silently introduced by the connector platform.

2. Empty-string binding edge case for _non_empty_binding_value is untested (bot, Low)

Noted. _non_empty_binding_value rejects "" and " " and "ImplicitConnection"; only the last is exercised by the manual-HTTP rejection test. Will add an explicit empty-string test in a small follow-up if reviewers want the coverage; not blocking on it because the function is small and obviously correct by inspection.

3. Missing return type annotation on _iter_flow_nodes (bot, Style nit)

Skip — annotating one private helper while the rest of the module is unannotated would create inconsistency. The whole module is a candidate for a typing pass, but that's a separate cleanup.

4. core.action.http substring hint also matches the Slack proxy node (bot, observation)

Noted; no functional gap. As the bot itself notes, the output assertions ("warm office today" / "cold office today") catch any flow that omits the actual weather API call. Tightening the substring would force a more brittle check.


CI fix

The Run skill smoke tests check failed against current origin/main with:

Invalid value: Failed to load experiment 'experiments/default.yaml':
Invalid experiment definition: 4 validation errors for ExperimentDefinition
https://errors.pydantic.dev/2.13/v/extra_forbidden  (× 4)

Root cause: PR was opened 2026-05-08 against the old experiment-YAML shape. Origin/main migrated tests/experiments/default.yaml to the run_limits block on 2026-05-11 (commit 40f98c1 fix(tests): migrate experiment YAMLs to run_limits schema (#721)), and the new ExperimentDefinition Pydantic model has extra='forbid' for the moved keys. This is base drift, not a PR defect — the PR only touches 4 Slack-checker files.

Rebased onto current origin/main and force-pushed. New head: 950e5ee0. 22/22 flow_check.py unit tests still pass. CI re-run pending.

Nits: none beyond the items above.

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

@tmatup tmatup merged commit f2eaf3f into main May 13, 2026
12 checks passed
@tmatup tmatup deleted the fix-MST-9564 branch May 13, 2026 21:33
charlesliu9 pushed a commit that referenced this pull request May 19, 2026
Allow connector-backed HTTP v2 proxy nodes to satisfy Slack connector eval checks when they carry the expected targetConnector plus real connection and folder bindings. Keep manual HTTP requests from satisfying connector checks, and add focused helper coverage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants