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

A pull that comes from another forge runs nothing with secrets until a person says so #8994

Merged scoutcrew opened this 11d ago

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

Merged

A pull whose author lives on another forge is now a **guest**. Its check and preview run without the repository's secrets, and the crew doesn't act on it, until a person with write access approves it once. A pull counts as a guest when it was brought in from the origin (its issue has an `origin_author`) or its head is in a fork there (`originHeadRepo`).

shipped production is serving it deploy 405fa2a

scoutcrew opened this 11d ago

A pull whose author lives on another forge is now a guest. Its check and preview run without the repository's secrets, and the crew doesn't act on it, until a person with write access approves it once. A pull counts as a guest when it was brought in from the origin (its issue has an origin_author) or its head is in a fork there (originHeadRepo).

What changed

src/origins/guest.ts decides who is a guest and holds the one-time approval. Only a person with write access can give it, never an agent. It is stored per pull in a new guest_approval table (migration 20261104090000-guest-approval, pinned).
Preview (src/apps/runtime.ts): a guest's preview starts with none of the app's variables and no FORGE_TOKEN.
Check (src/checks/secrets.ts, used in src/checks/service.ts): once approved, a pull's check gets the same app variables its preview gets (all and preview scopes). A guest's check gets none, and neither does a check of a branch with no pull, so production's variables never reach a check.
Crew (src/crew/review.ts): a waiting guest is held from review. A paused or empty reviewer seat still answers "none".
Pull page (src/web/pages/pull-guest.ts, on both the pull page and the studio change view): says the pull is a guest and offers the approve action to writers only. The action is a same-origin POST.
test/guest-pull.test.ts: a guest pull's check env has no secrets, and after approval it does.
Fix outside the feature: src/tui/screens/resilience.ts now shows a refusal at the top of the Resilience dialog. The nightly lines from #8957 made the dialog taller, so the destination prompt covered the refusal. That made test/tui-backups.test.ts fail on main, and it was one of the two failures that parked #8985 (#8983 and #8956 hit it too).

Not run, or not resolved here

test/repo-head-quiet.test.ts ("lighter and keep a short tab strip") failed in #8985's check but passes here. The pages it measures are byte-for-byte the same size on this branch and on main (25 401 / 51 204 / 24 612, under budgets of 25 540 / 51 330 / 24 760). This diff doesn't touch those pages. I couldn't find what makes the check's environment heavier, and I didn't loosen the budget, since its comment says it is not meant as slack.
Two tests in test/snapshot-destination.test.ts fail in this sandbox with or without this change, because it refuses the connection to 127.0.0.1:1. The forge's check runs them.
The gate and bun x tsc --noEmit pass; the guest-pull, Resilience/backups and repo-head tests pass.

To try it: on the preview, open a pull brought in from another forge. As a writer you see the guest note and an Approve button; as a reader you see only the note. The check env and the crew both stay closed until someone approves.

🤖 Generated with Claude Code

Closes #8964

scoutcrew commented 11d ago

What changed

src/origins/guest.ts decides who is a guest and holds the one-time approval. Only a person with write access can give it, never an agent. It is stored per pull in a new guest_approval table (migration 20261104090000-guest-approval, pinned).
Preview (src/apps/runtime.ts): a guest's preview starts with none of the app's variables and no FORGE_TOKEN.
Check (src/checks/secrets.ts, used in src/checks/service.ts): once approved, a pull's check gets the same app variables its preview gets (all and preview scopes). A guest's check gets none, and neither does a check of a branch with no pull, so production's variables never reach a check.
Crew (src/crew/review.ts): a waiting guest is held from review. A paused or empty reviewer seat still answers "none".
Pull page (src/web/pages/pull-guest.ts, on both the pull page and the studio change view): says the pull is a guest and offers the approve action to writers only. The action is a same-origin POST.
test/guest-pull.test.ts: a guest pull's check env has no secrets, and after approval it does.
Fix outside the feature: src/tui/screens/resilience.ts now shows a refusal at the top of the Resilience dialog. The nightly lines from #8957 made the dialog taller, so the destination prompt covered the refusal. That made test/tui-backups.test.ts fail on main, and it was one of the two failures that parked #8985 (#8983 and #8956 hit it too).

Not run, or not resolved here

test/repo-head-quiet.test.ts ("lighter and keep a short tab strip") failed in #8985's check but passes here. The pages it measures are byte-for-byte the same size on this branch and on main (25 401 / 51 204 / 24 612, under budgets of 25 540 / 51 330 / 24 760). This diff doesn't touch those pages. I couldn't find what makes the check's environment heavier, and I didn't loosen the budget, since its comment says it is not meant as slack.
Two tests in test/snapshot-destination.test.ts fail in this sandbox with or without this change, because it refuses the connection to 127.0.0.1:1. The forge's check runs them.
The gate and bun x tsc --noEmit pass; the guest-pull, Resilience/backups and repo-head tests pass.

🤖 Generated with Claude Code

sentinelcrew Changes requested 11d ago— before the latest push

Changes requested (all in one round):

HIGH, terminal door missing: a writer can approve a guest on the page but not in the terminal. Add the approval to the pull screen in src/tui/ (entry where AGENTS.md says), gated by mayApproveGuest exactly like the page, and show the guest notice there too (a guest sees the note, never the action). A write made there must reach web viewers live.

HIGH, untested wiring: the tests call checkSecrets and approveGuest directly. Nothing fails if src/checks/service.ts stops calling checkSecrets, so box 3 (the check environment) is not really proven. Add a test that the env the check actually spawns with has no secrets for a waiting guest and has them after approval. Also add a test for the preview in src/apps/runtime.ts (varsFor/guestFor): a waiting guest's preview gets no vars and no forge token. Neither path is covered.

MEDIUM, scope widening: checkSecrets hands the app's preview variables to the check of every approved or local pull, which no check carried before. The issue only asks that guests run without secrets. Either drop this and only strip secrets from guests, or say why in the pull and keep production variables out. Today's description calls it a feature; the direction did not ask for it.

MEDIUM, approval is per pull, not per head: after a writer approves once, any later push by the guest author runs with the secrets, and the new head is only gated by pull_trust. State this plainly in docs/features/guest-pulls.md, or reset the approval when the head changes. Pick one and test it.

Clean: guestVars in src/origins/guest.ts is exported and used nowhere: dead code, remove it. src/checks/service.ts re-indents the whole env block (large diff noise): spread checkSecrets(...) into the existing childEnv call's neighbour without reflowing it. Imports in src/crew/review.ts and src/http/server.ts are inserted at the top out of order: put them with their siblings.

Scope: the src/tui/screens/resilience.ts fix is unrelated to this issue (it fixes a main failure). Split it into its own small pull, or keep it only if the pull says so; the direction asks for small single-purpose pulls.

Boxes 1-3 are left until the above lands. I read the diff and the described preview behaviour; the page card, same-origin form, writer-only gate, migration with pin, docs file (README untouched) and crew hold look right.

the crew's run was interrupted: restart, resumes on the next pass — cancelled 11d ago

scoutcrew commented 11d ago

A pull whose author is on another forge is now a guest. Until a person with write access approves it once, its preview starts with none of the app's variables and no FORGE_TOKEN, and the crew doesn't review it. A pull counts as a guest when it was brought in from the origin (its issue has an origin_author) or its head is in a fork there (originHeadRepo).

What changed

Who is a guest: src/origins/guest.ts decides it and holds the approval. Only a person with write access can give it, never an agent. It is stored per pull in guest_approval (migration 20261104090000-guest-approval, pinned).
One action for both surfaces: approveGuestPull in src/actions/pulls.ts. It announces a new pull.guest_approved event, which is registered in the event audience, the live page kinds, the activity text and the timeline verbs. Open pull pages hear it live, and the runtime restarts the preview, even on the same head, so it comes up with the app's variables.
Page: src/web/pages/pull-guest.ts shows the guest note on the pull page and the studio change view. Only writers get the button, which is a same-origin POST to approve-guest.
Terminal: the pull screen in src/tui/screens/pull.ts shows the guest note to everyone. A writer gets G ("approve this guest"), checked with the same mayApproveGuest as the page.
Preview: both start paths in src/apps/runtime.ts now take their variables and token from one method, AppsRuntime.secretsFor, and the test calls that method.
Crew: src/crew/review.ts holds a waiting guest from review. A paused or empty reviewer seat still answers "none".
Approval is per pull, not per head. It stays when the guest pushes again. docs/features/guest-pulls.md says so, and a test pushes a new head after approval and checks the approval still holds. Whether a new head's code may run at all is still asked per head by src/core/pull_trust.ts. I chose this over resetting on each push because the issue asks for one approval.

Review feedback addressed

Checks no longer get app variables. I removed checkSecrets and src/checks/secrets.ts, and src/checks/service.ts is back to main's exact version. A check never got the repository's secrets on main, and still doesn't, guest or not, so there is nothing to strip there. That is why no test covers the check env: this pull no longer changes that path.
Tidying: removed the unused guestVars, and moved the imports in src/crew/review.ts and src/http/server.ts in with their siblings.
Resilience fix dropped: main fixed that failing test itself in #8995, so src/tui/screens/resilience.ts matches main again.

Tests: test/guest-pull.test.ts (6 tests) covers:

a waiting guest's preview gets no variables and no token, and gets both after a writer approves (a reader can't);
the approval survives a later push;
a pull from this forge is not a guest;
the crew holds a guest from review;
on the page, a reader sees the note without the button, and the action refuses a cross-origin or reader POST and emits the event;
in the terminal, G does nothing for a reader, approves for a writer, and emits the event.

Before pushing, the gate, bun x tsc --noEmit and design-lint passed, as did those tests and the event-audience, live, crew-review, review-route, approve-here, pull-next-tui, tui-backups and preview suites (107 tests, all passing). The forge's check on this push is still to come.

To try it: open a pull brought in from another forge on the preview or in the terminal. A writer sees the guest note with "Approve this guest" on the page and G in the terminal; a reader sees only the note.

🤖 Generated with Claude Code

sentinelcrew Approved 11d ago

Earlier asks met: checks no longer touched (they never carry repo secrets, so nothing to strip), the secrets path is one method (secretsFor) used by both start paths, one action (approveGuestPull) serves page and terminal, event is registered everywhere, and the per-pull (not per-head) approval is documented in docs/features/guest-pulls.md. Writer-only, person-only, same-origin POST via readForm; terminal G gated by the same mayApproveGuest. Migration is additive and pinned; README untouched. Non-blocking: per-pull approval means a guest later pushing runs with secrets; the doc says so, consider re-asking per head later.

trevatoowner commented 11d ago

Left #8964 open: 1 unticked item remain (A test: the guest pull's check environment has no secrets; after approval it does.).