Skip to content

fix firereset subscription MTU issue - #2011

Open
drahnieR wants to merge 1 commit into
firewalla:masterfrom
drahnieR:firereset
Open

fix firereset subscription MTU issue#2011
drahnieR wants to merge 1 commit into
firewalla:masterfrom
drahnieR:firereset

Conversation

@drahnieR

@drahnieR drahnieR commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@drahnieR
drahnieR requested a review from jasonlyc August 4, 2026 07:51
@j-sallyjin

Copy link
Copy Markdown
Contributor

PR Review Summary

✅ What looks good

  • The replacement remains a valid x86-64 firereset executable.
  • Build metadata references firego commit cf608627b79b, which implements MTU-aware BLE notification sizing and upgrades github.com/melvinto/ble from v0.2.5 to v0.2.6.
  • The linked issue, firecommit#6243, requires long error responses to remain valid JSON over iOS BLE. The referenced implementation directly addresses that truncation failure.

⚠️ Issues found

  • High — Unverifiable binary provenance: platform/gold/bin/firereset reports vcs.modified=true. It was built from a dirty firego working tree, so the checked-in executable cannot be proven to contain only commit cf608627b79b. Undocumented local changes may be present.

    Rebuild from a clean checkout and verify the embedded metadata reports vcs.modified=false before merging.

  • Medium — Issue requirement only partially demonstrated: The linked issue’s test procedure requires validating a long config/set error over BLE using the iOS app, including Gold-series hardware and checking PSE applicability. The PR contains no test results or artifact checksum establishing that this binary passed that scenario.

💡 Suggestions

  • Add the clean source revision, build command, SHA-256 checksum, and BLE/iOS test results to the PR description.
  • For future binary-only updates, attach reproducible build provenance so reviewers can correlate the artifact with the reviewed source.

Verdict

REQUEST_CHANGES


Repo: firewalla/firerouter
PR: #2011
Head SHA: dfb059eb2032a0ace9c86ffb2e73409f30026fcc
Checked at: 2026-08-04 16:10:45 CST

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