diff --git a/.github/ci/ci-tests/test_sync_upstream.py b/.github/ci/ci-tests/test_sync_upstream.py index e79c841b3f..d9f63b99fa 100644 --- a/.github/ci/ci-tests/test_sync_upstream.py +++ b/.github/ci/ci-tests/test_sync_upstream.py @@ -288,16 +288,19 @@ def test_existing_release_with_bind_change_requires_review(repositories): ) -def test_new_series_with_matching_bind_tree_bootstraps_a_build(repositories): +def test_new_series_with_matching_bind_tree_requires_bootstrap_review(repositories): decision = plan(repositories) - assert decision['action'] == 'bootstrap-build' + assert decision['action'] == 'bootstrap-review' assert decision['series'] == '26.7' assert decision['source_release'] == 'release/bind-rp/26.1' assert decision['target_release'] == 'release/bind-rp/26.7' assert decision['tools_tag'] == '26.7.1' assert decision['freebsd_release'] == '15.1' assert decision['bind_changed'] is False + assert decision['sync_branch'] == ( + f'sync/bootstrap/26.7/{repositories["stable_26_7"][:12]}' + ) def test_new_series_with_bind_change_requires_bootstrap_review(repositories): @@ -434,7 +437,7 @@ def test_missing_tools_os_assignment_blocks_planning(repositories): assert decision['reason'] == 'missing or invalid tools release profile' -def test_apply_bootstrap_build_creates_release_with_only_manifest_overlay_and_metadata( +def test_apply_bootstrap_review_creates_pristine_release_and_overlay_branch_for_unchanged_bind( repositories, tmp_path ): decision = plan(repositories) @@ -443,7 +446,13 @@ def test_apply_bootstrap_build_creates_release_with_only_manifest_overlay_and_me assert result.returncode == 0, result.stderr target = decision['target_release'] - assert git(repositories['repository'], 'show', f'{target}:tools/resolver-overlay.txt') == 'resolver overlay' + with pytest.raises(subprocess.CalledProcessError): + git(repositories['repository'], 'show', f'{target}:tools/resolver-overlay.txt') + assert git( + repositories['repository'], + 'show', + f'{decision["sync_branch"]}:tools/resolver-overlay.txt', + ) == 'resolver overlay' with pytest.raises(subprocess.CalledProcessError): git(repositories['repository'], 'show', f'{target}:tools/not-an-overlay.txt') target_metadata = json.loads(git(repositories['repository'], 'show', f'{target}:{METADATA_PATH}')) @@ -459,7 +468,7 @@ def test_apply_bootstrap_build_creates_release_with_only_manifest_overlay_and_me } -def test_apply_bootstrap_build_accepts_source_metadata_from_divergent_source_stable_branch( +def test_apply_bootstrap_review_accepts_source_metadata_from_divergent_source_stable_branch( repositories, tmp_path ): repository = repositories['repository'] @@ -499,7 +508,9 @@ def test_apply_creates_the_same_commit_when_a_publish_retry_rebuilds_a_branch( ) assert first.returncode == 0, first.stderr first_commit = git(repositories['repository'], 'rev-parse', decision['target_release']) + first_sync_commit = git(repositories['repository'], 'rev-parse', decision['sync_branch']) git(repositories['repository'], 'branch', '-D', decision['target_release']) + git(repositories['repository'], 'branch', '-D', decision['sync_branch']) second = apply( repositories, @@ -513,6 +524,7 @@ def test_apply_creates_the_same_commit_when_a_publish_retry_rebuilds_a_branch( assert second.returncode == 0, second.stderr assert git(repositories['repository'], 'rev-parse', decision['target_release']) == first_commit + assert git(repositories['repository'], 'rev-parse', decision['sync_branch']) == first_sync_commit def test_apply_bootstrap_review_creates_pristine_release_and_overlay_sync_branch( @@ -571,7 +583,7 @@ def test_apply_three_way_merges_same_result_and_retains_target_and_overlay_chang result = apply(repositories, decision, tmp_path) assert result.returncode == 0, result.stderr - assert git(repositories['repository'], 'show', f'{decision["target_release"]}:{path}') == ( + assert git(repositories['repository'], 'show', f'{decision["sync_branch"]}:{path}') == ( 'header\nshared=new\nline-03\nline-04\nline-05\nline-06\nline-07\n' 'line-08\nline-09\ncontext=target\nline-11\nline-12\nline-13\nline-14\n' 'line-15\nline-16\nline-17\nline-18\nline-19\noverlay=new\ntail' diff --git a/.github/ci/sync_upstream.py b/.github/ci/sync_upstream.py index 013cf49dd1..1f8e81bfa3 100644 --- a/.github/ci/sync_upstream.py +++ b/.github/ci/sync_upstream.py @@ -251,16 +251,14 @@ def plan(arguments: argparse.Namespace) -> dict: 'blocked', target_series, upstream_commit, source_release, target_release, None, None, False, 'upstream BIND tree is unavailable', ) - if bind_changed: - return decision( - 'bootstrap-review', target_series, upstream_commit, source_release, - target_release, tools_tag, freebsd_release, True, - 'new series has an upstream BIND change', - ) + reason = ( + 'new series has an upstream BIND change' + if bind_changed + else 'new series has an unchanged BIND tree' + ) return decision( - 'bootstrap-build', target_series, upstream_commit, source_release, - target_release, tools_tag, freebsd_release, False, - 'new series has an unchanged BIND tree', + 'bootstrap-review', target_series, upstream_commit, source_release, + target_release, tools_tag, freebsd_release, bind_changed, reason, ) diff --git a/.gitignore b/.gitignore index 279e0fd7eb..363012f332 100644 --- a/.gitignore +++ b/.gitignore @@ -6,3 +6,4 @@ venv /*/*/work /.github/ci-local/ /docs/superpowers/ +/.worktrees/ diff --git a/AGENTS.md b/AGENTS.md index 34bc3e87c0..b992aaae9f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -39,6 +39,7 @@ self-contained current and rollback channels are generated in ## Non-negotiable rules +- DO NOT open pr's on the official Opnsense Repos - `os-bind-rp` is a replacement for official `os-bind`, not a companion package. Keep `PLUGIN_NAME=bind-rp` and `PLUGIN_CONFLICTS=bind` intact unless the maintainer explicitly changes the package policy. @@ -72,23 +73,50 @@ self-contained current and rollback channels are generated in - Verify CI changes with the focused local checks in [docs/building.md](docs/building.md), and run the affected workflow manually only when authorized. Report the workflow URL and its actual outcome. -- A code review agent must be run on pr's when they are "ready", as a final step - befor merge. High priority items must be resolved before a pr can be merged. - Perform a remeditation and review cycle util the items are resolved and the - code-review agent approve the pr as ready, when no high priority items are - observed. - -- An agent must be used to review generated plans, and high priority items must be - resolved before a plan can be approved and implemented. -- When creating tests, categorize each test according to type relative to purpose. - Tests used for discovery and troubleshooting vs tests used for enforcing outcomes - and preventing regressions. In other words, tests that should live temporarily - vs long term in the repo. Long term tests should be commited to the repo in the - appropriate folder. Short term tests should be kept local only, and not commited - to the repo. If an issue has been resolved and the problem is unlikely to occur - again in the future, it should be categorized as a temporaty test. -- Code reviews should also include a code-simplication pass as well as a - documentation update and accuracy pass. +- If a written implementation plan is created, have an independent agent + review it before implementation. The plan must state the required behavior, + the minimal sufficient architecture, and the current requirement served by + every proposed abstraction, boundary, configuration option, retry, cache, + state store, dependency, and durable test. Remove proposals with no + demonstrated current requirement. +- Treat correctness, security, data-loss, compatibility, provenance, and + public-contract findings as blocking regardless of the review tool's + severity vocabulary. Resolve blocking findings and repeat review before + implementation or merge. +- Before declaring a PR ready, run an independent code review for changes to + executable code, tests, workflows, build or release behavior, provenance, + security boundaries, persisted state, or public contracts. Purely editorial + documentation changes may skip independent review only when they alter no + command, procedure, policy, generated content, link contract, or + machine-consumed content. Human PR review does not replace required agent + review. +- Every code review must apply both the `code-simplifier` and + `test-suite-simplifier` as read-only passes. Review implementation and + architecture for unexplained accretion; review changed tests for distinct + regression value, duplication, implementation coupling, inappropriate test + level, and exploratory residue. Also perform a documentation-impact and + accuracy check; update documentation only when the change affects its + contract or accuracy. +- Discovery and troubleshooting tests may remain temporary while investigating + uncertainty. Keep them outside the final diff, using a temporary directory + or `.github/ci-local/` for CI investigations. Promote a test into the + repository only when it protects a distinct observable behavior, invariant, + known regression, security or compatibility risk, integration boundary, or + machine-enforced contract. +- Do not retain a test merely because it was useful during development, raises + coverage, or exercises another example of an already-protected equivalence + class. Do not add tests that merely mirror ordinary Markdown wording. + Preserve uncertain tests until their value can be investigated. +- Follow the repository's existing language and runtime constraints. Use POSIX + `sh` for existing package or service hooks and thin orchestration that must + run in the base OPNsense/FreeBSD environment. Prefer Python for non-trivial + parsing, branching, state handling, or data transformation when Python is + guaranteed or explicitly provisioned. Do not introduce Bash-specific syntax + or rewrite working scripts solely because of line count. +- Prefer small pure functions for transformations and keep state changes, + filesystem access, network access, and other side effects explicit at + boundaries. Follow existing framework conventions; do not introduce + functional abstractions solely for stylistic purity. ## Documentation updates diff --git a/docs/superpowers/specs/2026-08-26-os-bind-rp-abi-aware-repository-design.md b/docs/superpowers/specs/2026-08-26-os-bind-rp-abi-aware-repository-design.md new file mode 100644 index 0000000000..18671a9491 --- /dev/null +++ b/docs/superpowers/specs/2026-08-26-os-bind-rp-abi-aware-repository-design.md @@ -0,0 +1,81 @@ +# ABI-aware os-bind-rp package repository + +## Problem + +The installed Resolver Plugins repository configuration currently points to a +series-specific GitHub Release URL (`pkg-26.1` or `pkg-26.7`). A major +OPNsense upgrade changes the FreeBSD package ABI before `pkg` refreshes the +third-party catalogue. A client left on `pkg-26.1` after upgrading to 26.7 +therefore receives a `FreeBSD:14:amd64` catalogue while it requires +`FreeBSD:15:amd64`. + +OPNsense invokes `opnsense-update -u` before its `upgrade` syshook. A plugin +syshook cannot fix the repository in time for that invocation. + +## Decision + +Use a single ABI-aware client URL: + +``` +https://resolver-plugins.github.io/repository/pkg/${ABI}/latest +``` + +`pkg` expands `${ABI}`. The initial supported mappings are: + +| OPNsense series | ABI | repository path | +| --- | --- | --- | +| 26.1 | `FreeBSD:14:amd64` | `pkg/FreeBSD:14:amd64/latest` | +| 26.7 | `FreeBSD:15:amd64` | `pkg/FreeBSD:15:amd64/latest` | + +The published path is keyed by ABI, not by OPNsense series. This allows a +client to select a compatible catalogue automatically after the system ABI +changes. GitHub Pages for the existing `resolver-plugins/repository` +distribution repository serves the static endpoint. A GitHub Release tag +cannot serve this URL because the ABI value cannot be transformed into the +existing `pkg-` tag format. + +## Client migration + +The next production 26.1 package release migrates only the known, +plugin-managed legacy `resolver-plugins.conf` form to the ABI-aware URL. It +preserves the signing key, `mirror_type`, and all other repository settings. +It must not rewrite an unknown custom URL, disabled repository, alternate key, +or unrelated configuration; such a client receives a clear message with the +manual migration instructions instead. + +New installations write the ABI-aware configuration from the outset. Already +upgraded 26.7 systems require a one-time manual correction because their +existing 26.1 package cannot run a migration retrospectively. + +## Publication and trust boundary + +The existing CI build and signing boundary remains intact. After `pkg repo` +has created and signed the catalogue, publication places the complete, +verified directory at the path for its declared package ABI. Publication must +reject a directory whose package manifests do not share one ABI, or whose ABI +does not equal the selected release profile. + +The established series-specific GitHub Release channels remain available only +as short-lived compatibility and rollback endpoints until the ABI-addressed +endpoint has been proven and the supported clients have migrated. They are not +used by new installation instructions. + +## Verification + +- Unit tests cover ABI-to-publication-path validation and reject mixed ABI + package sets. +- Tests cover exact legacy configuration conversion and non-modification of + custom configuration. +- A disposable FreeBSD 14.3 client resolves the ABI-aware URL to the 26.1 + signed catalogue; a FreeBSD 15.1 client resolves the same URL to the 26.7 + catalogue. +- The package query output confirms each client sees only packages whose ABI + equals its own. +- The existing signature, package-chain, provenance, and release-profile + checks continue unchanged. + +## Non-goals + +- No OPNsense-core upgrade-flow modification or pre-upgrade hook. +- No runtime redirect, proxy, tunnel, or ABI inference service. +- No rewrite of user-managed repository definitions. diff --git a/docs/upstream-sync.md b/docs/upstream-sync.md index b526dcb476..a5bb4a78fe 100644 --- a/docs/upstream-sync.md +++ b/docs/upstream-sync.md @@ -17,17 +17,11 @@ to advance the separately pinned BIND runtime package in | --- | --- | --- | | `noop` | Current upstream BIND tree is unchanged and no newer stable series exists. | Finish without creating a ref, PR, or artifact. | | `update-review` | The current release series has an upstream BIND tree change. | Create or recover a `sync/bind/...` review PR. No artifact is built before review. | -| `bootstrap-review` | A newer OPNsense series exists and its BIND tree differs. | Create a `sync/bootstrap/...` review PR for the new release source. No artifact is built before review. | -| `bootstrap-build` | A newer OPNsense series exists and its BIND tree is unchanged. | Create the release branch, build the pinned BIND pair and plugin in the selected FreeBSD VM, and upload a seven-day artifact. | +| `bootstrap-review` | A newer OPNsense series exists, whether or not its BIND tree differs. | Create a `sync/bootstrap/...` review PR for the new release source. No package artifact is built before review. | -A bootstrap artifact contains the pinned `bind-tools` and `bind920` packages, -their BIND provenance, `os-bind-rp`, and build metadata. It is temporary, -retained for seven days, and is neither signed nor published to a package -channel. - -When a BIND change is present, review the generated PR and merge it only after -the fork-specific behavior has been verified. The synchronizer never silently -advances a release branch across an upstream BIND change. +Review the generated PR and merge it only after the fork-specific behavior has +been verified. The synchronizer never silently advances a release branch for a +new OPNsense series, even when its BIND tree is unchanged. ## Provenance and recovery