Skip to content

Conversation

@jason-ha
Copy link
Contributor

@jason-ha jason-ha commented Jan 7, 2026

  • Process all of packages local source files for assertions.
  • Apply results to restore missing short codes.

Copy link
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

This PR improves the assertion tagging process by processing all package local source files instead of only a subset. This allows the tool to identify and restore missing assertion short codes across the codebase.

  • Updated build-tools to process all package source files by disabling dependency skipping and filtering to package-local sources
  • Converted string literal assertion messages to hex short codes in presence framework files
  • Added 11 new assertion short code mappings to the central map

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
build-tools/packages/build-cli/src/commands/generate/assertTags.ts Modified to process all package-local source files by setting skipFileDependencyResolution: false and filtering to package directory sources excluding .d.ts files
packages/runtime/test-runtime-utils/src/assertionShortCodesMap.ts Added 11 new assertion short code mappings (0xa3a, 0xa3c, 0xa59, 0xa5a, 0xaad, 0xcb0-0xcb6) for presence and connection-related assertions
packages/framework/presence/src/systemWorkspace.ts Converted assertion message from string literal to hex code 0xcb6
packages/framework/presence/src/presenceManager.ts Converted assertion message from string literal to hex code 0xcb5
packages/framework/presence/src/presenceDatastoreManager.ts Converted 4 assertion messages from string literals to hex codes (0xcb0, 0xcb1, 0xcb2, 0xcb3, 0xcb4)

- Process all of packages local source files for assertions.
- Apply results to restore missing short codes.
@jason-ha jason-ha force-pushed the tools/fix-tagging-for-presence branch from 3c68dce to 8e533e5 Compare January 7, 2026 01:06
assert(
alternateProvider !== undefined,
"Self is not in audience and no alternateProvider given",
0xcba /* Self is not in audience and no alternateProvider given */,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't want to include the actual tagging in this PR since that leaves us in a state where if someone runs assert tagging on main, it would lose the tags for these (might even reuse the tags for something else if run again later).

This should probably wait until at least the change which integrates the updated build tools change which contains this new tagging logic into client.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know we already basically have this bug, but for other files, but I'd like to avoid the risk of introducing a new variant of it transiently while we are getting this fix plumbed over to client

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I come down on the other side. I only expect tagging to be run for tagging process work. I'd rather get rid of the noise of changes without these corrections and also make sure that at least the "unused" tags are restored. I've reviewed these results and know they are good - we clearly don't scrutinize the results when tagging is run for release (otherwise we'd have noticed this a long time ago).
We already know the result of running without the build-tools fix and I think that is okay if we fall back in the meantime.

Copy link
Contributor

Choose a reason for hiding this comment

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

When tagging for release I go often review the tags which were removed from the list, if it looks suspicious and have caught bugs where tags were incorrectly removed. Apparently though we missed this one.

The problem here is that the version of build tools we use to do tagging for releases is the version depended on by client, which will not have your fix until after the next release and integration into client after your change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood

Copy link
Contributor

Choose a reason for hiding this comment

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

If you are confident no one will try and do a release between when you merge this and when integrate these changes into client, I'm fine with this as it. You just need to commit to either doing a build tools release and integration yourself, or getting someone else to do it, and getting it done early next week and/or coordinating with the release engineer.

@jason-ha jason-ha requested a review from CraigMacomber January 8, 2026 20:05
Copy link
Contributor

@CraigMacomber CraigMacomber left a comment

Choose a reason for hiding this comment

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

See pending comment: if you are ok ensuring this is integrated into client on time, feel free to merge.

@jason-ha
Copy link
Contributor Author

See pending comment: if you are ok ensuring this is integrated into client on time, feel free to merge.

That is my plan. Thanks!

@jason-ha jason-ha merged commit ae94ddc into microsoft:main Jan 10, 2026
31 checks passed
@jason-ha jason-ha deleted the tools/fix-tagging-for-presence branch January 10, 2026 00:15
anthony-murphy-agent pushed a commit to anthony-murphy-agent/FluidFramework that referenced this pull request Jan 14, 2026
- Process all of packages local source files for assertions.
- Apply results to restore missing short codes.
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