Skip to content

Replace manifest upload with zip import, add webApplicationInfo flags#2752

Open
heyitsaamir wants to merge 2 commits intomainfrom
fix/manifest-upload-zip-import-and-webappinfo
Open

Replace manifest upload with zip import, add webApplicationInfo flags#2752
heyitsaamir wants to merge 2 commits intomainfrom
fix/manifest-upload-zip-import-and-webappinfo

Conversation

@heyitsaamir
Copy link
Copy Markdown
Collaborator

Summary

  • Delete manifestToAppDetails() — replaced manifest upload with TDP's zip import endpoint (/appdefinitions/v2/import?overwriteIfAppAlreadyExists=true). The backend handles all manifest-to-definition mapping, fixing the bug where webApplicationInfo.resource (and many other fields) were silently dropped.
  • Add --web-app-info-id / --web-app-info-resource flags to teams app update for directly editing webApplicationInfo without a full manifest upload.
  • Auto version bumping on teams app manifest upload — when manifest version matches server but content changed, patch version is auto-bumped (opt out with --no-bump-version).

Test plan

  • npm run build compiles cleanly
  • npm run test — 86 tests pass (24 new: version utilities, zip import, webApplicationInfo validation)
  • Integration: teams app update <id> --web-app-info-id <id> --web-app-info-resource api://botid-<id> → doctor confirms webApplicationInfo configured => pass
  • Integration: teams app manifest upload manifest.json <id>webApplicationInfo.resource persists on server (verified via download)
  • Integration: upload with content change → version auto-bumped 1.0.01.0.1
  • Integration: --no-bump-version → version stays the same
  • Integration: 5 fields changed in one upload → single version bump (1.0.11.0.2)

🤖 Generated with Claude Code

…, auto version bump

The CLI's manifestToAppDetails() was silently dropping fields (most critically
webApplicationInfo.resource, breaking SSO). Instead of patching the mapping,
this eliminates it entirely by using TDP's zip import endpoint — the backend
handles all manifest-to-definition mapping, so no fields are lost.

Also adds --web-app-info-id / --web-app-info-resource flags to `teams app update`
and automatic patch version bumping on manifest upload when content changes.

Co-Authored-By: Claude <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Reworks Teams app manifest upload to use TDP zip import (so manifest fields aren’t dropped), adds teams app update flags for webApplicationInfo, and introduces auto patch-version bumping when uploading changed manifests with unchanged versions.

Changes:

  • Switch manifest upload to download existing package → replace manifest.json → import zip with overwrite.
  • Add --web-app-info-id / --web-app-info-resource options to teams app update.
  • Add version utilities + tests and implement optional auto patch bumping on manifest upload (--no-bump-version to disable).

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
teams.md/docs/cli/commands/app/update.md Documents new app update flags (version, icons, webApplicationInfo, json).
teams.md/docs/cli/commands/app/manifest-upload.md Documents zip import behavior + --no-bump-version and version bump rules.
packages/cli/tests/web-app-info-update.test.ts Adds offline validation tests for new webApplicationInfo flags.
packages/cli/tests/version.test.ts Adds unit tests for version compare/bump helpers.
packages/cli/tests/manifest-upload-zip.test.ts Adds tests verifying zip import + icon preservation behavior.
packages/cli/src/utils/version.ts Introduces bumpPatchVersion and compareVersions.
packages/cli/src/commands/app/update.ts Implements new webApplicationInfo flags + validation + update payload mapping.
packages/cli/src/commands/app/manifest/upload.ts Adds --no-bump-version flag and returns bumped version info in JSON output.
packages/cli/src/commands/app/manifest/actions.ts Implements zip-import upload call path + optional auto version bump logic.
packages/cli/src/apps/tdp.ts Extends importAppPackage to support overwrite query parameter.
packages/cli/src/apps/api.ts Replaces manifest upload implementation with zip download/modify/import flow.

Comment on lines 169 to +171
}

return { version: manifest.version, versionBumped };
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

[HIGH] UploadResult requires version: string, but this function explicitly allows uploading a manifest missing version (user can confirm). That path returns { version: manifest.version } which can be undefined at runtime while still typed as string. Make UploadResult.version optional/nullable (and adjust JSON output accordingly) or require version when proceeding.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed — made UploadResult.version optional. Downstream consumer (upload.ts) already had ManifestUploadOutput.version as optional.

Comment thread packages/cli/src/commands/app/update.ts Outdated
Comment on lines +291 to +296
if (options.webAppInfoResource !== undefined && options.webAppInfoResource.length > 100) {
throw new CliError(
'VALIDATION_FORMAT',
'webApplicationInfo resource URI must be 100 characters or less.'
);
}
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

[HIGH] --web-app-info-resource validation only checks length. Elsewhere the CLI expects api://botid-${botId} format (see app doctor). If this flag is meant to configure SSO correctly, add format validation (at least api://botid- prefix; ideally validate against the app’s botId when available) to prevent creating configs that doctor will immediately warn/fail on.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed — added api:// prefix validation with a helpful error message. Also added a test case for it.

Comment thread packages/cli/src/utils/version.ts Outdated
@@ -0,0 +1,30 @@
/**
* Bump the patch (last) segment of a dotted version string.
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

[NIT] Docstring says “Bump the patch segment”, but the function bumps the last segment (so 1.01.1, i.e., minor). Consider rewording to avoid implying strict SemVer patch behavior.

Suggested change
* Bump the patch (last) segment of a dotted version string.
* Increment the last numeric segment of a dotted version string.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed — updated to "Increment the last numeric segment of a dotted version string."

Comment on lines +18 to +22
export function compareVersions(a: string, b: string): number | null {
const pa = a.split('.').map(Number);
const pb = b.split('.').map(Number);
if (pa.some(isNaN) || pb.some(isNaN)) return null;
const maxLen = Math.max(pa.length, pb.length);
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

[HIGH] compareVersions uses split('.').map(Number), which treats empty segments as 0 (e.g., "1." or "1..0" parses instead of returning null). Add a stricter numeric-only validation (regex per segment) before converting so malformed versions don’t compare as valid.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed — added strict per-segment regex validation (/^\d+$/) so empty segments like "1..0" or "1." now return null. Added tests for both cases.

Comment on lines +19 to +22
function runJson<T = Record<string, unknown>>(command: string): { data: T; exitCode: number } {
const { stdout, exitCode } = run(command);
return { data: JSON.parse(stdout) as T, exitCode };
}
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

[HIGH] runJson<T = Record<string, unknown>> + downstream as Record<string, ...> casts make these tests loosely typed and easy to break when the JSON error shape changes. Prefer a small explicit interface for the JSON error response (e.g., { ok: false; error: { code: string; message?: string } }) and type runJson to return that.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed — added a JsonErrorResponse interface and typed runJson to return it instead of Record<string, unknown>. Removed all inline casts.

Comment thread packages/cli/src/apps/api.ts Outdated
Comment on lines +262 to +268
try {
// Download existing package to preserve icons
zipBuffer = await downloadAppPackage(token, teamsAppId);
} catch {
// No existing package — create fresh zip with default icons
await importAppPackage(token, createDefaultZip(manifestJson), true);
return;
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

[CRITICAL] uploadManifest falls back to creating a default zip on any download failure. A transient/500 error would silently overwrite existing icons with defaults. Only treat “no existing package” as fallback (e.g., 404) and rethrow other errors (consider throwing a typed CliError from downloadAppPackage that preserves status).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed — downloadAppPackage now throws a typed CliError with statusCode, and uploadManifest only falls back to default zip on 404. All other errors are rethrown.

Comment thread packages/cli/src/commands/app/manifest/actions.ts Outdated
…safety

- Only fall back to default zip on 404 in uploadManifest (not any error)
- Use stable sorted-key stringify for manifest comparison
- Make UploadResult.version optional to match runtime behavior
- Add api:// prefix validation for --web-app-info-resource
- Fix compareVersions to reject malformed versions (empty segments)
- Add JsonErrorResponse interface and remove Record casts in tests
- Update bumpPatchVersion docstring for accuracy

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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