Skip to content

arazzo-executor: retain document identities and load source graphs - #286

Merged
SVilgelm merged 2 commits into
mainfrom
feat/arazzo-source-graph
Sep 10, 2026
Merged

arazzo-executor: retain document identities and load source graphs#286
SVilgelm merged 2 commits into
mainfrom
feat/arazzo-source-graph

Conversation

@SVilgelm

@SVilgelm SVilgelm commented Sep 10, 2026

Copy link
Copy Markdown
Member

Add the optional source-graph executor feature, following the checked preparation
work in #285. Complete documents now retain opaque registry handles, retrieval
locations, resolved Arazzo $self identities, effective reference bases and written
versions. Supplied Arazzo documents are fully parsed and indexed before resolution;
aliases and source/API-base overrides are scoped to the owning document.

Source traversal uses caller-configured roas::Loader fetchers, with shared
documents, explicit back edges, separate document/depth budgets, root-source selection,
and located errors that preserve readable parts of a graph. Later identity discovery
can repair an earlier unresolved reference, including a newly discovered shorter
path. A qualified operation may prepare with an unrelated source missing; a bare
operation ID still requires enough sources to prove uniqueness.

The loader adds unchanged-document/retrieval-metadata APIs with default methods for
existing sync/async fetchers. HTTP fetchers expose final redirect URLs and retain
caller redirect/timeout policy. YAML sniffing respects explicit content types and,
when absent or generic, either the requested or final URL's extension. Thus
/source.yaml redirecting to /blob retains legacy parsing behavior.
Legacy reference readers retain requested-URI rewriting and store only one full
document plus changed-reference strings. A raw view is materialized only on demand,
without another fetch. Shared-document APIs let the loader, graph and cloned Options
retain the same immutable raw value; both full views are retained only if both are
requested. All loader types are also exported at the roas crate root.
Options::source remains compatible, and explicit source/base overrides take precedence.

The CLI keeps selected-workflow source discovery by default. New flags expose
--source-document, --load-all-sources, --source-max-documents,
--source-max-depth, and --allow-source-retrieval-aliases; none enables IO beyond
the existing --load policy. Canonical Arazzo identities are strict by default;
retrieval aliases are an explicit compatibility extension. Fetched unrelated failures
are reported without unnecessarily blocking a qualified checked run. Each diagnostic
is rendered once with its owning document identity, location and source alias, without
opaque registry IDs; quiet mode suppresses optional warnings but retains failure details.
Relative sources under a non-hierarchical $self name the two remedies: an absolute
source URL or a hierarchical $self.

crates/roas-cli/tests/fixtures/source-graph/root.json links JSON/YAML branches
sharing shared.yaml and api.json, with shared.yaml pointing back to the root.
For identity resolution, a document retrieved from https://example.test/cache/child.json
with "$self":"../identity/child.json" is identified by
https://example.test/identity/child.json, and resolves its relative sources there.

Recognized families are Arazzo 1.0/1.1, OpenAPI 2.0/3.0/3.1/3.2, and AsyncAPI
2.6/3.0/3.1. API documents retain complete raw values with model-checked versions,
not structural/schema validation. Broker execution, external workflow calls,
referenced OpenAPI Path Items and relative API-server resolution remain deferred.

The additive loader/fetcher APIs bump roas to 0.20.1 and roas-http-fetcher to 0.2.5;
the executor remains on unreleased 0.2.0. Those dependency minimums ensure downstream
builds receive the metadata APIs. Consumers inherit the workspace dependency versions
while explicitly preserving their existing feature selections. No existing execution
signatures are changed.

Add optional owner-scoped source registries, canonical Arazzo identity resolution, bounded loading and located partial-graph diagnostics. Preserve raw documents and redirect metadata through additive loader/fetcher APIs, and expose graph policy in the CLI without changing execution signatures.

Assisted-by: Codex
Signed-off-by: Sergey Vilgelm <sergey@vilgelm.com>
Copilot AI lite review requested due to automatic review settings September 10, 2026 03:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@SVilgelm

Copy link
Copy Markdown
Member Author

Reviewed at 5acb74a, based on current main (dbdbcbb). cargo fmt --all --check, cargo clippy --workspace --all-features --all-targets -- -D warnings, doctests, and cargo nextest run --workspace --all-features (3343 tests) are green, as are cargo machete and cargo deny check for the new optional roas / roas-asyncapi dependencies (the yanked chacha20 advisory is pre-existing, via salvo_core). -p roas-arazzo-executor builds both with default features and with --no-default-features --features source-graph, so the new module is properly gated.

The traversal design holds up well: the retry sweep in Traversal::next is bounded by successful inserts, so later identity discovery can repair an earlier reference without looping; failed prevents refetching; and cycles reports back edges as data rather than discarding the readable graph. Splitting identity ($self, resolved against the retrieval URI) from retrieval location, and making retrieval aliases opt-in, is the right default.

Confirmed Issues

1. roas-http-fetcher now sniffs YAML from the redirect target, breaking existing fetch callers — Medium

crates/roas-http-fetcher/src/lib.rs:156 and :193 changed parse_body(uri, …) to parse_body(&retrieval, …), and is_yaml (:225-243) falls back to the URL path extension whenever Content-Type is absent, empty, or application/octet-stream. A .yaml URL that redirects to an extensionless storage/CDN location — a common pattern for signed or blob URLs — now parses as JSON.

Measured against a local server returning 302 /spec.yaml → /blob/abc123, no Content-Type, YAML body:

dbdbcbb (0.2.4): Ok(Object {"openapi": String("3.1.0")})
5acb74a (0.2.5): Err(Parse { uri: "http://…/blob/abc123", source: Error("expected value", line: 1, column: 1) })

This is not confined to the new API. fetch at :127-128 and :163 now delegate to fetch_document, so every existing caller of the old method gets the new sniffing, and it ships as a patch bump. The new test redirected_yaml_uses_the_final_extension_when_content_type_is_absent covers only the direction that gains (/start/document.yaml); nothing covers the direction that loses.

Fix: when Content-Type gives no answer, accept either URL as the hint — is_yaml(content_type, &retrieval) || is_yaml(content_type, uri) — which keeps the new capability and the old behaviour, and add the losing-direction test. The README sentence would then read "the final URL's extension, or the requested one".

2. The CLI prints every source diagnostic twice — Low

crates/roas-cli/src/arazzo.rs:615-618 writes each diagnostic to stderr, and explain at :509-514 appends the same list to the error. A run whose source cannot be fetched prints:

- file:///…/wf.json: document DocumentId(0) #.sourceDescriptions[0].url (`petStore`): no fetcher registered for `file:///…/missing-openapi.json`
Error: workflow preparation failed; nothing was run
- #.workflows[0].steps[0].operationId: source description `petStore` was not supplied — pass its document in the options

file:///…/wf.json: document DocumentId(0) #.sourceDescriptions[0].url (`petStore`): no fetcher registered for `file:///…/missing-openapi.json`
no source description was supplied — pass `--source <name>=<path>` …

The same ~250-character line, twice. Two smaller things show in that output: SourceDiagnostic's Display (source_registry.rs:206) renders the opaque handle as DocumentId(0), and the CLI has already prefixed the owner's identity, so the reader gets both an identity and a meaningless index. The happy path is clean — I checked that a resolved --source produces no diagnostics.

Fix: pick one channel (keep the stderr loop and let explain add them only when --quiet suppressed it), and have the Display either name the owner by identity or leave the owner to the caller, which already has the registry.

Possible Risks

3. A complete document is now resident three or four times — Medium

  • crates/roas/src/loader.rs:263load_resource_by_key goes through load_document and clones: the legacy path now stores the raw value in documents and the rewritten projection in cache, where it previously stored one. That lands on existing roas users who never call the new API, in a patch release.
  • source_graph.rs:80 and :96loader.load_document(…).cloned() per document, which SourceRegistry::insert then stores again behind an Arc.
  • source_registry.rs:532-533Options::source_registry clones document.value into Source.document while also keeping origin: Some(Arc<SourceDocument>).

Each step is individually reasonable and the .cloned() calls are there for borrow reasons, but the total is three to four resident copies of every source document, for a feature aimed at multi-megabyte OpenAPI specs. Worth deciding deliberately rather than by accumulation: Source.document could read through origin, and load_resource_by_key need not populate documents when only the rewritten projection was asked for.

4. roas became a workspace dependency that three of its four consumers do not use — Low

The root Cargo.toml gains roas = { version = "0.20.1", path = "crates/roas", default-features = false }, but only roas-arazzo-executor takes it with .workspace = true. roas-http-fetcher declares "0.20.1" directly, and roas-cli and roas-file-fetcher still declare "0.20" — three declaration styles and two version floors for one dependency, which is what AGENTS.md's "declare it once … so versions stay in lockstep" is aimed at. The obstacle is real: the workspace entry sets default-features = false for the executor's benefit, which the others need on. Either give the workspace entry the union and let the executor opt out, or leave roas out of [workspace.dependencies] and keep the executor's declaration local.

Nice-to-Have Improvements

5. The non-hierarchical $self guidance was lost — Low

The old sources() said: "{url} is relative, and $self ({base}) is not something a relative reference can be resolved against — give the source an absolute URL, or the description a hierarchical $self". A description with "$self": "urn:example:root" and a relative source URL now reports:

urn:example:root: document DocumentId(0) #.sourceDescriptions[0].url (`petStore`): invalid document URI `./openapi.json`: resolving against `urn:example:root`: relative URL with a cannot-be-a-base base

Accurate, but it no longer tells the author what to do. SourceError::InvalidUri could carry that advice when the base cannot be a base.

6. Partial re-export of the loader surface — Low

crates/roas/src/lib.rs:15 adds pub use loader::{DocumentFetchFuture, LoadedDocument}; while Loader, ResourceFetcher, AsyncResourceFetcher, FetchFuture and LoaderError stay module-only, and the pub use sits between pub mod loader; and pub mod merge;. Callers now reach two of these types by two paths and the rest by one. Re-export the loader surface or none of it.

— Reviewed by Claude Opus 5

Preserve requested YAML hints through redirects, render source diagnostics once, and keep legacy-only loads to one full document tree. Share raw documents through graph-backed options, centralize dependency declarations, restore actionable URI errors, and export the loader API at the crate root.

Assisted-by: Codex
Signed-off-by: Sergey Vilgelm <sergey@vilgelm.com>
Copilot AI review requested due to automatic review settings September 10, 2026 15:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@SVilgelm

Copy link
Copy Markdown
Member Author

Re-reviewed at 2b78beb. All six findings are addressed, each with a test. cargo fmt --all --check, cargo clippy --workspace --all-features --all-targets -- -D warnings, doctests, cargo machete and cargo nextest run --workspace --all-features (3352 tests) are green, and -p roas-arazzo-executor lints clean both with default features and with --no-default-features --features source-graph — worth checking, since SourceData now has a feature-gated variant.

No major issues detected.

1. YAML hint through redirects. parse_body takes both URLs and is_yaml(ct, requested) || is_yaml(ct, retrieval). Measured against a local redirect server:

/spec.yaml → /blob/abc123, no Content-Type, YAML body   => Ok({"openapi":"3.1.0"})
/start     → /doc.yaml,    no Content-Type, YAML body   => Ok({"openapi":"3.1.0"})
/spec.yaml → /blob/abc123, Content-Type: application/json, JSON body => Ok(…)
/spec.yml  → /blob/xyz,    no Content-Type, YAML body   => Ok({"openapi":"3.1.0"})

The first case is the one that regressed at 5acb74a; it matches 0.2.4 again, the new capability is kept, and an explicit non-YAML media type still wins over both URLs.

2. CLI diagnostics. The stderr loop moved after successful preparation, so a preparation failure carries them only inside the error and a successful run prints them once as warnings, suppressed by --quiet. DocumentId(0) is gone — the CLI formats identity: path (\name`): erroritself. Confirmed all three paths by running the binary; droppingexplain` from the execute path is right, since preparation now guarantees the sources it would have suggested.

3. Document sharing. This is the substantial one, and the journal approach is sound but load-bearing, so I exercised it rather than reading it. A document with relative refs that change, an already-absolute ref that does not, a fragment-only ref, an empty ref, a non-string $ref, and refs nested in arrays and objects round-trips exactly:

  • legacy read first, then load_document → raw equals the original
  • load_document first, then load_resource, then load_document again → both raw views equal the original, rewritten values stable
  • preload_resource → same
  • preload_resource over an already-materialized raw view → the stale view is dropped and the replacement is returned
  • load_document_shared twice → Arc::ptr_eq, still pointer-equal and still equal to the original after an intervening legacy read

The ordinal scheme holds because visit_refs increments only on the $ref key branch and recursing into that string value is a no-op, so both walks agree; and there is no &mut accessor to a cached value, so "the cached tree is never reordered" is structurally enforced rather than only asserted. SourceData::Registry(Arc<SourceDocument>) and load_document_shared remove the remaining clones, so the loader, registry and Options now share one tree.

4. Dependency declarations. roas and roas-asyncapi are reached through workspace = true in roas-cli, roas-file-fetcher, roas-http-fetcher and roas-http-validator, each naming its own features. roas's default = ["v3_2"], so default-features = false plus features = ["v3_2"] compiles exactly what the old default did — the conversion is behaviour-preserving, and the two version floors are now one.

5. Non-hierarchical $self. The guidance is back, and better placed than before — it lives on SourceError::InvalidUri in the library rather than in the CLI, so library callers get it too:

urn:example:root: #.sourceDescriptions[0].url (`petStore`): invalid document URI `./openapi.json`: base `urn:example:root` is not hierarchical — give the source an absolute URL, or the Arazzo description a hierarchical `$self`

6. Loader re-exports. Loader, LoaderError, ResourceFetcher, AsyncResourceFetcher, FetchFuture, JsonFileFetcher, LoadedDocument and DocumentFetchFuture are all at the crate root now, so the surface is reachable by one path rather than two.

— Reviewed by Claude Opus 5

@SVilgelm
SVilgelm merged commit c26b57c into main Sep 10, 2026
127 of 128 checks passed
@SVilgelm
SVilgelm deleted the feat/arazzo-source-graph branch September 10, 2026 16:14
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.

2 participants