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

A test pins forking when the copy fails: `fork_failed`, with no repository row or directory left behind #7179

Merged opened this

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

Merged

A failed fork copy now has a test, and the test found the answer was wrong. `POST /api/v1/repos/:owner/:name/forks` replied 422 `{"error":"fork_failed"}` with no message. I fixed `src/http/api/upstream.ts` so it also carries "the fork could not be created", the same words the terminal already uses.

shipped production is serving it deploy 5f89d5b

opened this

A failed fork copy now has a test, and the test found the answer was wrong. POST /api/v1/repos/:owner/:name/forks replied 422 {"error":"fork_failed"} with no message. I fixed src/http/api/upstream.ts so it also carries "the fork could not be created", the same words the terminal already uses.

The new test/fork-failed.test.ts makes the repos root read-only and forks as a second user. It checks three things:

  • Refusal: the answer is a 422 fork_failed with that message, not a 500.
  • Nothing left behind: no repository row exists for the fork, and the repos root lists the same directories as before.
  • Retry: once the root is writable, the same fork request succeeds with 201 under the same name.

The new test passes and bun x tsc --noEmit is clean. I did not run the rest of the suite.

The test makes the root unwritable with chmod. It would not fail for a process running as root, because root ignores the permission. It did fail here as an ordinary user, which is how the forge runs it.

Closes #7175

commented

A failed fork copy now has a test, and the test found the answer was wrong. POST /api/v1/repos/:owner/:name/forks replied 422 {"error":"fork_failed"} with no message. I fixed src/http/api/upstream.ts so it also carries "the fork could not be created", the same words the terminal already uses.

The new test/fork-failed.test.ts makes the repos root read-only and forks as a second user. It checks three things:

  • Refusal: the answer is a 422 fork_failed with that message, not a 500.
  • Nothing left behind: no repository row exists for the fork, and the repos root lists the same directories as before.
  • Retry: once the root is writable, the same fork request succeeds with 201 under the same name.

The new test passes and bun x tsc --noEmit is clean. I did not run the rest of the suite.

The test makes the root unwritable with chmod. It would not fail for a process running as root, because root ignores the permission. It did fail here as an ordinary user, which is how the forge runs it.

Approved

Approved. It does what #7175 asks. A fork whose copy fails now answers 422 fork_failed with "the fork could not be created", the same words the terminal uses (src/tui/screens/common.ts:867). Before this change the API sent the code with no message, so the message assertion fails without the fix. The test also checks that no repository row is left and that the repos root lists the same directories, and a second try forks under the same name.

Security: the only source change is a constant error string. Identity, scope (requireScope rw) and body handling are unchanged, and nothing user-supplied reaches the output or SQL. On the preview, / answers 200 and an unauthenticated POST to /forks gets 401.

Two notes, neither blocks:

  • forkBareRepo (src/git/repos.ts:66) does not remove forkDir when clone succeeds but the git config step fails. That partial case would leave a directory behind, and every later fork under that name would fail. The test's read-only root covers only the case where no directory can be made, which is all the issue asked for. Removing the directory on that path is a follow-up worth its own issue.
  • The test depends on chmod 0555 being enforced. Under a root process the first test would fail with a 201, not pass silently. The forge runs as an ordinary user, so it is fine here.
Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs