mosaicstack/stack
25
0 Fork 1
Code Issues 394 Pull Requests 13 Packages Projects Releases 15 Wiki Activity

Forge wrapper follow-ups from PR #1842 reviews #1846

New Issue
Open
opened 2026-11-05 20:12:38 +00:00 by jason.woltje · 0 comments
jason.woltje commented 2026-11-05 20:12:38 +00:00
Owner
Copy link
Copy source

Follow-ups from the PR #1842 reviews (forge wrapper gaps, merge policy, PR workflow skills). None blocked the merge. File references are to packages/mosaic/framework/tools/git/ at PR head faf3f90e3.

Pagination in forge-api.py pages()

  1. Server ignores page (R2-M1). A Gitea endpoint that honours limit but ignores page now returns its first page as the whole collection, in 2 requests, exit 0. The unordered compare added in 01662b281 does the same when the repeat comes back reordered. Suggested fix: treat a repeated page of length LIMIT as incomplete and exit 6, or fetch the known unpaginated endpoints once and keep strict page-until-empty elsewhere.
  2. Non-adjacent repeat. The repeat and overlap check compares only adjacent pages. If page 3 repeats page 1, the result has duplicates (300 records instead of 200). This predates #1842.
  3. Comment added mid-walk (R2-M2). On an unpaginated endpoint, a comment added between the page 1 and page 2 requests looks like a partial overlap and gives a transient exit 6. It fails closed and a retry clears it. At minimum, document it in the README failure table.

Gitea approvals

  1. official outside the whitelist (M1). forge-api.py:139 and :473 check official only when the base rule enables an approvals whitelist. Gitea counts only official approvals in every case. A read-only approver, or an approval followed by a dismissed rejection on the same head, makes the adapter report approved=True while Gitea counts zero. Merge still refuses with 405, so the damage is a wrong status. Fix: on Gitea and Forgejo, require official is True whenever the row has the field.

Merge wrapper

  1. Legacy options on the non-squash path (M2). When the pinned declaration has a merge key and the method is not squash, pr-merge.sh execs forge-api.py merge with the original arguments. The adapter has no --dry-run, -d, --base-line, --no-ci-expected or trailer options and requires --expect-head, so pr-merge.sh -n 5 -m merge --dry-run exits 2 with a JSON usage error. README.md says the legacy interface "retains its options", which is true only for squash. Fix: map the supported options and refuse the rest by name, or narrow the README sentence.

Docs

  1. README contract sections removed (M3). The README rewrite dropped the git-credential-mosaic seat/service store table, the "What happens when nothing resolves" fail-closed table, tea login provisioning through seat-logins.sh, and "Running these tests". The behaviour still lives in seat-logins.sh, detect-platform.sh and git-credential-mosaic.impl. Restore short sections or link to the scripts.

Tests and CI

  1. test-merge-policy.py:206 sets metadata['baseRefOid'] = declared when it already holds that value. Remove the redundant line.
  2. test-forge-schema.sh needs eslint. Confirm the CI pipeline runs it, not skips it.

Port

  1. Port the #1842 framework changes to the refactor branch, which holds the rebuilt foundation.
Follow-ups from the PR #1842 reviews (forge wrapper gaps, merge policy, PR workflow skills). None blocked the merge. File references are to `packages/mosaic/framework/tools/git/` at PR head faf3f90e3. ## Pagination in `forge-api.py` `pages()` 1. **Server ignores `page` (R2-M1).** A Gitea endpoint that honours `limit` but ignores `page` now returns its first page as the whole collection, in 2 requests, exit 0. The unordered compare added in 01662b281 does the same when the repeat comes back reordered. Suggested fix: treat a repeated page of length LIMIT as incomplete and exit 6, or fetch the known unpaginated endpoints once and keep strict page-until-empty elsewhere. 2. **Non-adjacent repeat.** The repeat and overlap check compares only adjacent pages. If page 3 repeats page 1, the result has duplicates (300 records instead of 200). This predates #1842. 3. **Comment added mid-walk (R2-M2).** On an unpaginated endpoint, a comment added between the page 1 and page 2 requests looks like a partial overlap and gives a transient exit 6. It fails closed and a retry clears it. At minimum, document it in the README failure table. ## Gitea approvals 4. **`official` outside the whitelist (M1).** `forge-api.py:139` and `:473` check `official` only when the base rule enables an approvals whitelist. Gitea counts only official approvals in every case. A read-only approver, or an approval followed by a dismissed rejection on the same head, makes the adapter report approved=True while Gitea counts zero. Merge still refuses with 405, so the damage is a wrong status. Fix: on Gitea and Forgejo, require `official is True` whenever the row has the field. ## Merge wrapper 5. **Legacy options on the non-squash path (M2).** When the pinned declaration has a merge key and the method is not squash, `pr-merge.sh` execs `forge-api.py merge` with the original arguments. The adapter has no `--dry-run`, `-d`, `--base-line`, `--no-ci-expected` or trailer options and requires `--expect-head`, so `pr-merge.sh -n 5 -m merge --dry-run` exits 2 with a JSON usage error. README.md says the legacy interface "retains its options", which is true only for squash. Fix: map the supported options and refuse the rest by name, or narrow the README sentence. ## Docs 6. **README contract sections removed (M3).** The README rewrite dropped the git-credential-mosaic seat/service store table, the "What happens when nothing resolves" fail-closed table, tea login provisioning through `seat-logins.sh`, and "Running these tests". The behaviour still lives in `seat-logins.sh`, `detect-platform.sh` and `git-credential-mosaic.impl`. Restore short sections or link to the scripts. ## Tests and CI 7. `test-merge-policy.py:206` sets `metadata['baseRefOid'] = declared` when it already holds that value. Remove the redundant line. 8. `test-forge-schema.sh` needs eslint. Confirm the CI pipeline runs it, not skips it. ## Port 9. Port the #1842 framework changes to the `refactor` branch, which holds the rebuilt foundation.
Sign in to join this conversation.
No Branch/Tag Specified
Labels
Clear labels
bug
Defect in framework tooling or delivered code
 fleet-enhancement
Fleet-wide reliability/speed/cost improvements surfaced by the nightly enhance pass
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
code-be-01 (Mosaic fleet seat code-be-01) code-be-02 (Mosaic fleet seat code-be-02) code-dogfood-01 (Mosaic fleet seat code-dogfood-01) code-infra-01 (Mosaic fleet seat code-infra-01) darkwing (Mosaic fleet seat darkwing) dewey (Mosaic fleet seat dewey) fargo filbert (Mosaic fleet seat filbert) fred gate-merge-01 (Mosaic fleet seat gate-merge-01) happy jason.woltje (Jason Woltje) marcie merge-gate mosaic-stack-coder-bot (mosaic-stack coder bot) mosaic-stack-cto-bot (mosaic-stack cto bot) mosaic-stack-pm-bot (mosaic-stack pm bot) ops-01 (Mosaic fleet seat ops-01) ops-02 (Mosaic fleet seat ops-02) ops-03 (Mosaic fleet seat ops-03) ops-ci-01 (Mosaic fleet seat ops-ci-01) ops-deploy-01 (Mosaic fleet seat ops-deploy-01) orch-01 (Mosaic fleet seat orch-01) pepper resume rev-code-01 rev-code-02 rev-security-01 rev-security-02 rev-security-03 (Mosaic fleet seat rev-security-03) rocko (Mosaic fleet seat rocko) sanity scooby (Scooby) scrappy shaggy tiny topher (Mosaic fleet seat topher) velma veronica (Mosaic fleet seat veronica) vision woodpecker
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#1846