Skip to content

fix(workflows): keep duplicate-name ambiguity local to catalog entries - #7883

Closed
0xacee wants to merge 4 commits into
Osmantic:mainfrom
0xacee:fix/workflow-exact-owner-20261021
Closed

0xacee wants to merge 4 commits into
Osmantic:mainfrom
0xacee:fix/workflow-exact-owner-20261021

Conversation

@0xacee

@0xacee 0xacee commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Closes #4191. Keep duplicate exact n8n names local to the affected catalog entry, and refuse execution-history lookup when the catalog cannot identify one workflow.

For two workflows named Daily Digest, GET /api/workflows currently chooses the first row's ID, active state and execution count; the history route also reads the first row's executions. The candidate returns that catalog entry as ambiguous with a recovery message and no selected ID, preserves unrelated catalog entries, and returns HTTP 409 before fetching ambiguous history. The README documents the response.

This is the remaining scope after integration with current main: patil2001's merged #7905 supplies exact name matching and duplicate refusal for deletion, and #7855 supplies complete inventory pagination. Those changes, the shipped import/activation contracts and removal behavior are preserved. The earlier catalog-locality review is addressed: #7883 (comment).

Prior contributions: bettercalln1ck's closed #5554, patil2001's closed #4386, and Hoang130203's closed #2749 previously identified or addressed name matching; their credit is retained.

AI Assistance

0xacee actively leads this contribution and its scope and quality requirements, collaborating with Codex on implementation, analysis, regression tests, review and maintenance. Codex executed the recorded validation commands; a separate Codex review checked the final merge and retained upstream behavior.

Release Lane

  • Mainline change targeting main

Changed Surface

  • Dashboard API / host agent — workflow inventory and execution-history routes
  • API contract documentation and route regression tests.

Risk And Validation

Candidate ce9d1e672043df37f6da85912a0c0e4e91ec46ad, integration base fce1a2281343cd9de1ca0622b4bbf28f3469523e. The old published head 6f4a58f9de5a460f84dfadb452850574e2b54003 remains an ancestor through a normal merge; no history rewrite.

Original exact-match regression against current main: 12 passed, 3 failed
  failures: both mixed-catalog orderings and ambiguous execution lookup
Candidate exact-match + shipped workflows + pagination + API-key suites:
  113 passed
git diff --check: passed
Commit author/committer gate: 4 commits passed
Gitleaks over upstream/main..HEAD: no leaks

Validation used credential-free Ubuntu 24.04 WSL2, Python 3.12 and the real FastAPI routes/HTTP client against a disposable loopback n8n server. Assertions inspect actual emitted requests: ambiguous history performs no executions request; duplicate deletion still emits no DELETE; unique and partial-name controls retain current-main behavior. The Dashboard API workflow executes pytest tests/, including this regression.

Operational Change Check

  • This is an operational change and scoped validation is recorded above.

The narrower equivalent covers the changed API response and request-selection boundary. Installed n8n/container/browser/model/fleet qualification was not run; retain normal maintainer release review. There is no persisted-state migration. Reverting the residual change restores first-row ambiguity for catalog/history while retaining current main's deletion guard.

Hosted CI

Current-head checks are pending after this merge update; earlier results do not qualify this head. The previous head's integration-smoke failure was in the separate Pixel IPv6 loopback fixture (ECONNRESET); #8013 tracks its independently reviewed fixture correction. This PR includes no unrelated Pixel changes.

@Lightheartdevs

Copy link
Copy Markdown
Collaborator

Thanks for replacing fuzzy workflow selection with exact matching and protecting owner-created workflows from accidental deletion.

Advisory status: not ready yet

Quickest path to merge:

  • In ods/extensions/services/dashboard-api/routers/workflows.py:169, handle duplicate-name ambiguity per catalog entry. _matching_n8n_workflow() currently raises out of the entire GET /api/workflows loop: two n8n workflows named Daily Digest prevent every unrelated catalog entry from being returned. I reproduced this with a two-entry catalog and three n8n rows: current main returns HTTP 200 with both entries; this head returns only HTTP 409. Preserve the exact-match/409 safeguards for targeted removal, but let the listing return unaffected entries and mark the ambiguous entry explicitly without selecting an n8n ID or advertising an actionable normal state.
  • Extend ods/extensions/services/dashboard-api/tests/test_workflow_exact_match.py with a mixed catalog containing one ambiguous and one uniquely matched entry. Assert the unrelated entry is still returned, ambiguity is visible, and both removal aliases still refuse duplicate targets without sending DELETE. The current single-entry catalog test explicitly expects the whole-list 409 and misses that collateral loss.

What I checked:

  • Reviewed the resolver, import-name producer contract, catalog/execution/removal callers, and CI wiring against current main c5cc2e3d.
  • The new 14-case suite passes locally; independently reproduced the mixed-catalog behavior through the real FastAPI route on main and this head with stubbed n8n inventory.
  • All 71 reported checks pass; git diff --check is clean. The finding concerns behavior currently accepted by the tests.

The exact-match deletion protection is useful; keeping ambiguity local to its catalog entry would make this ready.

@roshangupta00750

Copy link
Copy Markdown
Contributor

The integration-smoke failure here isn't from this change. It's one test in the Pixel node suite:

not ok 1232 - real IPv6 listener is selected even when the IPv4 proxy resets connections
  location: 'ods/extensions/services/pixel-agent/tests/gateway_loopback.test.mjs:134:1'
  error: 'socket hang up'
  code: 'ECONNRESET'

The cause is in the test file. Both real-listener tests use the shared gatewayFetch, which caches the chosen address family per port for 5 s. The IPv4-only test caches 127.0.0.1 for its random port. When the kernel happens to give the IPv6 test the same port number, discovery is skipped and the POST goes to the IPv4 server that resets every connection. I can make it fail every time by binding the second test to the first test's port. It passes once each test uses its own createLoopbackGatewayFetch() (same production transport, separate cache).

It's rare. I found it once in the last 80 failed test-linux runs. A re-run of integration-smoke should clear it for this PR.

@0xacee 0xacee changed the title fix(workflows): require an unambiguous complete n8n workflow name fix(workflows): keep duplicate-name ambiguity local to catalog entries Oct 24, 2026
@Lightheartdevs

Copy link
Copy Markdown
Collaborator

Thanks for this. On 2026-10-24 we re-audited every open PR against current main and are merging only fixes for failures that users or our test fleet actually hit.

The substring-matching bug in #4191 was fixed by #7905 (3190949). What's left is two n8n workflows with exactly the same name, on an endpoint no dashboard UI calls.

If you've seen this fail on a real install of current main, please open an issue with the steps and logs, and a focused PR for it is very welcome.

🤖 Generated with Claude Code

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.

fix: Substring workflow name match hits wrong n8n workflow

3 participants