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

A setting's out-of-range refusal and an app's members refusal live in one module, and the API and MCP read them from there #5577

Merged masoncrew opened this 20d ago

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

Merged

Two refusal sentences now live in one module, and the API and MCP doors both read them from it: - "that value is outside what this setting takes" - "the people an app names are its writers' to choose"

shipped production is serving it deploy eddfe7b

masoncrew opened this 20d ago

Two refusal sentences now live in one module, and the API and MCP doors both read them from it:

"that value is outside what this setting takes"
"the people an app names are its writers' to choose"

The new module is src/actions/setting_words.ts, which exports both as a const object, SETTING_WORDS (outOfRange and appMembersForbidden).

What moved

src/http/api.ts: the members refusal in membersAnswer and the capacity PUT refusal now use SETTING_WORDS.
src/http/mcp.ts: the app_members tool's forbidden answer and the capacity tool's out-of-range answer now use SETTING_WORDS. Each edit touches only the literal's own line, plus one import line next to the app_members import, so it should merge cleanly with parallel MCP work.
limits is not touched. Neither sentence appears there: its out-of-range answer passes on changed.error.message from the action, so there was nothing to move.

The wording is unchanged. A search of src/ now finds each sentence only in the module.

Test test/setting-words.test.ts triggers each refusal through each door:

Out of range: PUT /api/v1/admin/capacity/checks.concurrency with value: "0" returns 422, and the MCP capacity tool with dryRun: false refuses the same value.
Members: a non-writer with an admin-scope token asks for GET /api/v1/repos/alice/garden/app/members and gets 403, and the MCP app_members tool refuses them too.

In every case the message must match the module's sentence. A third test scans src/*/.ts and fails if any file other than the module types either sentence. That catches a door that copies the words back in unchanged, which the per-door checks alone would not.

Checked before pushing: test/setting-words.test.ts, test/capacity.test.ts, test/app-members.test.ts and test/limits.test.ts all pass unchanged (28 tests), and bun x tsc --noEmit is clean. I did not run the full suite; the forge's check does that on this push.

🤖 Generated with Claude Code

Closes #5563

masoncrew commented 20d ago

Two refusal sentences now live in one module, and the API and MCP doors both read them from it:

"that value is outside what this setting takes"
"the people an app names are its writers' to choose"

The new module is src/actions/setting_words.ts, which exports both as a const object, SETTING_WORDS (outOfRange and appMembersForbidden).

What moved

src/http/api.ts: the members refusal in membersAnswer and the capacity PUT refusal now use SETTING_WORDS.
src/http/mcp.ts: the app_members tool's forbidden answer and the capacity tool's out-of-range answer now use SETTING_WORDS. Each edit touches only the literal's own line, plus one import line next to the app_members import, so it should merge cleanly with parallel MCP work.
limits is not touched. Neither sentence appears there: its out-of-range answer passes on changed.error.message from the action, so there was nothing to move.

The wording is unchanged. A search of src/ now finds each sentence only in the module.

Test test/setting-words.test.ts triggers each refusal through each door:

Out of range: PUT /api/v1/admin/capacity/checks.concurrency with value: "0" returns 422, and the MCP capacity tool with dryRun: false refuses the same value.
Members: a non-writer with an admin-scope token asks for GET /api/v1/repos/alice/garden/app/members and gets 403, and the MCP app_members tool refuses them too.

🤖 Generated with Claude Code

sentinelcrew Approved 20d ago— before the latest push

Clean, behaviour-preserving refactor that does what #5563 asked. Both sentences now live in src/actions/setting_words.ts. The four places that typed them in src/http/api.ts and src/http/mcp.ts (the membersAnswer forbidden case, the capacity PUT, MCP app_members and MCP capacity) now read them from that module, and the words are unchanged byte for byte. limits really does pass on the action's message, so leaving it alone is right. No security surface changes: no new input, no new route, and no change to who is checked or how. The new test reaches each refusal through both doors, using a real admin-scope non-writer and an out-of-range value. Its scan of src/ catches a door that types the words in again, so it would fail if they drifted. On the preview, / answers 200; the capacity PUT and app-members reads refuse an anonymous caller (404 and 401), as on main.

Not blocking: src/web/pages/capacity.ts:233 and src/contract/openapi.ts:257 still hold capitalized or rephrased versions of the out-of-range sentence. The issue scoped this to the API and MCP doors, and the scan is case-sensitive, so it does not flag them. If the page should read the module too, give that its own small issue.

masoncrew commented 19d ago

A setting's out-of-range refusal and an app's members refusal are now written once, in src/actions/setting_words.ts. The REST API (src/http/api.ts) and the MCP tools (src/http/mcp.ts) both read them from there instead of each typing its own copy. Behaviour doesn't change: each door gives the same sentence it gave before.

test/setting-words.test.ts checks that each door answers with the module's sentence. It also checks that no other file under src/ types that sentence out again, so a new copy fails the test.

This includes a merge of main. The only conflict was the import block in src/http/api.ts: main added SPEND_WORDS from src/actions/spend_words.ts on the same line where this branch adds SETTING_WORDS. Both imports are kept.

After the merge, the type check passes. The setting, spend and member tests also pass: 18 files, 0 failures, 1 skipped.

To try it, ask the API or MCP to set a setting outside its allowed range: both answer with the sentence from setting_words.ts.

🤖 Generated with Claude Code

sentinelcrew Approved 19d ago— before the latest push

Clean, and scoped to the issue (#5563). I read every changed line; no security surface changes. Both refusals move into src/actions/setting_words.ts as plain data, word for word, and api.ts and mcp.ts read them from there. Authorization stays where it was: the forbidden branch in membersAnswer and the app_members tool still come from the action's result, and the capacity tool still requires an admin-scope token before changeCapacity runs.

At head 7069ef99 I ran test/setting-words.test.ts, test/capacity.test.ts and test/app-members.test.ts: 23 pass, 0 fail. A grep of src/ finds each sentence only in the module. The other matches are the Capacity page's sentence-cased invalid_value and the OpenAPI description. Both are different strings, and the issue scoped the change to the API and MCP doors, so leaving them out is right.

The preview answers / with 200. Its unauthenticated capacity PUT and app-members GET still refuse as before (not_found and unauthorized), so the refusals did not reorder.

No page or tty change is involved, because this pull only moves words. The source-scan test is the right guard against a door retyping the sentence.

trevatoowner commented 19d 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.

masoncrew commented 19d ago

The out-of-range refusal for a capacity setting and the refusal for an app's members now come from one module, src/actions/setting_words.ts, which holds them as plain data. The API and the MCP tools read them from there and no longer type the sentences out themselves. The words stay exactly the same.

This update merges main, where #5932 split src/http/mcp.ts into one file per tool:

The pull's change is now in the new per-tool files: src/http/mcp/tools/app_members.ts and src/http/mcp/tools/capacity.ts read SETTING_WORDS. mcp.ts is main's version unchanged.
In src/http/api.ts, the SETTING_WORDS import sits next to main's new ADMIN_WORDS import.
No MCP tool is added and no tool's interface changes, so the generated docs did not need regenerating.

The type check passes. test/setting-words.test.ts, test/capacity.test.ts and test/app-members.test.ts ran 22 pass, 1 fail. The failure is in test/capacity.test.ts ("reported: the Capacity page says it once, as a note; vitals do not"). It expected no admin notes and got "Disk is nearly full: 9% free.", which reflects this machine's disk; this pull changes no capacity behaviour. The test that checks no other file under src/ types the sentences still guards the new per-tool layout.

Left alone on purpose: the Capacity page's sentence-cased invalid_value and the OpenAPI description. They are different strings and the issue covers only the API and MCP.

🤖 Generated with Claude Code

sentinelcrew Approved 19d ago

Clean. This is a refactor with no change in behaviour: two refusal sentences move into src/actions/setting_words.ts (plain data), and api.ts plus the two per-tool MCP files read them from there. The wording is identical, the status codes are unchanged, and no auth path is touched. Both refusals still fire after the identity and scope checks, as they did before.

Checked at head 2a7c187: bun test ./test/setting-words.test.ts ./test/app-members.test.ts gives 10 pass, 0 fail, and bun x tsc --noEmit is clean. The new test hits each door (API and MCP) for each sentence, and the src/** scan fails if a door types the words back in, so it would catch a regression. The preview answers / with 200. Anonymous GET …/app/members gets 401 and anonymous PUT /api/v1/admin/capacity/… gets 404, so the doors stay gated.

No security findings: no new input, no output, no SQL or shell, no dependency. Leaving the page's sentence-cased invalid_value and the OpenAPI text alone is in scope as the issue states it. The diff is small and single-purpose, and it touches the per-tool MCP files rather than mcp.ts, which fits the direction.