Skip to content

fix(framework) Allow commas in FAB file names - #7973

Open
danielpolimac wants to merge 1 commit into
flwrlabs:mainfrom
danielpolimac:fix/fab-comma-filenames
Open

fix(framework) Allow commas in FAB file names#7973
danielpolimac wants to merge 1 commit into
flwrlabs:mainfrom
danielpolimac:fix/fab-comma-filenames

Conversation

@danielpolimac

@danielpolimac danielpolimac commented Aug 22, 2026

Copy link
Copy Markdown

Issue

Description

FAB installation fails when an app contains a file whose name includes a comma. The CONTENT manifest parser splits every comma, so the file path is parsed as multiple fields and raises ValueError.

Related issues/PRs

Fixes #7944.

Proposal

Explanation

Parse each CONTENT manifest entry from the right with two splits. The hash and size fields remain fixed, while any commas in the preceding relative path are preserved.

Add a regression test that verifies the hash of a file named metrics,round=1.json.

Checklist

  • Implement proposed change
  • Write tests
  • Update documentation (not needed; this restores supported file handling)
  • Address LLM-reviewer comments, if applicable (review completed with no comments)
  • Make local checks pass
  • Make CI checks pass
  • Ping maintainers on Slack (channel #contributions)

Any other comments?

Local verification:

  • python -m pytest py/flwr/cli/install_test.py
  • python -m ruff check py/flwr/cli/install.py py/flwr/cli/install_test.py --no-respect-gitignore
  • uv run --no-sync --python=3.11.14 ./dev/test.sh false

Copilot AI lite review requested due to automatic review settings August 22, 2026 14:07

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.

Pull request overview

This PR fixes FAB installation failures when a file path in the .info/CONTENT manifest contains commas by changing the manifest parsing logic to split from the right, preserving commas in the path portion while still extracting the fixed hash and size fields.

Changes:

  • Update _verify_hashes to parse CONTENT manifest lines with rsplit(",", maxsplit=2) so commas in file paths no longer break parsing.
  • Add a regression test covering a file named metrics,round=1.json to ensure hash verification succeeds.

Critical issues

  • None found.

Simplicity/readability suggestions

  • None required.

Consistency concerns

  • None found (the test’s size_bits field matches the existing CONTENT manifest format in build.py).

Should this PR be split?

  • No.

Overall verdict

  • The change is minimal, targeted to the reported failure mode, and includes a regression test.

Reviewed changes

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

File Description
framework/py/flwr/cli/install.py Parse CONTENT manifest entries from the right to preserve commas in relative paths.
framework/py/flwr/cli/install_test.py Add regression test ensuring _verify_hashes accepts commas in filenames.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions github-actions Bot added the Contributor Used to determine what PRs (mainly) come from external contributors. label Aug 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Contributor Used to determine what PRs (mainly) come from external contributors.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Comma in a filename in app directory causes crash

2 participants