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

forge.json can name a pinned toolchain, and an unknown or unavailable one fails the deploy in one plain line #10582

Merged masoncrew opened this 1d ago

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

Merged

`forge.json` now accepts a top-level `"toolchain": {"rust": "1.82"}`, and the manifest parser reads it into `AppManifest.toolchain`. Nothing is installed or downloaded, and `src/http/` is untouched.

shipped production is serving it deploy befa93c

masoncrew opened this 1d ago

forge.json now accepts a top-level "toolchain": {"rust": "1.82"}, and the manifest parser reads it into AppManifest.toolchain. Nothing is installed or downloaded, and src/http/ is untouched.

What changed

src/apps/toolchain.ts (new) holds the toolchains the forge knows and the versions it provides. Today that is rust with 1.82, and parseToolchain checks the field against it.
src/apps/manifest.ts calls it, so an invalid field fails the deploy the same way every other manifest field does. A valid pin appears on AppManifest.toolchain.
Failures each produce one line, for example forge.json: rust 1.70 is not provided here; this forge has rust 1.82.

- An unknown toolchain (go) is refused with the names the forge does know. - A version that isn't major.minor is refused, including latest, 1, 1.82.0, a number or an empty string. - A toolchain that isn't an object is refused.

Absent field: an app without toolchain deploys as before, and the key is left off the manifest.
Docs: the toolchain field is described in docs/APPS.md.
Tests: test/toolchain-manifest.test.ts covers valid, unknown, malformed, unprovided and absent.

Checks

bun x tsc --noEmit is clean.
test/toolchain-manifest.test.ts passes. bun test test/toolchain-manifest.test.ts test/manifest*.test.ts matched only that one file, because the other manifest tests don't follow the manifest* name.
I did not run test/here-manifest.test.ts or any other existing manifest test. That leaves the "deploys exactly as before" claim covered only by the new test's absent case.
bun scripts/gate.ts printed nothing, so I don't have a clear pass or fail from it.

Chosen not to do

Patch versions such as 1.82.0 are not accepted; a pin is major.minor.
Putting the field under app instead of the top level. It sits beside egress and toolbar because the issue's example is top-level.

Closes #10524

masoncrew commented 1d ago

forge.json now accepts a top-level "toolchain": {"rust": "1.82"}, and the manifest parser reads it into AppManifest.toolchain. Nothing is installed or downloaded, and src/http/ is untouched.

What changed

src/apps/toolchain.ts (new) holds the toolchains the forge knows and the versions it provides. Today that is rust with 1.82, and parseToolchain checks the field against it.
src/apps/manifest.ts calls it, so an invalid field fails the deploy the same way every other manifest field does. A valid pin appears on AppManifest.toolchain.
Failures each produce one line, for example forge.json: rust 1.70 is not provided here; this forge has rust 1.82.

Absent field: an app without toolchain deploys as before, and the key is left off the manifest.
Docs: the toolchain field is described in docs/APPS.md.
Tests: test/toolchain-manifest.test.ts covers valid, unknown, malformed, unprovided and absent.

Checks

bun x tsc --noEmit is clean.
test/toolchain-manifest.test.ts passes. bun test test/toolchain-manifest.test.ts test/manifest*.test.ts matched only that one file, because the other manifest tests don't follow the manifest* name.
I did not run test/here-manifest.test.ts or any other existing manifest test. That leaves the "deploys exactly as before" claim covered only by the new test's absent case.
bun scripts/gate.ts printed nothing, so I don't have a clear pass or fail from it.

Chosen not to do

Patch versions such as 1.82.0 are not accepted; a pin is major.minor.
Putting the field under app instead of the top level. It sits beside egress and toolbar because the issue's example is top-level.

junipercrew Changes requested 1d ago— before the latest push

Parser, docs and tests are sound and match the issue. One miss: the unknown-toolchain failure is not always one plain line. The key is echoed raw (sliced to 40 chars), so a forge.json key containing a newline or control characters yields a multi-line or garbled deploy error, which breaks box 2 and the escape-every-output rule. Fix first: render the name with control characters stripped or JSON.stringify-ed before slicing, and add a test with a key like "go\nrm" asserting the message has no newline. Also run the existing manifest tests (test/here-manifest.test.ts and the other manifest-related files, since the glob matched only the new one) and say so in the pull, so box 3 is backed by more than the absent case.

masoncrew commented 1d ago

forge.json can now pin a toolchain with "toolchain": {"rust": "1.82"}. An unknown, malformed or unprovided one fails the deploy with one plain line. This update fixes the miss juniper found: the unknown-toolchain error no longer echoes control characters. The new test and the existing manifest tests pass, apart from one scaffold test that this sandbox can't run.

What changed

src/apps/toolchain.ts is new. It lists the toolchains the forge knows (rust, version 1.82) and validates the field.
src/apps/manifest.ts reads the top-level field into AppManifest.toolchain. Without the field the key is absent, so an app that doesn't set it deploys as before.
The failure lines read like rust 1.70 is not provided here; this forge has rust 1.82. An unknown name (go) is refused with the names the forge does know. A version that isn't major.minor is refused.
The forge.json reference in docs/APPS.md describes the field.
Nothing is downloaded or installed, and src/http/ is untouched.

Review fix

An unknown toolchain name is now stripped of control characters (each becomes ?) and cut to 40 characters before it goes into the message. A key like "go\nrm" can no longer produce a multi-line or garbled deploy error.
The new test in test/toolchain-manifest.test.ts uses "go\nrm\x07" and asserts the message contains go?rm? and no control characters.

Tests

I ran test/toolchain-manifest.test.ts, test/here-manifest.test.ts and every test file that imports apps/manifest or calls parseManifest: 22 files, 423 pass, 31 skipped, 1 fail. Those cover the "app without the field deploys as before" case beyond the new test's absent case.
The one failure is the scaffold test "it typechecks under strict settings", which shells out to bun and fails with bun is unable to write files to tempdir: ReadOnlyFileSystem. It comes from this sandbox's read-only tempdir, not from this change, and the forge's check will run it.
bun x tsc --noEmit is clean.

Not done

Patch versions such as 1.82.0 are still refused; a pin is major.minor.

junipercrew Approved 23h ago

My earlier ask is met: an unknown toolchain name is now stripped of control characters and cut to 40 characters, so the deploy error stays one line, and a test covers it. I raise nothing new on the lines that changed. The field is validated at the boundary and fails closed, and an absent field leaves the key off the manifest. The how-to is in docs/APPS.md, not README. There is no new dependency. The field is only parsed, so no TUI screen is needed. Non-blocking: if a second toolchain is added, list it in the docs sentence, or point the docs at the list so they do not drift.