Repository navigation
MCP server params - #57
Merged
Merged
Conversation
…nd 2 A Forge at schema 4 reads its integrations blocks (spec 33), expands section markers (spec 11), and every craftar that does not substitute MCP server params refuses it at load (spec 34). All three gates spec 33 left—readsIntegrations, reservedKey, manifestWithSections—already use 'or above', so 4 inherits their behaviour with no code change.
Implements spec 34 §4.3 row U: a predicate that scans text for URLs with
credentials, letting through mail addresses, pkg@version, and the remedy
(a userinfo made only of ${NAME} references).
…tituted values Add src/core/mcp-server.ts, the one place a server is rendered or scanned for placeholders. Includes rendersServers, renderServer, serverCitations, credentialIn, message builders, and holdsBaseCitations.
Move serverSecret (and the private secretInServerField) from the importer to the new module. Import's behavior and messages do not change.
…escape The step's string held a U+00A0 between "com" and "x@y"; the committed file held a plain space (U+0020), so the row did not test what its label said. Write the character as \u00A0 so no tool can normalise it again.
…arams rendered At schema 4, mcpServers() renders each surviving MCP ingredient's server through renderServer, reports missing declared keys, warns for undeclared-but-valued keys, and checks substituted leaves for credentials before the plan returns.
…m a server cites from moving At schema 4 sync writes a server with its declared params rendered, so import must compare the workspace's server with that rendering or every templated server becomes a variant. A recipe default import's map cannot see (G1) keeps the workspace's bytes as a variant, and a key a server cites as declared is pinned on reuse and never set by inference (F9).
… profile At schema 4 a declared key in an MCP server is rendered, so a profile value written by take: param would move that server's bytes; cites() now counts it. An undeclared citation is never substituted and stays out, and below the gate nothing is read from a server.
…clares some, at schema 4
A variant import wrote holds the base's server with its braces and declares nothing; at
schema 4 it used to compare equal, resolve with zero decisions and be deleted, after which
sync rendered the value over the workspace's braces. It now differs in params and stays,
with a second warning line saying what discarding it does. rendersServers takes any
{ schema: number }, so callers that hold only the number need no cast.
…s unchanged at schema 4 Pins that the plan error reaches sync, status, diff, ls, explain, add/remove recipe, init, doctor, workspaces and forge impact with one line and never the value, and that the commands which read the Forge as written print at schema 4 what they print at 3.
…l check
Says what a schema 4 Forge renders into a server and what it leaves, what each of the three
citation cases makes sync do, and what the credential check catches, misses and refuses
wrongly — without claiming where a tool expands ${NAME}, which is not measured.
…t by a stack overflow A YAML alias can make a server cyclic. Below schema 4 that fails in the serializer; at 4 the two server walks recursed until the stack ended, before the named cyclic-alias refusal the fingerprint gives. Both walks now track their ancestors and throw that same sentence.
…messages; one CLI test per command (review) The review dropped the declared filter from decide() and no test failed: case 16 pins it. inferParams loses a parameter no MCP base can reach. The plan test compares whole messages, and the CLI tests that spawned ten processes in one test are one test per command.
…ial check really passes and refuses (review) Five older sentences said a server is written exactly as the Forge holds it or that only body files are citations; the unify and import rows and the two lists of the credential check said more, or less, than the code does.
…elp says schema 3 or above (review) The cyclic-alias refusal of the server walks named no ingredient, where every other message about a server starts with its ref and the schema 3 path names the file. The import help text said "a schema 3 Forge" for behaviour a schema 4 Forge has too. The env/args split of import's server scan gets the unit test the move had left it without.
…nd what doctor can show (review)
The command rows said "at schema 3" for gates that are 3 or above; unify refuses a plan
rather than silently not writing, and its second warning line is conditional; the
`${…}`-under-env sentence sat in the wrong list without its conditions.
…st's title (review) The drift show row and one Integrations sentence still said "schema 3" for a 3-or-above gate; the import row gave reuse precedence over the recipe-default case, which the code decides first; the credential lists said "a space" and "http(s)" where the code reads white space and four schemes. No src/ change.
…two wordings Spec 33's rows now say "in a schema 3 Forge"; with this branch's "or above" the phrase read wrong.
…aid without ambiguity (review) The confirmation round after the rebase: five sentences that could be read as the lock's schema or as "from schema 2".
Two sentences of the server params notes named the integrations release by number; it moved.
… a server's value urlCredentialIn read the authority up to the first slash, so https:///<userinfo>@host/… had none and passed the check that keeps a credential out of a generated MCP file; curl reads past the extra slashes and sends the userinfo. Every scheme but file is now read past the leading separators, as credentialFault reads a Forge URL.
…; its rows pinned (review) Review: a non-file URL written with an empty authority whose first path segment holds an @ is refused since the last commit — said in the README's list and pinned by two rows; the drift commands' refusal has its CLI row; the comment no longer compares with credentialFault, which does not read a Forge URL that way on this branch; and integrations are for a Forge at schema 3 or above in the one row that still said 3.
… its credential A tab, a newline or a space after :// stopped the reading of an empty authority, so https://<TAB>/<userinfo>@host/ passed; a URL parser drops the tab and reads the userinfo. The leading run now takes white space with the slashes, for every scheme but file. The README names what that refuses though it is no secret.
…redential A token written as a YAML block scalar ends in a line feed, so https://<token><LF>@host/ had no @ in its authority and passed, while a URL parser deletes tab, LF and CR and reads the token as the user. A run of those three directly before the @ no longer ends the authority; a space still does, and the README says what that leaves passing and what it newly refuses.
…ugh, and what Rule B refuses besides a mention (review) Review: "a tab or a line break inside the scheme" passes only a user-only token — with a password it is refused; the spelling that passes with one is a tab or a line break inside the :// itself. The line-break item now says a tab counts too, names the markdown and CSS shapes, and its scope off http(s) and ws(s). Eight rows pin it; one needless non-null assertion goes.
… paragraph's own phrase (review) Review: the previous commit left `;;` after dropping a non-null assertion; "outside http(s) and ws(s)" left out the <x>+http family, where an @ alone is enough; and only an unquoted CSS url(…) is refused.
The placeholder the branch carried until its number was known: main is at 0.27.2 and this is a minor.
…, and that this is not the forge: check (review) Final review on the new base: 0.27.1 and 0.27.2 read a schema 3 Forge too; the invalid-manifest message gained an alternative, as 0.27.0's did; and the server check and the forge: check read a URL by different rules.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
npmenvironmentschema: 4(which includes 3 and 2). At schema 4 anmcpingredient'sserveris rendered with its declared params —{{key}}in a string leaf, for a key the ingredient declares and a layer gives a value to — before it is written to.mcp.json/.kiro/settings/mcp.json; an undeclared{{…}}is left as written and warned about; a declared key with no value refuses the sync like any other unset declared param.url-credentials) or a token of a known format fails the plan on every surface (sync,status,diff,ls,explain, thedriftcommands,add recipe/remove recipe,init,doctor,workspaces,forge impact), naming the ingredient, the field and the kind — never the value. A token-shaped value underenv/argswarns.doctor'smcp-envreads the rendered server;importcompares an MCP base assyncrenders it;forge unifydoes not write into a profile a param a server cites as declared.updatefrom upgrading alone; moving a Forge to 4 is what renders its placeholders.How a URL is read inside a server value — three rulings taken while this branch was reviewed, all in
urlCredentialInand the README's two lists (still passes, though it is a secret / refused, though it is not a secret):/or\after://, for every scheme butfile(https:///user:pw@host/is a finding);@does not end the authority (a token from a YAML block scalar ends in a line feed). A space before the@still ends it — a token followed by a space still passes, by decision.sqlite:///a:b@c.db; a URL with no path followed by a line that starts with@(a registry, then@scope/pkg; a markdown link or an unquoted CSSurl(…), then a mention or@media).Known and open:
://itself (https:<TAB>//user:pw@host/) still passes — the check looks for a literal://. Named in the README and pinned by a test as passing; whether to close it is an open decision.forge:URL (credentialFault, 0.27.1): the two read a URL by different rules, and the README says so.Notes on the history (never amended):
31d3af0and7a75f95are typeddocs(readme)but also carry a source comment or a one-token change and test rows;7a75f95left a stray;;thatd4360e4removes. Their bodies say what they touch.6531137compares withcredentialFault; that comparison became true only with 0.27.1, now inmain.Test plan
npm run typecheck— pass (on the bumped HEAD03279ae)npm run build— passnpx vitest run --exclude test/ci.test.ts --maxWorkers=6— 100 files passed, 1 skipped; 2724 tests passed, 8 skipped (Linux). Oracle skipped (no fixture): byte fidelity below schema 4 rests on the gate being read in the code and on the unit, golden and CLI suites; schema 4 bytes rest on those suites alone.test/ci.test.tsis left to CI.node bin/craftar.js --versionprints0.28.0test/server-params-cli.test.ts(thedrift showrows fail if the refusal goes: without the token the same commands exit 0)urlCredentialIn— every row of the two lists pinned intest/url-credential.test.ts; the rows of the three rulings were each shown failing before their fix (9, 8 and 7 failing tests)mainat 0.27.2 with the branch's net change unchanged (same added and removed lines on the old and the new base)releaserun only when ready to publishcraftar@0.28.0