Skip to content

refactor: move manifest template resolver into @microsoft/app-manifest and route fx-core through it - #16525

Open
svalenciasan wants to merge 6 commits into
OfficeDev:devfrom
svalenciasan:odsp/manifest-api-extraction
Open

refactor: move manifest template resolver into @microsoft/app-manifest and route fx-core through it#16525
svalenciasan wants to merge 6 commits into
OfficeDev:devfrom
svalenciasan:odsp/manifest-api-extraction

Conversation

@svalenciasan

@svalenciasan svalenciasan commented Aug 5, 2026

Copy link
Copy Markdown

Summary

Builds on #16270 (Ian Clanton-Thuon (@iclanton)). Moves the DriverContext-free parts of fx-core's manifest
templating — $[file('...')] function expansion and ${{ENV}} variable expansion — down into
@microsoft/app-manifest, so the logic can be consumed without @microsoft/teamsfx-core, a
DriverContext, localization, or UserError.

This PR goes one step further than #16270: it also routes fx-core's getResolvedManifest
through the shared resolver
, removing the duplicated orchestration that #16270 intentionally
left in place. This is a behavior-preserving refactor — fx-core keeps only its host concerns
(telemetry + localized error mapping) and delegates the resolution itself to the moved logic.

What moves into @microsoft/app-manifest (from #16270)

  • expandEnvironmentVariable / getEnvironmentVariables — moved from
    fx-core/src/component/utils/common.ts (re-exported there via @microsoft/teamsfx-api so
    existing import paths are unchanged).
  • The file() function resolver and file reader — moved from
    fx-core/src/component/utils/envFunctionUtils.ts, decoupled from DriverContext and raising
    plain typed ManifestTemplateError subclasses that carry the offending path/token.
  • The ManifestType enum.
  • New public API: resolveManifest, expandFileFunctionMacros, processManifestFunction, and
    the ResolveManifestResult type.

What this PR adds on top of #16270

  • fx-core now delegates end-to-end. getResolvedManifest
    (teamsApp/utils/utils.ts) drops its inline expand → env → check body and calls the new
    resolveManifestWithContext, so the whole resolve pipeline lives in one place
    (@microsoft/app-manifest). fx-core keeps only the telemetry-property selection it did before.
  • resolveManifest returns { content, functionCount } (ResolveManifestResult) instead of
    just a string, so the host can still emit the existing manifest-with-function telemetry event
    after delegating.
  • Complete host-side error mapping. toFxError maps every ManifestTemplateError subclass to
    its localized fx-core UserError / FileNotFoundError, maps MissingEnvironmentVariablesError
    (which now carries fromPath) to fx-core's localized
    MissingEnvironmentVariablesError("manifest", names, fromPath), and wraps anything unexpected via
    assembleError so the Result<string, FxError> contract always holds.
  • Shared runManifestResolver helper consolidates the try/catch → toFxError mapping plus the
    function-count telemetry used by both expandVariableWithFunction and resolveManifestWithContext.

Behavior preservation

  • fx-core call sites (ManifestUtils, PluginManifestUtils, CopilotGptManifestUtils,
    createAppPackage) are unchanged and still receive Result<string, FxError>.
  • Telemetry events (manifest-with-function, customized-keys properties) and localized error
    messages/keys are preserved.

Testing

  • @microsoft/app-manifest unit suite (manifestTemplate.test.ts) covers expandEnvironmentVariable,
    getEnvironmentVariables, the file() resolver, expandFileFunctionMacros, and resolveManifest.
  • fx-core affected suites updated: manifestUtils.test.ts and envFunctionUtils.test.ts now drive
    $[file()] resolution through real temp files, since the read now happens inside
    @microsoft/app-manifest and cross-package fs mocks no longer apply.

Notes / follow-ups

  • Schema-validation packaging (same note as refactor: move manifest template resolver into @microsoft/app-manifest #16270): @microsoft/app-manifest's published tarball
    omits build/json-schemas/, so offline schema validation silently network-falls-back; bundling the
    schemas would make it fully hermetic.
  • Downstream consumer: the SPFx copilotAgentPlugin build (ODSP-Web) currently ships an interim
    in-process copy of this file() resolver; once this lands and publishes it will swap to
    expandFileFunctionMacros / resolveManifest.

Opened as a draft (mirrors #16270's draft status) to align with the upstream API-surface review.

Related PRs

Design / architecture

This PR moves structural shape across a package boundary (host-agnostic manifest-template resolution fx-core@microsoft/app-manifest), so it is recorded as an ADR:

  • ADR-0022 — Ownership of host-agnostic manifest-template resolution (added in this PR under docs/02-architecture/adr/). Decision: the ${{ENV}} / $[file()] grammar lives once in @microsoft/app-manifest; hosts (fx-core today, the SPFx build next) wrap it and add only their cross-cutting concerns (telemetry, localized FxError). Follows the existing fx-core → @microsoft/teamsfx-api → @microsoft/app-manifest dependency direction — it relocates logic to the correct existing layer rather than introducing a new boundary.
  • Related fact page: manifest-schemas.md — the platform manifest schemas the resolved output conforms to (context, not the decision).
  • Architecture scope (package boundaries / layering / dependency direction): docs/02-architecture/README.md.

…pp-manifest

Relocate the DriverContext-free parts of fx-core's manifest templating into
@microsoft/app-manifest so the logic can be consumed without fx-core's
DriverContext, localization, or FxError:

- expandEnvironmentVariable / getEnvironmentVariables (moved from fx-core
  component/utils/common.ts)
- the file() function resolver and file reader (moved from fx-core
  component/utils/envFunctionUtils.ts), decoupled from DriverContext and
  raising plain typed errors that carry the offending path/token
- the ManifestType enum

Also expose expandFileFunctionMacros (the resolution loop, returning the
expanded content plus a function count for host telemetry) and resolveManifest,
the host-agnostic counterpart of fx-core's getResolvedManifest. Reuses the
package's existing strip-bom and fs-extra usage (no new dependencies).

Adds unit tests covering file inlining, JSON escaping, BOM/CRLF normalization,
nested file() calls, env-as-parameter, ApiSpec skip, and every typed error path.
Now that the resolver lives in @microsoft/app-manifest (reached transitively via
@microsoft/teamsfx-api), remove the duplicated logic from fx-core and delegate:

- component/utils/common.ts re-exports expandEnvironmentVariable and
  getEnvironmentVariables from @microsoft/teamsfx-api, so existing call sites are
  unchanged.
- envFunctionUtils.expandVariableWithFunction delegates its loop to
  expandFileFunctionMacros, keeping only the telemetry event and mapping the
  resolver's plain errors to localized UserError / fx-core FileNotFoundError via
  toFxError. getResolvedManifest is unchanged and keeps composing these primitives.

Update the affected tests to drive resolution through real temp files instead of
fs mocks, since file reads now happen inside app-manifest.
toFxError now returns assembleError(e, source) instead of throwing on the fallback branch, so expandVariableWithFunction always honors its Result<T, FxError> contract. Documents why MissingEnvironmentVariablesError is intentionally unmapped, adds a localization-remap note to ManifestTemplateError, and restores explanatory comments in manifestTemplate.
…fest

getResolvedManifest now delegates its file()+env resolution ordering to @microsoft/app-manifest's resolveManifest (single source of truth), via a new resolveManifestWithContext wrapper that re-adds fx-core's function-count telemetry and localized FxError mapping. resolveManifest returns ResolveManifestResult { content, functionCount } and its MissingEnvironmentVariablesError carries fromPath so the error mapping is uniform in toFxError. A shared runManifestResolver helper removes the duplicated await/try/telemetry shape between expandVariableWithFunction and resolveManifestWithContext. Behavior preserved.
@svalenciasan svalenciasan changed the title Odsp/manifest api extraction refactor: move manifest template resolver into @microsoft/app-manifest and route fx-core through it Aug 6, 2026
…xtraction

# Conflicts:
#	packages/fx-core/src/component/utils/envFunctionUtils.ts
@svalenciasan

Copy link
Copy Markdown
Author

Possible pre-existing bug: an empty file() target leaves the macro unresolved

While wiring the downstream SPFx consumer to this resolver, I noticed a behavior in expandFileFunctionMacros (packages/manifest/src/manifestTemplate.ts) worth a second look:

if (isJson && value) {
  value = JSON.stringify(value).slice(1, -1);
}
if (value) {
  functionCount += 1;
  content = content.replace(placeholder, value);
}

Both steps are guarded by if (value). If a referenced .txt/.md file resolves to an empty string (empty file, or a file whose content becomes empty after env expansion), value === "" is falsy, so neither the escape nor the replacement runs — the $[file('…')] placeholder is left literally in the output.

For a JSON string field (e.g. a declarative agent's instructions), that ships the unresolved macro verbatim. Because the result is still a syntactically valid string, downstream JSON-schema validation won't flag it, so it can surface as a broken agent at runtime rather than a build error.

To be clear: this is pre-existing behavior carried over verbatim in this behavior-preserving refactor — not introduced here — so I'm not suggesting we change it in this PR. Flagging it for a possible follow-up.

Question for the owners: is "leave the placeholder literal when the resolved file is empty" intentional (e.g. a signal that nothing was inlined), or should an empty resolved file replace the macro with an empty string like every other value? The content.replace(placeholder, value) path with an empty value would simply remove the macro, which seems safer than shipping an unresolved $[file()]. If the current behavior is intentional, a short comment + test documenting it would help future consumers.

Downstream note: the SPFx copilotAgentPlugin that will consume this API plans to add a guard rejecting any residual $[file(...)] after expansion, so it's defended on that side regardless — raising here so the shared utility's behavior is a deliberate choice rather than an accident.

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