Commit graph

2 commits

Author SHA1 Message Date
Tejas Chopra
ac8646aa3c
fix(ci): scope the release credential and stop persisting it to disk (#3062)
## Description

`RELEASE_PLEASE_TOKEN` is currently a maintainer's personal PAT. It
bypasses branch and tag protection on `main` (`release-please.yml` says
so in its own comment), and forging a tag with it fires `release.yml`
and `docker.yml` on `release: published`, which publish to PyPI, npm and
GHCR. If it is a classic token with `repo` scope it is also valid
against every other repository that account can reach.

`release-metadata-sync.yml` made that credential readable on the runner.
`actions/checkout` defaults to `persist-credentials: true`, writing the
token into `.git/config`, and the very next step runs
`scripts/version-sync.py` **from the checked-out branch**. The trigger
is a push to the glob `release-please--branches--**`, which is not a
protected namespace, so a principal with push access could land a
modified `version-sync.py` and read it.

Closes #2955.

## Type of Change

- [x] Bug fix (non-breaking change that fixes an issue)
- [ ] New feature (non-breaking change that adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] Documentation update

## Changes Made

- Both release workflows now prefer a GitHub App installation token —
scoped to this repository, expiring in an hour — over the PAT, via
`actions/create-github-app-token@v3`.
- The minting step is gated on `vars.RELEASE_APP_ID` and marked
`continue-on-error`, so an unconfigured app falls through to the
existing `PAT -> GITHUB_TOKEN` chain and nothing breaks today.
- `release-metadata-sync.yml`'s checkout no longer persists credentials,
and no longer receives a token at all.
- The final push supplies the credential through the step's own `env`
and an explicit remote URL, so it is never on disk while branch-supplied
code runs.

## Testing

- [x] Unit tests pass
- [x] Linting passes (ruff check + format on the test file)
- [ ] Type checking passes — N/A (YAML + test only)
- [x] New tests added for new functionality

### Test Output

```text
$ .venv/bin/python -m pytest tests/test_release_workflows.py -q
1 failed, 44 passed, 1 skipped in 0.23s
```

The single failure is `test_no_native_tls_in_wheel_build_tree`, which
shells out to `cargo`. It reproduces identically on unmodified `main` on
this machine (no Rust toolchain installed) and is unrelated to this
change.

New tests only:

```text
$ .venv/bin/python -m pytest tests/test_release_workflows.py -q -k "persist_credentials or scoped_app_token"
3 passed, 46 deselected in 0.18s
```

Against the parent commit:

```text
FAILED test_metadata_sync_does_not_persist_credentials_for_branch_supplied_code
FAILED test_release_workflows_prefer_scoped_app_token[release-please.yml-release-please]
FAILED test_release_workflows_prefer_scoped_app_token[release-metadata-sync.yml-sync]
3 failed, 46 deselected
```

## Real Behavior Proof

- Environment: macOS 15 (darwin 25.4.0), Python 3.12.13; workflows
parsed with PyYAML, not executed on a runner.
- Exact command / steps: parse both workflow files and assert (a) every
`actions/checkout` step sets `persist-credentials: false` and receives
no `token`, (b) exactly one gated `create-github-app-token` step exists
per workflow, and (c) every credential consumer places
`steps.app-token.outputs.token` ahead of `secrets.RELEASE_PLEASE_TOKEN`
in its fallback chain.
- Observed result: all three assertions pass on this branch and fail on
the parent commit. Both files remain valid YAML.
- **Not tested — important:** none of this has executed on a GitHub
runner. I have not minted a real installation token, not confirmed the
app-token step's `continue-on-error` fallback behaves as expected when
`vars.RELEASE_APP_ID` is unset, and not performed a real push with the
explicit-remote-URL form. The first live release run is the real test.

## Runtime Rollout Safety

- Rollout-managed feature(s): none.
- Minimum rollout channel: N/A.
- Stable/default behavior changed: no, unless `vars.RELEASE_APP_ID` is
set — without it both workflows resolve to exactly today's credential
chain.
- Kill switch / disable path: unset `vars.RELEASE_APP_ID` to fall back
to the PAT.
- Unsafe override required: none.
- Qualification impact: none.
- Rollback path: revert this commit.

## Review Readiness

- [x] I have performed a self-review
- [x] This PR is ready for human review

## Checklist

- [x] My code follows the project's style guidelines
- [x] I have performed a self-review of my code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective
- [x] New and existing unit tests pass locally with my changes

## Additional Notes

**This narrows blast radius; it does not make the trigger safe on its
own.** For an `on: push` workflow GitHub reads the workflow file from
the pushed ref, so a principal with push access can still edit this file
on their branch. The durable fix is the scoped app token *plus revoking
the personal PAT* — the revocation is a console action and is
deliberately not in this commit.

**Two repo settings are required to actually complete #2955**, and
neither can land in git:

```
vars.RELEASE_APP_ID              (repository variable)
secrets.RELEASE_APP_PRIVATE_KEY  (repository secret)
```

Until those exist this PR is a no-op on behavior and a defense-in-depth
improvement on the `persist-credentials` path only.

Co-authored-by: Tejas Chopra <tejas@Tejass-MacBook-Pro.local>
2026-08-16 19:05:32 -07:00
Tejas Chopra
5383c6bf2f
fix(release): sync generated version metadata on the release branch (#2659)
## Description

The 0.33.0 release PR (#2339) has sat in `changes-requested` since
2026-07-17. Root cause: **release-please only rewrites `pyproject.toml`
and its configured `extra-files`**, but other tracked files also carry
the version — and `server.json` is asserted byte-for-byte against
`render_server_json()`, which derives its version from `pyproject.toml`.
So the bump alone fails
`tests/test_mcp_registry/test_server_json.py::test_root_server_json_matches_builder`
(the `test (2)` shard) on every regenerated release PR.

Nothing in the repo regenerated `server.json` at all, so it fell behind
every release.

Unblocks #2339.

### Why the release *build* passes but the release PR does not

`release.yml` already runs `scripts/version-sync.py` immediately before
its own `verify-versions.py` gate (lines 145 and 278). That is why
`build` and `build-wheels` are green on #2339 despite the drift — it
syncs in the workspace, uncommitted. The regular CI test job does
**not** sync, so the fix has to be committed to the branch.

This also explains why reviewers kept seeing `verify-versions.py` fail
locally while CI's build jobs passed: the verifier is never run
un-synced inside `release.yml`.

## Type of Change

- [x] Bug fix (non-breaking change that fixes an issue)
- [ ] New feature (non-breaking change that adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] Documentation update
- [ ] Performance improvement
- [ ] Code refactoring (no functional changes)

## Changes Made

- **`scripts/version-sync.py`**: also write `server.json`. It was the
one version-carrying file with no writer anywhere. Values are rewritten
in place so key order and formatting keep matching the builder's
byte-for-byte output (verified: the file is pure ASCII and round-trips
exactly through `json.dumps(..., indent=2) + "\n"`).
- **`.github/workflows/release-metadata-sync.yml`** (new): on a push to
`release-please--branches--**`, run version-sync → gate on
verify-versions → commit if changed.
- **Keyed off the branch push** because release-please force-regenerates
that branch on every merge to main. That is precisely what wiped the
hand-pushed metadata fixes on #2339 (`2a86c8ff`, `d5ea4dc5`) — a push
trigger re-heals after every regeneration instead of being lost.
- **Uses the same PAT as `release-please.yml`**: a `GITHUB_TOKEN` push
does not trigger workflows, so the release PR's checks would never
re-run against the synced commit and would stay red.
- **Idempotent**: the self-triggered rerun finds no diff and exits
before pushing, so the loop terminates after one no-op run.
- **Corrected pre-existing drift on `main`**: the agent-hooks plugin
manifests, both marketplace manifests, and `.releasemetadata` were
stranded at **0.31.0** — never bumped for 0.32.0 either.
`verify-versions.py` now passes on `main`.

### Why not more `extra-files` entries

That would need ~13 jsonpath entries restating what `version-sync.py`
already knows, and a jsonpath that fails to match **fails silently** —
the same class of failure this PR removes, discoverable only after a
real release PR regenerates. There is also no precedent for nested
jsonpath (`$.packages[0].version`, `$.metadata.version`) in the config
today; both existing entries are plain `$.version`. Running the script
keeps one source of truth, and files added to it later are covered with
no change here.

## Testing

- [x] Unit tests pass (`pytest`)
- [x] Linting passes (`ruff check .`)
- [ ] Type checking passes (`mypy headroom`) — no `headroom/` sources
touched
- [x] New tests added for new functionality
- [x] Manual testing performed

### Test Output

```text
$ python -m pytest scripts/tests/ tests/test_release_workflows.py tests/test_mcp_registry/ -q
207 passed in 2.69s

$ ruff check scripts/version-sync.py scripts/tests/test_version_sync.py tests/test_release_workflows.py
All checks passed!
$ ruff format --check <same>
3 files already formatted

$ actionlint .github/workflows/release-metadata-sync.yml
(clean)
```

New tests:
- `test_server_json_version_is_synchronized` — version-sync moves both
`server.json` version fields and preserves the other keys.
- `test_release_metadata_sync_runs_on_release_please_branch` — asserts
the trigger, the sync→verify→commit ordering, the no-op guard, and the
PAT.
- `test_version_sync_covers_every_file_the_verifier_gates` — guards
`version-sync.py` and `verify-versions.py` against drifting apart again,
which is the root cause here.

## Real Behavior Proof

- **Environment:** macOS (Darwin arm64), Python 3.12, repo venv.
- **Exact command / steps:** reproduced the CI failure locally by
simulating release-please's partial bump, then applying the fix.

**Reproducing the exact `test (2)` failure** — set `pyproject` to 0.33.0
while `server.json` stays at 0.32.0, as release-please leaves it:

```text
$ python -m pytest tests/test_mcp_registry/test_server_json.py -q
FAILED tests/test_mcp_registry/test_server_json.py::test_root_server_json_matches_builder
1 failed, 3 passed
```

**After `version-sync.py`:**

```text
$ python scripts/version-sync.py && python -m pytest tests/test_mcp_registry/test_server_json.py -q
4 passed
```

**Both gates green on a simulated 0.33.0 bump:**

```text
$ python scripts/version-sync.py --version 0.33.0
Version synchronized to 0.33.0
$ python scripts/verify-versions.py
All versions aligned at 0.33.0
$ python -m pytest tests/test_mcp_registry/test_server_json.py -q
4 passed
```

**Idempotency** (the property the workflow's loop-termination relies
on): re-running against an already-synced tree leaves `pyproject.toml`,
`server.json`, `openclaw`, and `sdk/typescript` untouched.

- **Not tested:** the workflow has not executed on a real release-please
branch regeneration — that can only be exercised once this is on `main`
and release-please next updates #2339. The PAT push path and the
self-trigger no-op are reasoned from `release-please.yml`'s existing
token comment and from local idempotency, not observed in CI.

## Review Readiness

- [x] I have performed a self-review
- [x] This PR is ready for human review

## Checklist

- [x] My code follows the project's style guidelines
- [x] I have performed a self-review of my code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] New and existing unit tests pass locally with my changes
- [x] I did **not** edit `CHANGELOG.md`

## Additional Notes

**Context on the v0.32.0 release failure, since it is easy to misread as
"images never build".** Every artifact built for v0.32.0 — all 5 wheel
platforms including Windows, all 16 Docker builds + 8 manifests +
`promote-latest`, npm, and GitHub Packages. Only `publish-pypi` failed
(PyPI attestations, already fixed by `f9cbdd6e` / #2405), and
`create-release` was skipped because it depends on it. That is why the
release looked like it produced nothing.

**Separate, approaching blocker — not addressed here.** PyPI is at
**9.69 GB of its 10 GB project cap (96.9%)**, leaving ~305 MB against
~68 MB per release, so roughly 4 more releases fit. The `0.21.x` series
alone holds **6.58 GB across 31 releases**, from the old
every-push-is-a-release era; pruning it would reclaim two thirds of the
quota. Worth a separate issue.

**`.releasemetadata` is written but never read** by anything outside
`version-sync.py` and its test. It is kept in sync here for internal
consistency, but it may be a deletion candidate.
2026-07-29 15:12:04 -07:00