fix(workflows): serve canonical builtin service URLs, never a stored Docker-internal host - #4982
Conversation
dosubot
Bot
added
size:M
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA new ChangesService URL Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
🤖 The AI agent says: What I need reviewed This removes the #4936 builtin-invoke FE workaround and fixes the real root cause in the
Verified: new unit tests + live EE dev — the previously broken agent app now invokes A separate question for a maintainer: should we also ship a one-off data migration to null |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e69783d-2d36-4bce-b594-b18daac74cd9
📒 Files selected for processing (3)
api/oss/src/core/workflows/service.pyapi/oss/tests/pytest/unit/workflows/test_service_url_resolution.pyweb/packages/agenta-entities/src/workflow/state/runnableSetup.ts
| if revision.data: | ||
| resolved_url = _resolve_service_url(revision_data=revision.data) | ||
| if resolved_url and resolved_url != revision.data.url: | ||
| revision = revision.model_copy( | ||
| update={ | ||
| "data": revision.data.model_copy( | ||
| update={"url": env.agenta.services_url.rstrip("/") + path} | ||
| ) | ||
| "data": revision.data.model_copy(update={"url": resolved_url}) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ensure builtin URLs never fall back to stored values.
Line 2358 still returns the original revision_data.url for a builtin URI when infer_url_from_uri() returns no path. That preserves the poisoned Docker/internal URL this PR is trying to mask. Also update the read/simple-data callers to clear stale URLs when the resolver returns None, instead of only overwriting on truthy values.
Proposed fix
- if resolved_url and resolved_url != revision.data.url:
+ if resolved_url != revision.data.url:
revision = revision.model_copy(
update={
"data": revision.data.model_copy(update={"url": resolved_url})
}
)- if uri:
- _, kind, _, _ = parse_uri(uri)
- if kind == "builtin" or not url:
- path = infer_url_from_uri(uri)
- if path:
- url = env.agenta.services_url.rstrip("/") + path
-
- return url
+ if not uri:
+ return url
+
+ _, kind, _, _ = parse_uri(uri)
+ path = infer_url_from_uri(uri)
+
+ if kind == "builtin":
+ return env.agenta.services_url.rstrip("/") + path if path else None
+
+ if not url and path:
+ return env.agenta.services_url.rstrip("/") + path
+
+ return url resolved_url = _resolve_service_url(revision_data=revision_data)
if resolved_url:
data_dict["url"] = resolved_url
+ elif resolved_url != revision_data.url:
+ data_dict.pop("url", None)
return SimpleWorkflowData(**data_dict)Also applies to: 2339-2375
| // Resolve service URL: prefer stored url, fall back to building from URI | ||
| const serviceUrl = | ||
| entity.data.url?.replace(/\/+$/, "") ?? buildServiceUrlFromUri(entity.data.uri) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat blank URLs as missing before falling back to the URI.
With ??, data.url: "" skips buildServiceUrlFromUri(...), so a valid URI can still produce no invocation URL.
Proposed fix
- const serviceUrl =
- entity.data.url?.replace(/\/+$/, "") ?? buildServiceUrlFromUri(entity.data.uri)
+ const storedServiceUrl = entity.data.url?.trim().replace(/\/+$/, "")
+ const serviceUrl = storedServiceUrl || buildServiceUrlFromUri(entity.data.uri)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Resolve service URL: prefer stored url, fall back to building from URI | |
| const serviceUrl = | |
| entity.data.url?.replace(/\/+$/, "") ?? buildServiceUrlFromUri(entity.data.uri) | |
| // Resolve service URL: prefer stored url, fall back to building from URI | |
| const storedServiceUrl = entity.data.url?.trim().replace(/\/+$/, "") | |
| const serviceUrl = storedServiceUrl || buildServiceUrlFromUri(entity.data.uri) |
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Backend (root cause): add _resolve_service_url, which always recomputes the
canonical URL from the URI for builtin apps/evaluators (ignoring any stored value), and
keeps the stored value for non-builtin (custom) workflows. Use it on all read paths. On
commit, drop any incoming data.url for builtin apps so a bad value is never persisted
again.
I don't think we need to make any changes to the backend. The issue was in the agent making a bad call and calling this endpoint. Let's not change the business logic for the backend, so please revert the change and just revert the frontend change there.
…host
Builtin app/evaluator service URLs are deterministic from the URI
(`{services_url}/{key}/{version}`), but the read path only recomputed them
when `data.url` was empty. An agent self-update (`commit_revision`) could
persist a Docker-internal host (`http://agenta-agent:8000/v0/invoke`) into
`data.url`, which the API then served verbatim; the browser could not resolve
it (ERR_NAME_NOT_RESOLVED on the builtin `/invoke` call).
- Resolve builtin URLs from the URI on every read (`_resolve_service_url`),
ignoring any stored value; non-builtin URLs keep the stored value.
- Stop persisting a URL for builtin apps on commit.
- Revert the PR #4936 `invocationUrlAtomFamily` FE workaround so builtins use
the normal `data.url` / URI path again (it only ever covered completion/chat,
never agent, so it did not even fix the broken case).
Claude-Session: https://claude.ai/code/session_014emxsaD8EFiNP3m3f56nhD
65cbca9 to
b955fd6
Compare
dosubot
Bot
added
size:S
|
🤖 The AI agent says: Shipped as FE-revert-only, per the owner's review. The commit now contains just the The backend change to |
1 participant
Symptom
A builtin agent app could not be invoked from the playground. The browser tried to
POSTto a Docker-internal host and failed before reaching the service:Root cause
Builtin app/evaluator service URLs are deterministic from the URI
(
{AGENTA_SERVICES_URL}/{key}/{version}). The backend was supposed to never store a URLfor them — but the read path (
_normalize_revision_for_read,_get_service_url,_build_simple_workflow_data) only recomputed the URL whendata.urlwas empty, andthe commit path only skipped setting one (it never stripped an incoming value).
On 2026-06-29 an agent self-update via the
commit_revisionplatform op persisteddata.url = http://agenta-agent:8000/v0/invoke(the agent service's internal address)onto a builtin agent revision. Because that value was non-empty, the API served it
verbatim, and the browser cannot resolve
agenta-agent. Stored DB values confirmed it:The FE workaround added in #4936 (
invocationUrlAtomFamilyrouting builtins throughresolveBuiltinAppServiceUrl) only ever coveredcompletion/chat, neveragent, so itdid not fix the broken case — and it was unnecessary for completion/chat, which had
run for ~3 months on the plain
data.urlpath since #3883 removed that helper from invoke.Change
_resolve_service_url, which always recomputes thecanonical URL from the URI for builtin apps/evaluators (ignoring any stored value), and
keeps the stored value for non-builtin (custom) workflows. Use it on all read paths. On
commit, drop any incoming
data.urlfor builtin apps so a bad value is never persistedagain.
invocationUrlAtomFamilyhunk so builtins use the normaldata.url/ URI path. (resolveBuiltinAppServiceUrlstays; it is still used bydeploymentUrlAtomFamilyandstore.ts.)Verification
test_service_url_resolution.py(8 cases) + existingtest_retrieve_resolve.pygreen.pi-agents) nowinvokes against
http://144.76.237.122:8280/services/agent/v0/invoke→ HTTP 200.The run executes end to end; the only remaining failure is a downstream Anthropic model
error (
"thinking.type.enabled" is not supported for this model), unrelated to this fix.https://claude.ai/code/session_014emxsaD8EFiNP3m3f56nhD