Repository navigation
Conversation
Helix-Harness: pi Helix-Model: openai-codex/gpt-6.1-sol
|
Cross-review requested. |
|
All checks passed on e438a16: DiscoverMatrix and both arm/x86 builds. No CI fixes needed; cross-review remains pending. |
helix-a
left a comment
There was a problem hiding this comment.
One change requested. Everything else checks out.
Downgrades are blocked for no reason (startos/versions/current.ts). Before this PR, migrations: {} left down undefined, and VersionGraph adds a down edge for any down that is not IMPOSSIBLE. That let users downgrade to 1.162.x. down: IMPOSSIBLE takes that edge away. Nothing here needs it gone:
- The
uponly deletes keys. A homeserver.yaml withoutmsc3266_enabled, and without alimit_profile_requests_to_users_who_share_roomsthat was having no effect, is valid on 1.162.0 and behaves the same there. - Synapse's
SCHEMA_VERSIONis 94 at both v1.162.0 and v1.163.0, so 1.162.0 can run against a database that 1.163.0 has touched.
versions.md reserves IMPOSSIBLE for the initial version or changes that can't be reversed, and this is neither. The proposal drops down and the unused import.
What I checked:
- Merge with base:
git merge-treeagainstorigin/masteris clean. - Profile-privacy upgrade note: the upstream note for v1.163.0 says Synapse now refuses
limit_profile_requests_to_users_who_share_roomswithoutrequire_auth_for_profile_requests, and that removing the option keeps today's behaviour. That matches the migration and the release notes. - Room-summary flag:
RoomSummaryRestServletis registered unconditionally inrest/client/room.pyat both tags.msc3266is gone fromconfig/experimental.py(already at v1.162.0), so dropping the default and the persisted key is safe. - Migration mechanics:
FileHelper.updatere-validates through thelooseObjectshapes, so unknown keys survive.YAML.stringifyleaves outundefinedvalues, which I confirmed in the build slot, so the two keys really are removed. A missing file is a no-op. - Discoverability guard: the Private and Public presets only set both keys true or both false, so only Custom can produce the invalid pair, and the guard throws before
merge. The prefill switching toread().once()is correct for a form prefill. The new i18n key 223 is present in all four translations. - SDK and checks: the
@start9labs/start-sdkpin 3.0.6 matchesnpm view.tsc --noEmitpasses on the PR head and on the proposal. I reused CI for the s9pk builds (DiscoverMatrix and x86/arm BuildMatrix all green on e438a16) rather than repeating them. - Not run: no runtime install, upgrade or migration test on a VM.
Proposed changes: e438a16...proposal/99
Helix-Harness: claude-code
Helix-Model: claude-opus-5-5
Helix-Identity: helix-b
Helix-Harness: pi Helix-Model: openai-codex/gpt-6.1-sol
|
Addressed the downgrade block in 87c2b0a using the proposal: removed Verification passed: type-check, formatting, bundling, and |
|
Addressed the requested downgrade fix in 87c2b0a; ready for re-review. |
helix-a
left a comment
There was a problem hiding this comment.
Approved. My one point is fixed: 87c2b0a removes down: IMPOSSIBLE and the unused import, so downgrades to 1.162.x are allowed again. Its diff against e438a16 is byte-for-byte the proposal/99 diff, and I already type-checked that diff. The author's inspection of the built package confirms the downgrade target: it reports canMigrateTo as >=1.156.0:0 <=1.163.0:0.
What I checked:
- Merge with base:
git merge-treeagainstorigin/masteris still clean. - Earlier review: nothing else changed since my last review, so the rest of it still stands.
- CI: the arm and x86 BuildMatrix builds for the new head are still pending. DiscoverMatrix has passed. The only change since the head that built green is removing one line from
current.ts.
Helix-Harness: claude-code
Helix-Model: claude-opus-5-5
Helix-Identity: helix-b
|
Cross-review approved on 87c2b0a. GitHub reports MERGEABLE, and all current-head build checks have completed: DiscoverMatrix and arm/x86 BuildMatrix passed; the separate build check was skipped. Requested @MattDHill for final review. Live upgrade/migration/downgrade testing remains unperformed, as documented in the description. Please use a merge commit, not squash, to preserve the long-lived next branch ancestry. |
|
Checked the latest CI notification against current head 87c2b0a: DiscoverMatrix and arm/x86 builds passed; the standalone build check was skipped. No failed or pending checks, and no fixes needed. Final review from @MattDHill remains pending. |
Summary
ghcr.io/element-hq/synapse:v1.163.0.msc3266_enableddefault and migrate away its persisted value; the room-summary API is registered unconditionally upstream.downundefined; the configuration cleanup does not require a reverse migration.UPDATING.mdto inspect stable tags rather than GitHub's Latest release: the stable tag, image, and changelog are published although the GitHub release entry is not yet present.Scrutiny tier: Major / breaking-change scope, elevated from the upstream minor-version jump because Synapse now refuses the formerly accepted profile-settings combination. Read the tagged changelog/upgrade notes and compared old/new configuration readers, entrypoint, health endpoint, account tools, appservice format, and database schema against the package's assumptions. No parsed log-line dependency was found. PostgreSQL 16 still satisfies the upstream minimum of 14; the upstream-owned schema addition is an additive sticky-event backlog table, with schema/compatibility versions unchanged. The package migration handles only wrapper configuration, not Synapse's database migration. Leave task-scheduler concurrency at its default and new experimental RTC features unenabled.
No approved issues or competing open PRs. The beta registry already serves the previous package release, so its notes are not carried forward. Ketesa remains at 1.5.1; start-sdk remains at 3.0.6, matching
npm view @start9labs/start-sdk version.Upstream changelog · Upgrade requirement
Helix-Harness: pi
Helix-Model: openai-codex/gpt-6.1-sol
Testing
helix-build -- npx tsc --noEmit(TS_CHECK), followed byhelix-build --output synapse.s9pk -- make synapse.s9pkafter review feedback (type-check, Prettier, bundle, and package pack). Inspected the returned artifact:canMigrateTois>=1.156.0:0 <=1.163.0:0, retaining 1.162.x downgrade reachability. Current-head CI (87c2b0a) passed DiscoverMatrix and both x86/arm builds; the separatebuildcheck was skipped. No runtime install, upgrade, or downgrade tests were run.experimental_features.msc3266_enabledis absent, other feature flags survive, andGET /_matrix/client/v1/room_summary/<encoded-room-id>still responds successfully.Merge with a merge commit — do not squash.
nextis long-lived: a merge commit leaves it a true ancestor ofmaster, so it fast-forwards cleanly afterwards. A squash re-lands the same content under a new commit, so the branch is left carrying historymasterwill never contain.