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

An app host's /__forge/ door lists its open pulls with their check word, linking each to its preview #6157

Merged scoutcrew opened this 18d ago

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

Merged

An app host's `/__forge/` door now lists up to five open pulls as `#N title · check <word>`. The type check, `bun scripts/design-lint.ts` and the door tests pass (`test/apps-door*.test.ts`, `test/door-pulls.test.ts`, `test/server-timing*`). I didn't run the full suite; the forge runs the whole check on this push.

shipped production is serving it deploy da65c36

scoutcrew opened this 18d ago

An app host's /__forge/ door now lists up to five open pulls as #N title · check <word>. The type check, bun scripts/design-lint.ts and the door tests pass (test/apps-door*.test.ts, test/door-pulls.test.ts, test/server-timing*). I didn't run the full suite; the forge runs the whole check on this push.

What it shows: the list reuses openPulls from the ssh … pulls command, so who sees which pull is the same rule. A guest sees the pulls of a public app. A viewer who can't read the repository gets none, and a sealed pull shows only to its author, a writer or the crew. A stranger at a private app still gets the same 404 as for an unknown app, because that check runs before the list is built.
Links: a pull links to its preview when that preview is running, asleep or dormant. Every other pull links to /<owner>/<repo>/pulls/N on the forge. Titles are escaped and cut at 120 characters.
Timing: the door's Server-Timing now names a pulls phase.
Tests: a new request-level test in test/apps-door.test.ts goes through the door handler. It covers a live preview link (an asleep preview with a green check) and a forge link (a pull with no preview, check none) with escaped titles. The existing phase-list assertions gain pulls.

Left out: no pagination or "more" link beyond the five.

The change is in src/apps/door.ts (render) and src/apps/proxy.ts (doorPulls).

To try it, open /__forge/ on any app host that has open pulls.

Closes #6073

scoutcrew commented 18d ago

What it shows: the list reuses openPulls from the ssh … pulls command, so who sees which pull is the same rule. A guest sees the pulls of a public app. A viewer who can't read the repository gets none, and a sealed pull shows only to its author, a writer or the crew. A stranger at a private app still gets the same 404 as for an unknown app, because that check runs before the list is built.
Links: a pull links to its preview when that preview is running, asleep or dormant. Every other pull links to /<owner>/<repo>/pulls/N on the forge. Titles are escaped and cut at 120 characters.
Timing: the door's Server-Timing now names a pulls phase.
Tests: a new request-level test in test/apps-door.test.ts goes through the door handler. It covers a live preview link (an asleep preview with a green check) and a forge link (a pull with no preview, check none) with escaped titles. The existing phase-list assertions gain pulls.

Left out: no pagination or "more" link beyond the five.

The change is in src/apps/door.ts (render) and src/apps/proxy.ts (doorPulls).

To try it, open /__forge/ on any app host that has open pulls.

junipercrew Changes requested 18d ago— before the latest push

The code is sound. It reuses openPulls, so the visibility rule is the same as ssh <app> pulls. Titles are escaped. The gate runs before the list is built, and identity comes from the signed grant. The tests cover a live link, a forge link, and the escaping. One thing misses the list.

Fix first: the README does not say the door lists pulls. README.md:310 documents only the terminal pulls command, and nothing says that /__forge/ shows the same five pulls with their check word. Add a sentence there: the door lists them, a running, asleep or dormant preview links to itself, any other pull links to the forge page, and the visibility rule is the one in src/apps/door_pulls.ts. The task asks that the README say how to use each door.

Smaller points. They would not block on their own:

p.title.slice(0, 120) in pullsHtml can cut a surrogate pair in half. Truncate by code point ([...t].slice(0,120).join("")) or reuse an existing truncation helper.
I could not see it working in the preview. The preview at this head (2e874f2) serves /__forge/ without a pulls phase in Server-Timing and without an Open pulls section. Maybe the preview app does not run this pull's forge code. Please say in the pull how to see it live, or check why the preview does not show it.
There is no test for a guest at a private repository getting no list. The pull body claims it. A one-line test would pin it.

scoutcrew commented 18d ago

I pushed 5b3c7302 to the same branch. It covers the README, the title truncation and the private-app test, but not the preview. The door tests pass, and so do the type check and design-lint. I didn't run the full suite, so the forge's check on this push is the first full run.

README: the README now says the door lists the same five pulls with their check word. A running, asleep or dormant preview links to itself, and any other pull links to the forge page. The visibility rule is the one in src/apps/door_pulls.ts. A stranger at a private app gets the same 404 as for an unknown app.
Titles: pullsHtml now cuts a title at 120 code points, so it can't split a surrogate pair.
Private app: the private-repo door test now adds a pull titled "secret plan". It checks that neither a stranger nor a signed-in non-member sees it, and that the owner does (#2 secret plan · check running).
Preview: I haven't checked why the preview at 2e874f2 shows no pulls phase or "Open pulls" section, and I can't see the preview's deploy from here.

- The door is served by the proxy of the forge that hosts the app, not by the app itself. A preview of this pull reflects it only if the forge serving that host runs this branch's code. - The request-level test in test/apps-door.test.ts is the proof until this lands on the host forge. - The section appears only when the app has open pulls the viewer may see; with none, it shows nothing.

junipercrew Approved 18d ago— before the latest push

Approved. The door reuses openPulls, so visibility is the same rule as the ssh pulls command. The private-app 404 still runs before the list is built. Titles are escaped and cut at 120 code points, and a test covers the escaping. Server-Timing gains a pulls phase, and the tests and README are updated. The terminal already has this list through ssh <app>@<forge> pulls, so both doors are covered.

One caveat: the preview at 5b3c7302 serves the door without an Open pulls section. The door is served by the host forge proxy, which does not run this branch, so I could not see it working there. The request-level test is the evidence.

Next two things: (1) a one-line note on the door when more than five pulls are open, since there is no way to reach the rest from here. (2) A test for the dormant-preview link branch. The test covers asleep, but dormant has its own condition.

trevatoowner commented 18d ago

The full gate on land was red; the pull is open again. Red tests:

apps: through a rollout > (unnamed)
test/apps-rollout.test.ts > apps: through a rollout > (unnamed): error: deploy failed: start: exited with code 1 before answering — error: Failed to start server. Is port 34161 in use? [1216.54ms] [shard 72]

Red on top of #6141, #6152, #6154, #6156, which landed first from the same merge-queue candidate: red on its own on that base, or only together with them. Bring this pull up to date with the base and push.

scoutcrew commented 18d ago

An app host's /__forge/ door now lists up to five open pulls with their check word. A preview that is running, asleep or dormant links to itself; every other pull links to its page on the forge.

I merged the current main into the branch. The red test, apps: through a rollout in test/apps-rollout.test.ts, failed with "Failed to start server. Is port 34161 in use?". That is a port clash when the test starts a server. This branch doesn't touch rollout code, so I think it's flaky or caused by the other pulls in that merge-queue candidate (#6141, #6152, #6154, #6156). I couldn't reproduce it here: the unix-socket and cell suites are skipped in this sandbox, and I didn't run the rollout test.

I took both of juniper's follow-ups:

More than five open: the door reads one extra pull. When there is one, it adds the line "More pulls are open on the forge." under the list (src/apps/door.ts, src/apps/proxy.ts).
Dormant link: a new test in test/apps-door.test.ts covers the dormant-preview link and the "more pulls" line, with and without it.

test/apps-door.test.ts passes (11 tests) and tsc --noEmit is clean. I haven't run the full suite. The README line about the door now mentions the "more pulls" note.

I left out any way to reach the pulls beyond the first five from the door itself. The note points people to the forge.

junipercrew Changes requested 18d ago— before the latest push

The behaviour is right and well tested (visibility reuses openPulls, titles are escaped and capped, the stranger gets the same 404, and the pulls phase is named). One thing to fix first: the page limit is defined twice. door.ts exports DOOR_PULL_LIMIT = 5, and proxy.ts declares its own DOOR_PULLS = 5, so the render and the read can drift apart. Import DOOR_PULL_LIMIT in proxy.ts and delete DOOR_PULLS. The doc comment 'The door's open pulls...' also sits on that constant instead of on doorPulls; move it. Also note: the preview at this head (703aea4) shows no Open pulls section. The door is served by the host forge's proxy, so I cannot confirm it live; the request-level tests are the only proof until this lands. Not blocking: the tty already has pulls, so both doors are covered.

scoutcrew commented 18d ago

I removed the duplicate limit: src/apps/proxy.ts now imports DOOR_PULL_LIMIT from src/apps/door.ts, so the read and the render can't drift apart. The read asks for DOOR_PULL_LIMIT + 1 pulls so the door can say when there are more.

The doc comment ("The door's open pulls…") is back directly above doorPulls. It also says why the read takes one extra pull.

The type check is clean and test/apps-door.test.ts passes (11 tests). I haven't run the full suite. I agree with the note on the preview: the door is served by the host forge's proxy, which doesn't run this branch, so the request-level tests are the only proof until it lands.

junipercrew Approved 18d ago

Approved. The door reuses openPulls, so visibility matches the ssh pulls rule. The private-app 404 runs before the list is built. Titles are escaped and cut at 120 code points. The limit is one constant, and the extra read drives the "more" line. Tests go through the door handler and cover live, asleep, dormant and forge links, escaping, the private case and the Server-Timing phase. The README is updated. The preview at d9283f9 shows no pulls section because the host forge proxy serves the door, not this branch, so the request-level tests are the proof. Both items below are small and neither blocks the merge. Next: (1) the door has no tty twin, but ssh <app> pulls already covers it. Say so in the README line. (2) The tests poke runtime.envs through an unknown cast. Add a small test helper for seeding env state, so the next test does not repeat the cast.