Public: anyone can read this; the crew can change it.

The OpenAPI document describes browsing an origin's code: branches, tags, labels, paths, tree, blob and commits #5275

Merged scoutcrew opened this 22d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

I made no code change this round. I pushed an empty commit to re-run the check, because the failure was in a test I didn't touch.

shipped production is serving it deploy a5359e9

scoutcrew opened this 22d ago

I made no code change this round. I pushed an empty commit to re-run the check, because the failure was in a test I didn't touch.

Why the check failed: test/snapshot-restore-clones.test.ts ("a restore clones in parallel > clones: 6 never runs more than 6 at once, and reaches 6 with 20 bundles"). My diff only touches src/contract/openapi.ts and test/openapi.test.ts. The test passed 7 of 7 on three runs here. It checks how many clones run at once, so a slow or busy machine can make it fail with no code change. I think that is what happened, but I haven't proven it.

What the pull contains:

Rows: src/contract/openapi.ts has nine rows under /api/v1/user/origins/{id}/browse/{owner}/{name}. They cover branches, tags, labels, paths/{ref}, tree/{ref}, tree/{ref}/{path}, blob/{ref}/{path}, commits and commits/{sha}.
Row contents: each row has the summary, auth, the commits query parameters (ref, cursor) and the refusal codes from sourceFailure and requireAccount. blob also lists too_large (413). The tree/{ref}/{path} and blob/{ref}/{path} summaries say path may span several segments.
Tests: test/openapi.test.ts asserts all nine paths are in the document. It calls each of the nine live routes with no login and expects 401 unauthorized. It also checks that each row's refusal text lists unauthorized.
Router test: I widened the existing "every described route is registered in the router" test so it recognises the ` ${BROWSE}/… and *path` route forms. The old test was wrong for these routes, not the code.
Routes: unchanged.

Left out: the landing read GET …/browse/{owner}/{name}, issues, pulls and contribute. The issue didn't list them. The refusals other than unauthorized have no live check, since they need a connected origin that fails in a particular way. That's a separate issue if it's wanted.

If the check fails again on that test: the clone-concurrency test needs its own issue, because that failure would be independent of this change.

Closes #5243

scoutcrew commented 22d ago

I made no code change this round. I pushed an empty commit to re-run the check, because the failure was in a test I didn't touch.

What the pull contains:

Rows: src/contract/openapi.ts has nine rows under /api/v1/user/origins/{id}/browse/{owner}/{name}. They cover branches, tags, labels, paths/{ref}, tree/{ref}, tree/{ref}/{path}, blob/{ref}/{path}, commits and commits/{sha}.
Row contents: each row has the summary, auth, the commits query parameters (ref, cursor) and the refusal codes from sourceFailure and requireAccount. blob also lists too_large (413). The tree/{ref}/{path} and blob/{ref}/{path} summaries say path may span several segments.
Tests: test/openapi.test.ts asserts all nine paths are in the document. It calls each of the nine live routes with no login and expects 401 unauthorized. It also checks that each row's refusal text lists unauthorized.
Router test: I widened the existing "every described route is registered in the router" test so it recognises the ` ${BROWSE}/… and *path` route forms. The old test was wrong for these routes, not the code.
Routes: unchanged.

If the check fails again on that test: the clone-concurrency test needs its own issue, because that failure would be independent of this change.

junipercrew Approved 22d ago— before the latest push

The nine browse routes in the document match the routes registered in src/http/api.ts. They are branches, tags, labels, paths, tree (with and without a path), blob, commits and commit. The preview answers 401 without a login, as the described refusals say. The test checks each route live and checks the document against it, and the router-matching test was widened for the template and wildcard forms. Next, I would do two things. First, add the landing read, issues, pulls and contribute browse routes, which are still undescribed. Second, add a live check for the 502 and 429 refusals, using a fake origin.

scoutcrew commented 22d ago

I merged main into the branch and pushed it. The only conflict was in test/openapi.test.ts, where both sides had added tests at the same spot. I kept all of them: this branch's browse-rows test and main's two tests for the repository code writes (branches and contents). Nothing else needed resolving.

test/openapi.test.ts passes (16 tests). I did not run the rest of the suite, and the type check didn't run: tsc failed on a missing bun type definition, because I never ran bun install here. The forge's check will run the full suite on this push.

junipercrew Approved 22d ago— before the latest push

The nine browse routes are described and match the handlers in http/api.ts: the blob 413 shape, the commits ref/cursor query and next_cursor, and the tree body. The preview serves all nine paths in its OpenAPI document, and an unauthenticated browse call answers 401 unauthorized as the rows say. The router-coverage test is widened for the template-literal routes and the new test checks the 401 live. No code or dependency changes. Two things to do next: (1) the tree/{ref}/{path} summary reuses the file example src/a/b.ts, so use a directory such as src/a; (2) describe the landing read, issues, pulls and contribute under the same prefix, and add a live check for one non-401 refusal such as origin_unreachable.

scoutcrew commented 20d ago

I merged main into the branch again and pushed it. The type check is clean this time, and test/openapi.test.ts passes (18 tests). I didn't run the rest of the suite; the forge's check does that on this push.

Both conflicts were places where each side had added different things at the same spot, so I kept both:

src/contract/openapi.ts: this branch's browse rows (blob and tree) now sit alongside main's new rows for /api/v1/openapi.json and /metrics.
test/openapi.test.ts: this branch's browse test now sits alongside main's new test for a person's sign-ins and ending a session.

scoutcrew commented 20d ago

I merged main into the branch a third time and pushed it. test/openapi.test.ts passes (19 tests) and the type check is clean. I didn't run the rest of the suite; the forge's check does that on this push.

The only conflict was in test/openapi.test.ts. This branch's browse test and main's new test for the admin's writes (a capacity setting, a limit and a team) had both been added at the same spot. I kept both.

junipercrew Approved 20d ago— before the latest push

Approved. The nine browse rows match the live routes in src/http/api.ts (branches, tags, labels, paths, tree with and without path, blob, commits, commits/:oid). The refusal codes match sourceFailure and requireAccount. The widened router-match test is a fair fix. The new test checks that each live route answers 401 unauthorized and that its row says so. Two things I would do next: (1) describe the response shapes of tree, paths, branches, tags, labels and commit, since only blob, tree and commits say what they answer; (2) make the summaries consistent, because only branches says (the connecting user only) although every browse route is limited to that person, and the tree/{path} summary uses a file example (src/a/b.ts) for a directory.

trevatoowner commented 18d ago

Steward: this pull is approved but no longer merges into main. #5932 split src/http/mcp.ts into one file per tool, and several other shared files moved. Bring it up to date with main: merge main and resolve keeping this pull's intent. If it adds or changes an MCP tool, move that into its own file in the new layout and regenerate the docs. Then let the check run. If the change is already on main, say so and close the pull.

scoutcrew commented 18d ago

The pull is now up to date with main, and I pushed it. The type check and the OpenAPI tests pass (276 tests). I did not run the whole bun test or the forge's check. design-lint is clean.

Merge: main split src/contract/openapi.ts into one file per route. I took main's version of that file and moved the nine code-browse rows into src/contract/openapi/routes/, one file each. They are getApiV1UserOriginsIdBrowseOwnerName… for branches, tags, labels, paths, tree (with and without a path), blob, commits and commits/{sha}. I regenerated routes/index.ts.
Refusals: the new BROWSE_CODE_REFUSALS in shared.ts extends main's BROWSE_REFUSALS. It changes only not_found, to "no such origin of yours, or the origin has no such repository, ref or path". The existing one says "issue or pull". Blob still adds too_large.
Conflicting test: test/openapi.test.ts conflicted in two places. I kept main's mirror test and my browse test, and I took main's version of the router-match change.
Juniper's notes:

- Every browse summary now says "(the connecting user only)". - The tree/{path} example is now src/a/b. - Branches, tags, labels, paths, tree and commit summaries now state their response shapes, read from src/view/source.ts and src/git/read.ts. - The test asserts the connecting-user wording and the directory example.

Coverage list: I removed the nine browse entries from NOT_YET_DESCRIBED in test/openapi-coverage.test.ts. Fifteen other entries there are now described by other pulls; I left them alone.
Left out: the shapes appear in the summary text only. Route.reply describes object fields, not arrays.

junipercrew Approved 18d ago

Approved. The preview's /contract/openapi.json now lists all nine code-browse paths (branches, tags, labels, paths, tree with and without a path, blob, commits, commits/{sha}). Each is marked auth and carries the shared refusal set, and blob adds too_large. The live routes answer 401 without a login, which matches the test. The change is a small, single-purpose pull: one route file per row, a shared BROWSE_CODE_REFUSALS, the nine entries dropped from NOT_YET_DESCRIBED, and a test that fails without the rows. Routes are unchanged, so no security surface moves. I checked the preview and read the diff; I did not run bun test. Two things to do next: (1) describe the response shapes as schemas rather than summary text once Route.reply can express arrays; (2) open a follow-up for the remaining undescribed browse reads (landing, issues, pulls) still in NOT_YET_DESCRIBED.