feat: accept an org parameter on the execute tools

[AlexKantor87]

Closes #8.

Reaching a second org meant restarting the server with a different KOSLI_ORG.
A per-call override already existed, buried inside the free-form params
record. Nothing in the tool schema told the model it was there.

org is now a first-class optional input on execute_read_action and
execute_write_action. It applies to that call only. Omit it and you get
KOSLI_ORG as before, and params.org still works. As a top-level input, the
target org also shows up in the client's write-approval prompt. Before, it sat
inside a params blob.

No new tool. Still three, per CLAUDE.md.

One function decides the target org (normalizeOrg plus orgError). Every way
of naming one obeys the same rules. The org is trimmed, null counts as not
supplied, and the value that was checked is the value that goes out. Three
things are now rejected rather than guessed at:

• a blank org, which used to produce a URL with the org segment missing
• an org aimed at one of the four actions that are not org-scoped, which used to
  ride along as a stray query parameter
• two different orgs named in one call

That last one needs params.org read before unwrapBodyParam and the body's
org after it. A write's request body is flattened over the top level, so reading
once let the spread pick a winner. The write then went to an org nobody
approved.

Verified against the live API through the stdio entry point, not just the
function. Covered: the default org, cyber-dojo, kosli-public, an unreachable
org (403), and each rejection path. 88 tests.

Not in scope: loadConfig never trims KOSLI_ORG, so a padded env var still
produces a padded path segment. That is pre-existing and on a separate channel.
Happy to follow up.

🤖 Generated with Claude Code

[claude]

Claude finished @AlexKantor87's task in 4m 26s —— View job

---

Review: feat: accept an org parameter on the execute tools

• Read CLAUDE.md and the full diff
• Check the latest commits against the prior review rounds
• Review source changes (execute-action.ts, config.ts, index.ts)
• Review tests and docs
• Build + test (couldn't run locally — see note)
• Post review

Architectural conformance

Invariant	Status
Exactly three MCP tools	✅ no tool added
src/catalog.json not hand-edited	✅ untouched
KosliClient returns the error shape, doesn't throw	✅ preserved; org rejections use the same { error: true, message } shape as the existing Unknown action / wrong-mode errors
Compact JSON.stringify responses	✅ unchanged
org falls back to config.org in buildUrl	✅ preserved — executeAction deletes the key when nothing is named, so the fallback still fires
User-Agent header	✅ unchanged
ESM .js import extensions, strict: true	✅
No HTTP client dependency, fetchFn injected in tests	✅ every new test injects a mock
readOnlyHint / destructiveHint split	✅ unchanged; nothing crosses the boundary
Conventional Commits	✅ all five commits and the PR title
manifest.json version placeholder	✅ not touched

Findings

Two inline comments, both non-blocking:

1. src/index.ts:18 — the PR's headline benefit is undercut by a file it doesn't touch. searchActions returns entry.parameters verbatim, which advertises org as a required path parameter on all but four actions. The documented flow (search, then execute with what you were given) therefore steers the model back into params.org — the exact case where the target org is not visible as a top-level input in the write-approval prompt. Suggested a sentence in the shared description; the searchActions side is a follow-up.
2. test/catalog.test.ts:26 — the new "exactly four" guard classifies by parameters, same as orgError, so the two agree by construction. Neither looks at the path: a {org} in a path that doesn't declare the parameter would be classified non-org-scoped and ship the literal placeholder in the URL, with the suite still green. Can't happen today (I checked all four paths); one assertion closes the gap the test's own name claims to cover.

Two nits not worth inline comments:

• The non-org-scoped rejection message ends "…in params.org, or in the request body", but all four non-org-scoped actions are bodyless GETs (requestBody: null), so that clause can never apply when the message fires. Harmless, slightly confusing to the reader it's aimed at.
• has exactly the four documented actions that take no org will now turn the weekly chore/update-catalog bot PR red the first time Kosli ships a non-org-scoped endpoint. That's the intent, and the test comment explains the fix — but CLAUDE.md's update-catalog.yml bullet doesn't mention it, and whoever triages that PR reads CLAUDE.md, not the test body.

Worth calling out

• normalizeOrg requiring a string rather than coercing is the right resolution of the last round. 1234 was the dangerous case: the org path param carries no pattern in the catalog, so a coerced org id would have been a successful write into a real organization nobody named, not a 403. Rejecting instead of coercing removes that outcome entirely.
• Reading params.org before unwrapBodyParam and the body's org after it is a genuinely non-obvious ordering, and the comment explains why rather than what. This is the load-bearing line of the diff and it's the one most likely to be "tidied" away later.
• The checked value is the value that goes out (withOrg.org = named[0]) closes the gap between what an approval prompt shows and what buildUrl sees. Easy to get wrong by re-reading params.org at the end.
• An unnoticed bug fix: on main, { org: null } on a non-org-scoped action reached extraParams and — since the query filter drops only undefined — went out as ?org=null. Deleting the key fixes it, and the test pins it.
• bodyFields is unusually good for a test helper: it walks composition and dependency keywords, has direct tests for every branch it handles, documents the not and patternProperties limits, and explains why the walk is tested even though today's catalog doesn't exercise it. That last note is what stops a future simplification from quietly reverting the guard.
• 26 new behaviour tests across the org channels, each injecting fetchFn, plus the get_user_default_org addition to the fixture rather than loading the full catalog — exactly what CLAUDE.md asks for.

Note on verification

npm ci, npm test, and npm run build all required approval in this environment, so I reviewed statically rather than running the suite — the CI job on this PR is the authority on green. What I did verify by reading: list_envs exists in the catalog (the README example is valid), the four non-org-scoped action IDs and paths match both the test and the README's "the two /schemas/… actions", and body sits in requestBody rather than parameters for post_control/put_policy in the fixture, so unwrapBodyParam does proceed for both and the multipart tests exercise the branch they claim to. If you want me to run the suite, add Bash(npm test) to the allowed tools.

Nothing here blocks merge.
· feat/per-call-org

[claude]

This guard is the right instinct, but it has a gap the code below it depends on. executeAction also reads unwrapped.org — i.e. an org property inside a write's request body — and treats it as authoritative: it becomes the path segment and is stripped from the body.

Today that's always safe, because no schema in src/catalog.json declares a property named org (I checked — zero matches for "org": {), and the write bodies carry additionalProperties: false. But if a future npm run generate-catalog adds a body field that happens to be called org and means something else, it would be silently hijacked into the URL and dropped from the payload — with no test failing.

Worth extending the same guard to cover it:

Suggested change

	// buildUrl only fills `{org}` from a path parameter; an org arriving any
	// other way would bypass the checks in executeAction.
	it("never declares org as a query or header parameter", () => {
	const offenders = catalog
	.filter((entry) => entry.parameters.some((p) => p.name === "org" && p.in !== "path"))
	.map((entry) => entry.id);
	expect(offenders).toEqual([]);
	});
	// buildUrl only fills `{org}` from a path parameter; an org arriving any
	// other way would bypass the checks in executeAction.
	it("never declares org as a query or header parameter", () => {
	const offenders = catalog
	.filter((entry) => entry.parameters.some((p) => p.name === "org" && p.in !== "path"))
	.map((entry) => entry.id);
	expect(offenders).toEqual([]);
	});
	// executeAction treats an `org` inside a request body as the target org:
	// it moves to the path segment and is stripped from the payload. A body
	// field genuinely called "org" would be hijacked, so assert none exists.
	it("never declares org as a request body property", () => {
	const offenders = catalog
	.filter((entry) =>
	entry.requestBody?.some((b) => b.schema?.properties && "org" in b.schema.properties),
	)
	.map((entry) => entry.id);
	expect(offenders).toEqual([]);
	});
	});

[AlexKantor87]

Taking this. The body-property case is the one channel the guard missed, and executeAction does treat it as authoritative. Adding it alongside the query/header assertion.

Also folding in the loadConfig trim from your summary. KOSLI_ORG=" " being truthy is a better argument than the one I used to defer it.

[claude]

The read-before/read-after ordering is correct and the reasoning in the comment is exactly right — the spread really would pick a winner and hide the disagreement. Worth keeping.

One thing to consider, though, about treating the body as a legitimate third channel rather than just a thing to cross-check. When the body is the only place naming an org, named has one element, orgError is satisfied, and the write is retargeted to it — no top-level org, no params.org.

That's identical to pre-PR behaviour (unwrapBodyParam already spread it over the top level), so it's not a regression. But it sits slightly against the PR's stated goal: the target org is surfaced in the approval prompt because it's a top-level input, and this is precisely the case where it isn't. The user still sees it, just nested inside the params blob.

Since no catalog write body declares an org property (and they're additionalProperties: false), a body-only org is always spurious — so rejecting it, or at minimum not letting it silently decide the target, would close the loop rather than preserving the one path the PR set out to make visible. Your call; the current behaviour is defensible and documented. Just flagging that the strongest argument in the PR body ("the target is never guessed") applies here too.

[AlexKantor87]

Declining. unwrapBodyParam exists to treat body keys as if they had been sent as top-level params. Under that contract a body-nested org is params.org. That is a supported channel, so rejecting it would contradict the function it just passed through.

The dangerous case is the one this PR closes: a body org disagreeing with a named one. A body-only org still goes through the trim, blank and non-org-scoped checks. The guard added above means a body field called org cannot arrive without a test failing.

The class here is "a channel that decides the target without being checked". So I searched every place an org value is read:

search: grep -rnE "(params|unwrapped|withOrg|config).org|normalizeOrg(|KOSLI_ORG" src/
client/kosli-client.ts:131 params.org ?? this.config.org (only reader that fills the path segment)
tools/execute-action.ts:135 normalizeOrg(params.org) (pre-unwrap)
tools/execute-action.ts:138 normalizeOrg(org), normalizeOrg(unwrapped.org)
tools/execute-action.ts:150 delete withOrg.org / withOrg.org = named[0]
config.ts process.env.KOSLI_ORG (the default; now trimmed)
index.ts config.org (description strings only)

Four channels reach line 131. All four pass through normalizeOrg first.

mutation: add an org property to allow_artifact_for_env in src/catalog.json -> "never declares org as a request body property" red

[claude]

Minor maintainability nit, pre-existing but worsened here: executeAction is now eight positional parameters, and both call sites in src/index.ts have to pass a literal undefined for fetchFn to reach past it:

await executeAction(entries, config, actionId, params, fields, undefined, "GET", org);

mode and org are now adjacent and both string-ish. mode is a narrow union so swapping them is caught, but passing a ToolMode value into the org slot compiles fine. An options object for the optional tail would make the call sites self-describing:

executeAction(entries, config, actionId, params, { fields, mode: "GET", org })

Not blocking, and out of scope if you'd rather keep the diff tight — but this is the second optional parameter added to the tail, so it's likely to come up again.

[AlexKantor87]

Agreed on the smell, declining for this PR. Reworking the signature touches both call sites and every test that reaches past fetchFn. That belongs in its own diff, where the churn is the change rather than noise around a behaviour fix.

Raising it as a follow-up issue.

no mutation: this reply adds no test claim.

[AlexKantor87]

Raised as #64, along with the same blind spot in unwrapBodyParam own body-field check.

no mutation: this reply adds no test claim.

[claude]

String(value) on an unknown is the one hole left in "two different orgs in one call are rejected".

org as a tool input is z.string(), but params is z.record(z.string(), z.unknown()), so params.org — and a body org after the unwrap — can be any JSON the model emits. An array collapses to a single string before the dedupe ever sees it:

params: { org: ["cyber-dojo", "kosli-public"] }
  → normalizeOrg → "cyber-dojo,kosli-public"
  → named = ["cyber-dojo,kosli-public"]   // length 1, no conflict
  → GET /api/v2/environments/cyber-dojo%2Ckosli-public → 403

That's exactly the shape a model reaches for when asked to list envs across two orgs, and it gets a 403 on a nonsense org rather than the "supply just one" message the PR added for it. {} behaves the same way via "[object Object]".

The outcome is safe — no write lands in an unintended org, because no org has a comma in its name — so this is a diagnostics gap, not a correctness one. But normalizeOrg is the function whose stated contract is that the target is never guessed, and stringifying a container is a guess. Cheapest fix reuses the rejection path already there:

Suggested change

	function normalizeOrg(value: unknown): string | undefined {
	if (value === undefined || value === null) return undefined;
	return String(value).trim();
	}
	function normalizeOrg(value: unknown): string | undefined {
	if (value === undefined || value === null) return undefined;
	// Only a primitive can name an org. An array or object would stringify into
	// a single plausible-looking value ("a,b", "[object Object]") and pass the
	// conflict check below as one org, so collapse it and let orgError reject it.
	if (typeof value === "object") return "";
	return String(value).trim();
	}

Worth widening the blank-org message slightly if you take it — "An empty org was given" reads oddly for ["a","b"]. Something like Name a single organization as a string, or omit the org parameter to use the configured default. covers both.

[AlexKantor87]

Taken, and widened. Asking "is it an object" still coerces a number, and that case is worse. The org path parameter has no pattern in the catalog, so an org named 1234 is possible. A model holding an org id would write into a real organization nobody named. normalizeOrg now requires a string.

Two follow-ons, in e275976. The "" marker reached the disagreement branch, so a container next to a valid org reported a conflict against a nameless org. Both messages also said "the org parameter", which here means the tool input.

verified: sed -n 91,105p src/tools/execute-action.ts
function orgError(entry: CatalogEntry, named: string[]): string | undefined {
const takesOrg = entry.parameters.some((p) => p.name === "org" && p.in === "path");
if (!takesOrg) {
if (named.length === 0) return undefined;
return Action "${entry.id}" is not organization-scoped. It takes no org. Retry with no org in the org parameter, in params.org, or in the request body.;
}

// Before any disagreement: an unusable value is the thing to report, and
// reporting it as a nameless org disagreeing with a real one would send the
// caller to drop one of the two rather than to fix the bad value.
if (named.includes("")) {
  return "The org must be a single non-empty organization name. Check the org parameter, params.org, and any org in the request body, or omit all of them to use the configured default.";
}

if (named.length > 1) {

search: grep -rnE "String(|Number(|Boolean(" src/ --include=*.ts
client/kosli-client.ts:51,57 String(item) / String(value) (multipart form fields)
client/kosli-client.ts:109 String(v) (query values)
client/kosli-client.ts:146 encodeURIComponent(String(value)) (path params other than org)

Same coercion, other params. No other path param picks the organization, so a wrong one fails rather than succeeding somewhere unintended. Left alone.

mutation: normalizeOrg returning String(value).trim() -> five "rejects ... rather than coercing it into the path" cases red; moving the unusable check after the conflict check -> "reports the unusable value, not a disagreement" red

[claude]

Requiring a string is the right call, and the "" sentinel keeps the two rejections on one path. One diagnostics note, in the same spirit as the last round.

normalizeOrg now folds two distinct mistakes into the same marker, so they get the same message:

input	message says
org: " "	"must be a single non-empty organization name" ✓
org: 1234	"must be a single non-empty organization name" ✗
org: ["a","b"]	same ✗

1234 is the likely one — a model that has just read an org id off a previous response, which the comment on line 70 calls out by name. It is told the value is empty when it isn't, and the fix it needs ("name the org, not its id" / "one org, as a string") isn't in the sentence. "name" carries the whole hint.

The split is cheap because the two are already distinguishable before the merge — keep the sentinel, just record which kind:

Suggested change

	return "The org must be a single non-empty organization name. Check the org parameter, params.org, and any org in the request body, or omit all of them to use the configured default.";
	if (named.includes("")) {
	return "The org must be a single organization name, given as a non-empty string. Check the org parameter, params.org, and any org in the request body, or omit all of them to use the configured default.";
	}

That one-word version costs nothing and covers both. If you'd rather name the wrong-type case outright, normalizeOrg could return a small { kind: "blank" | "not-a-string" } marker instead of "" and orgError could pick the sentence — more machinery than it's worth for two strings, but it's there if the id case turns out to be common.

Either way, test/tools/execute-action.test.ts:434 (the it.each over list/object/number/boolean) already asserts the exact message, so it moves with this.

[AlexKantor87]

Took the one-word version. The message now asks for a single organization name, given as a non-empty string.

no mutation: the four cases asserting this string already exist and move with it; no behaviour changed.

[claude]

Test-coverage gap, and it's on the one behaviour you argued for rather than one you changed.

You declined rejecting a body-only org on the grounds that unwrapBodyParam makes a body key equivalent to params.org, so it is a supported channel. Agreed — but nothing asserts it. Every body-org test here is a conflict test; each one ends in expect(mockFetch).not.toHaveBeenCalled(). None shows a body-only org reaching the URL.

That leaves the reading ambiguous to a future maintainer. Someone who takes the earlier suggestion and rejects body-only orgs outright makes this file greener, not redder — the conflict tests still pass (a rejection is still a rejection), and the supported channel disappears with no failure. The decision you wrote up in the thread isn't pinned anywhere the suite can defend it.

One positive test closes it:

Suggested change

	});
	});
	// A body-nested org is params.org after the unwrap, so it is a supported way
	// to name the target, not just something to cross-check. Pinned because every
	// other body-org test asserts a rejection: dropping this channel would leave
	// those green, and the channel would vanish without a failure.
	it("targets an org named only inside a write's request body", async () => {
	const mockFetch = mockFetchOk({ created: true });
	await executeAction(
	entries, config, "post_control",
	{ body: { org: "cyber-dojo", identifier: "ctrl-1" } },
	undefined, mockFetch, "WRITE",
	);
	expect(mockFetch).toHaveBeenCalledWith(
	"https://app.kosli.com/api/v2/controls/cyber-dojo",
	expect.objectContaining({ body: JSON.stringify({ identifier: "ctrl-1" }) }),
	);
	});

The body assertion is worth keeping in it — it pins the other half of the contract, that the org leaves the payload on its way to the path segment.

[AlexKantor87]

Taken, with the reasoning corrected. Your premise was too kind to the gap in one direction and too harsh in another.

Dropping the channel outright is not silent. Removing the post-unwrap read turns three rejection tests red on its own, with the new test deleted. Those tests do pin that a body org is read.

What nothing pinned is that it is honoured. Keeping the cross-check while no longer using a body-only org as the target passed the whole suite. The comment now says that instead.

verified: sed -n 508,513p test/tools/execute-action.test.ts
// A body-nested org is params.org after the unwrap, so it is a supported way
// to name the target rather than only something to cross-check. The other
// body-org tests pin that the value is READ: removing the post-unwrap read
// turns three of them red. None pinned that it is HONOURED. Keeping the
// cross-check while no longer using a body-only org as the target passed the
// whole suite before this test existed.

search: awk "/describe("org selection"/,/^});/" test/tools/execute-action.test.ts
11 tests assert a URL was built. 10 assert no call was made.

The class is a channel asserted only by its rejections. Reading the 22 names, the other two channels each have a positive case. Those are "targets the org given as the org parameter" and "still accepts an org supplied inside params". The body was the one without.

mutation: taking the target from the org argument and params.org only, leaving the cross-check intact -> "targets an org named only inside a write request body" red, nothing red without it

[claude]

Content is right and worth recording — the ordering constraint around unwrapBodyParam is exactly the kind of thing that gets silently undone by a refactor, so pinning it here is the correct move.

Form is off for the file, though. Every other bullet in Conventions is one or two lines; this one is a ~12-line paragraph carrying six separate rules, and the ordering constraint — the load-bearing part — is the fifth sentence in. CLAUDE.md is read by agents that skim for the rule they need, so burying it costs the most where it matters most.

Same content, split so each rule is findable:

Suggested change

	- A per-call org is interpreted in exactly one place: `normalizeOrg` + `orgError` in `src/tools/execute-action.ts`, which both execute tools go through (the `KOSLI_ORG` default never passes through them: `loadConfig` trims it and rejects a blank one at startup, and `KosliClient.buildUrl` falls back to it when a call names no org). The `org` tool input and `params.org` obey the same rules — trimmed, only a string names one (coercing would turn `["a", "b"]` into the org `a,b`), `null` means not supplied, blank rejected, an org on a non-org-scoped action rejected, and two different orgs in one call rejected rather than resolved, because the same path performs writes. Note the ordering: `params.org` is read *before* `unwrapBodyParam` and the body's org *after* it — the unwrap flattens a write's body over the top level, so reading only once lets the spread pick a winner and hides the disagreement. Keep that decision in the one place; don't re-implement it per tool in `index.ts`.
	- A per-call org is interpreted in exactly one place: `normalizeOrg` + `orgError` in `src/tools/execute-action.ts`, which both execute tools go through. Don't re-implement it per tool in `index.ts`. The rules, applied identically to the `org` tool input and to `params.org`: trimmed; only a string names an org (coercing would turn `["a", "b"]` into the org `a,b`); `null` means not supplied; blank rejected; an org on a non-org-scoped action rejected; two different orgs in one call rejected rather than resolved, because the same path performs writes.
	- **Ordering matters in `executeAction`:** `params.org` is read *before* `unwrapBodyParam` and the body's org *after* it. The unwrap flattens a write's body over the top level, so reading only once lets the spread pick a winner and hides the disagreement.
	- The `KOSLI_ORG` default never passes through `normalizeOrg`: `loadConfig` trims it and rejects a blank one at startup, and `KosliClient.buildUrl` falls back to it when a call names no org.

[AlexKantor87]

Declining for this PR. Splitting the bullet is a readability change to a file this PR already rewrote twice, and it carries no behaviour.

The ordering constraint is the part worth finding, so I take the point about where it sits. Raising it with the rest of the CLAUDE.md tidy rather than adding a fifth commit here.

no mutation: this reply adds no test claim.

[claude]

Small coverage gap, and it's the one this PR creates rather than one it inherits.

Before this PR, the only way to aim a path segment was params, which the model already controlled. Now org is a declared top-level input on a write-capable tool, which makes it the most obvious thing for a model to fill from free text ("the org is cyber-dojo/../policies"). It reaches buildUrl and becomes a path segment.

The behaviour is safe: buildUrl runs encodeURIComponent(String(value)) (src/client/kosli-client.ts:146), so ../../user/default-org becomes ..%2F..%2Fuser%2Fdefault-org — one segment, and the WHATWG parser in fetch does not decode %2F before normalising, so no traversal. The org check doesn't need to reject it.

But nothing in the suite says so, on either side of the boundary — grep -rn "%2F\|encodeURIComponent" test/ is empty. normalizeOrg only trims, so the entire defence is one encodeURIComponent two files away, with no test tying it to the org. Swapping it for a template literal during a refactor passes everything.

The org-selection tests are the natural home now that the input exists:

Suggested change

	});
	});
	// The org is a top-level input on a tool that performs writes, and it lands
	// in a path segment. buildUrl percent-encodes it, so a slash-bearing value
	// stays one segment instead of re-pointing the request at another endpoint.
	// normalizeOrg deliberately does not reject this — the encoding is the
	// defence, so pin it here rather than leaving it implicit.
	it("encodes an org rather than letting it rewrite the path", async () => {
	const mockFetch = mockFetchOk();
	await executeAction(
	entries, config, "list_environments", {},
	undefined, mockFetch, "GET", "../../user/default-org",
	);
	expect(mockFetch).toHaveBeenCalledWith(
	"https://app.kosli.com/api/v2/environments/..%2F..%2Fuser%2Fdefault-org",
	expect.anything(),
	);
	});
	});

[AlexKantor87]

Taken. This also answers a question I had left open: whether the org needed rejecting for slashes or dots. It does not. The encoding is the defence, and the gap was that nothing said so.

verified: sed -n 626,640p test/tools/execute-action.test.ts
// The org is a top-level input on a tool that performs writes, and it lands in
// a path segment. buildUrl percent-encodes it, so a slash-bearing value stays
// one segment instead of re-pointing the request at another endpoint.
// normalizeOrg deliberately does not reject this: the encoding is the defence,
// so pin it here rather than leaving it implicit two files away.
it("encodes an org rather than letting it rewrite the path", async () => {
const mockFetch = mockFetchOk();

await executeAction(
  entries, config, "list_environments", {},
  undefined, mockFetch, "GET", "../../user/default-org",
);

expect(mockFetch).toHaveBeenCalledWith(
  "https://app.kosli.com/api/v2/environments/..%2F..%2Fuser%2Fdefault-org",
  expect.anything(),
);

The class is a caller value interpolated into a URL with nothing pinning the encoding:

search: grep -rn "encodeURIComponent|%2F" src/ test/ --include=*.ts
src/client/kosli-client.ts:146 path.replace(..., encodeURIComponent(String(value)))
test/tools/execute-action.test.ts:637 the new assertion

One interpolation site, now with one test on it. Query values go through URLSearchParams, which encodes them itself.

mutation: replacing encodeURIComponent(String(value)) with String(value) -> "encodes an org rather than letting it rewrite the path" red, nothing red on the parent commit

[claude]

The ordering is right and the test round-trip you added pins it. One seam worth a sentence, since the README now states the rule as an invariant.

The body channel only exists when unwrapBodyParam actually unwraps. When it bails — undeclared sibling key, entry takes no request body, a declared param genuinely called body — unwrapped === params, so normalizeOrg(unwrapped.org) re-reads params.org and the body's org is never looked at:

params: { org: "approved-org", unknown_key: 1, body: { org: "other-org", ... } }
  → siblings ["org", "unknown_key"]; unknown_key is not declared → no unwrap
  → named = ["approved-org"]          // "other-org" never seen, no conflict
  → POST /controls/approved-org  body {"body":{"org":"other-org",...},"unknown_key":1}

No safety problem, and I don't think it should change: the path org is still a value that was named and checked, and other-org rides along inside a payload the API will reject anyway. But README says "There are three ways to name an org in one call … They must agree", and here the third one silently isn't compared. The condition is non-obvious — it's a property of a different function, chosen for reasons that have nothing to do with orgs.

A line on the existing comment covers it:

Suggested change

	const unwrapped = unwrapBodyParam(entry, params);
	const unwrapped = unwrapBodyParam(entry, params);

…with the block above extended:

unwrapped is params when the unwrap declines (see unwrapBodyParam), so the body channel exists only when the body is flattened. That is the only case where a body org can reach the path, so it is also the only case worth cross-checking.

[AlexKantor87]

Taken as the comment you suggested, near enough.

verified: sed -n 142,148p src/tools/execute-action.ts
// Collect the org from every channel before deciding. unwrapBodyParam
// flattens a write request body over the top level, so params.org has to be
// read before the unwrap and the body org after it — otherwise the spread
// picks a winner and the disagreement is never seen. When the unwrap declines,
// unwrapped is params and the body org is never read. That is correct:
// an unflattened body cannot reach the path, so there is nothing to check.

The class is a decline path that returns the params untouched. All five return the same object, so the comment covers every one:

search: grep -n "return params" src/tools/execute-action.ts
46 body is null, not an object, or an array
47 entry takes no request body
48 entry declares a parameter called body
51 the body schema declares a body property
54 an undeclared sibling key sits alongside body

Left the README alone. No catalog action declares a parameter named body, and no request body schema has a body property, so the unwrap only declines on a malformed call.

no mutation: comment only, no behaviour changed.

[claude]

The one thing that stops the PR's headline benefit from landing in practice, and it's outside the files you touched.

The stated win is that a write's target org shows up in the approval prompt because it is a top-level input. That only happens if the model actually fills this input. But search_actions hands back entry.parameters verbatim (src/tools/search-actions.ts:50), and for all but four catalog actions that array contains:

{ "name": "org", "in": "path", "required": true, "description": "Organization name" }

So the documented flow — search first, then execute with the parameters you were given — tells the model that org is a required path parameter, i.e. something to put in params. This description says the opposite, and the two are read at different moments: the search result arrives with the action id the model is about to use, this description arrives with the schema. When they disagree, required: true in a per-action payload is the stronger signal.

Both channels work, so nothing breaks. But the case the PR set out to fix — org visible in the write-approval prompt rather than buried in a params blob — is exactly the case that reverts to the old shape whenever the model follows the search output.

Cheapest fix is a sentence here that names the competing signal, so the model doesn't have to resolve it by guessing:

Suggested change

	`Kosli organization to run this call against, e.g. "cyber-dojo". Defaults to "${config.org}" (from KOSLI_ORG). It applies to this call only — the next call goes back to the default unless you set it again.`,
	`Kosli organization to run this call against, e.g. "cyber-dojo". Defaults to "${config.org}" (from KOSLI_ORG). It applies to this call only — the next call goes back to the default unless you set it again. search_actions lists 'org' among an action's required path parameters; set it here rather than in params, and omit it entirely to use the default.`,

The more thorough version is to have searchActions drop the org path param from the parameters it returns (or annotate it as supplied by the server) — the model never needs to fill it, and it costs tokens on every search hit. That's a separate change to a file this PR doesn't touch, so a follow-up rather than something to fold in here.

Fix this →

[claude]

This test and orgError both decide "is this action org-scoped?" from parameters alone, so they agree by construction. The thing neither looks at is the path.

buildUrl substitutes {org} only while iterating entry.parameters (src/client/kosli-client.ts:126-148). An action whose path contains {org} but that doesn't declare it would be classified non-org-scoped here, orgError would reject any org the caller names, and the request would go out with a literal {org} in the URL — encoded to %7Borg%7D by neither side, since the replace never fires.

Today that can't happen (I checked: the four have paths /schemas/environment-policy/v1, /schemas/flow-template/v1, /user/default-org, /attestations/system-attestation-types). It's also a pre-existing buildUrl weakness rather than something this PR introduces. But this test's name asserts it has pinned the set of actions that take no org, and a {org} in one of those paths would make that claim false while leaving the suite green — the one classification the guard can't see.

One line covers it, in the same spirit as the bodyFields walk:

Suggested change

	.filter((entry) => !entry.parameters.some((p) => p.name === "org" && p.in === "path"))
	const withoutOrg = catalog
	.filter((entry) => !entry.parameters.some((p) => p.name === "org" && p.in === "path"))
	.map((entry) => entry.id)
	.sort();
	expect(withoutOrg).toEqual([
	"environment_policy_schema_v1",
	"flow_template_schema_v1",
	"get_user_default_org",
	"list_system_attestation_types",
	]);
	// Classifying by `parameters` matches buildUrl, which only substitutes
	// `{org}` while walking them. A path naming `{org}` without declaring it
	// would land here as non-org-scoped and ship the placeholder in the URL.
	const undeclared = catalog
	.filter((entry) => entry.path.includes("{org}"))
	.filter((entry) => !entry.parameters.some((p) => p.name === "org" && p.in === "path"))
	.map((entry) => entry.id);
	expect(undeclared).toEqual([]);

[AlexKantor87]

Superseded by #65.

Same feature, one commit instead of five. The review here found real things, and all of them are in #65. Two changes came out of re-reading the whole thing rather than the last diff:

search_actions now strips the org path parameter. It was advertising it as required for 114 of 118 actions, which told the model to put the org in params and undid the reason for the top-level input.

Registration moved to createServer in src/server.ts. Without that seam, dropping the org input from both tool schemas left the suite green while the MCP layer discarded the client org.

Closing this one. The threads stay readable here.