fix(interface): reject lossy agent profiles#61
Conversation
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 4074c473
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
tangletools · auto-approval · reason: drewstone_author · 2026-07-16T09:26:59Z
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — eb694277
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
tangletools · auto-approval · reason: drewstone_author · 2026-07-16T09:34:37Z
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Concerns | 1 (1 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 124.8s (2 bridge agents) |
| Total | 124.8s |
💰 Value — sound-with-nits
Tightens the canonical AgentProfile contract — strict objects everywhere, MCP as an exact local|remote|disabled union in both TS types and Zod, disabled MCP skipped in security checks — plus a bundled (separate-commit) realignment of per-harness reasoning-effort sets; the core change is in-grain and
- What it does: Converts every nested AgentProfile Zod object from strip-by-default to strictObject, so unknown fields anywhere in a parsed profile now fail validation instead of being silently dropped (profile-schema.ts:25-248). Replaces the flat, all-optional AgentProfileMcpServer interface with a three-way union — local (command required, url/headers forbidden), remote (url required and validated as http(s), p
- Goals it achieves: Fail closed on profile authoring: typos or misplaced fields previously vanished at parse time and then diverged between what the author wrote and what backends materialized; now they error at the boundary. The MCP union removes a genuinely ambiguous shape (command+url together, transport/fields mismatched, disabled-but-carrying-process-fields) that previously had to be policed by a superRefine aft
- Assessment: Good on its merits. The codebase's own comments already document the strip-vs-strict divergence as a prior bug ('the guard that was missing when the two silently diverged', profile-schema.ts:276-280), and the pattern (strictObject + a bidirectional tsc guard + runtime tests asserting rejection at every object boundary in the new profile-schema.test.ts) is exactly how this package already keeps TS
- Better / existing approach: Searched for existing equivalents:
rg AgentProfileMcpServershows the MCP shape is defined only in agent-interface (profile-schema.ts, agent-profile.ts, agent-candidate*.ts) — no duplicate MCP schema exists in providers (agent-provider-cli-bridge/e2b/etc. only carry boolean capability flags, not profile parsing); the opencode-native validateProfileSecurity referenced in profile-security.ts:15 li - Model: opencode/kimi-for-coding/k2p7
- Bridge attempts: 1
🎯 Usefulness — sound
Tightens the canonical agent-profile MCP contract into an unambiguous local|remote|disabled discriminated union (TS + Zod + security gate, all in lockstep) and corrects the per-harness reasoning-effort matrix to match real native CLIs — coherent, idiomatic, and fully wired into the contract every ba
- Integration: Fully reachable.
agentProfileSchema,agentProfileMcpServerSchema,harnessReasoningEfforts,reasoningEffortsFor, andvalidateAgentProfileSecurityare all exported from agent-interface and consumed: the profile schema is the runtime validator for the canonicalAgentProfilethat every backend translates from (cli-bridge declaresmcp: trueat agent-provider-cli-bridge/src/index.ts:250 an - Fit with existing patterns: Fits the codebase grain precisely. It strengthens an already-central contract rather than introducing a parallel one: the discriminated-union +
never-field pattern makes invalid MCP states unrepresentable at the type level, the Zod schemas mirror each variant withz.strictObject+z.union, and the existing compile-time drift guard (MutuallyAssignable, profile-schema.ts:281-290) keeps TS an - Real-world viability: Holds up off the happy path. The
z.unionof strict variants rejects every ambiguous or incomplete combination ({command,url}, {transport:'http',command}, bare {args}, disabled-with-fields, blank/non-HTTP urls), and the new test suite (profile-schema.test.ts) enumerates these. The security validator correctly skips disabled servers (profile-security.ts:154) and itsisLocalMcpServerpredicate (c - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 PR bundles an unrelated reasoning-effort behavior change under a profile-validation title [proportion] ``
Commit eb69427 ('align native reasoning controls') rewrites harnessReasoningEffortsOverride (harness-capabilities.ts:160-178) — codex now advertises minimal+ultracode, kimi-code swaps minimal→none, pi/openclaw gain none/ultracode. That changes what the sandbox UI pickers and router offer users, which is a real behavior delta with no connection to 'reject lossy profiles' beyond touching the same package. It's cleanly separated into its own commit and is itself a sensible grounding fix, so it does
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — cf5eade3
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
tangletools · auto-approval · reason: drewstone_author · 2026-07-16T09:43:07Z
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Concerns | 1 (1 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 181.2s (2 bridge agents) |
| Total | 181.2s |
💰 Value — sound-with-nits
Makes agent-profile validation strict end-to-end (no silent stripping, exact local/remote/disabled MCP unions in both TS types and zod, compile-time drift guard, security check honors disabled entries) plus two small ride-along exactness commits — coherent, in-grain, no existing equivalent; ship.
- What it does: Four concrete deltas. (1) Every zod object in profile-schema.ts becomes z.strictObject (profile-schema.ts:25-248) so unknown nested keys now reject instead of being silently stripped, while metadata/extensions stay open. (2) AgentProfileMcpServer is remodeled from one bag-of-optionals interface into a TS union of local/remote/disabled variants with
neveron cross fields (agent-profile.ts:233-272 - Goals it achieves: Eliminate lossy validation: a profile with a typo'd or future field previously parsed 'successfully' with the field dropped, so the deployed agent silently diverged from what the author wrote. Remove ambiguous MCP shapes (command+url together, transport/fields mismatch) that meant the same JSON could be interpreted differently by TypeScript consumers vs the runtime validator vs the security gate.
- Assessment: Good on its merits. The union-of-strictObjects is the idiomatic zod construction for exactly this problem and matches the package's existing grain (strict schemas, heavy doc comments on security boundaries, test-per-invariant). The drift guard is the right mechanism — it converts 'keep in sync by discipline' into a tsc failure. The candidate schema deliberately differing from the profile schema (n
- Better / existing approach: None found. Searched the full repo for other MCP schemas/types (grep for McpServerSchema, interface.*McpServer, transport-bearing zod objects): the only definitions are these two in agent-interface, and they intentionally differ (candidate forbids remote, uses AgentCandidateConfigValue, adds executable/cwd safety refines) — a shared factory would need parameterization for value type, remote arm, a
- Model: opencode/kimi-for-coding/k2p7
- Bridge attempts: 1
🎯 Usefulness — sound
A correctness hardening of the canonical @tangle-network/agent-interface contract: it makes every profile object boundary reject unknown fields, splits ambiguous MCP server shapes into exact local/remote/disabled unions, and realigns per-harness reasoning sets with the real native CLI controls — all
- Integration: Fully reachable. All touched symbols are public exports of the published agent-interface package (index.ts:830-832 re-exports harness-capabilities, profile-schema, profile-security). agentProfileSchema validates every profile at the application boundary and is the
definitionfield of the exported capabilitySchema (profile-schema.ts:313-316); validateAgentProfileSecurity gates unattended-code sur - Fit with existing patterns: Squarely in the grain. The codebase already established every pattern this PR extends: the bidirectional MutuallyAssignable drift guard (profile-schema.ts:281-290), .strict() on the root object, per-harness reasoning-effort overrides (harnessReasoningEffortsOverride), and the inline-profile security validator. The PR widens strictness to nested boundaries and corrects the previously-inaccurate rea
- Real-world viability: Holds off the happy path. The MCP union uses never-typed mutual-exclusion + z.undefined().optional() so JSON that round-trips explicit undefined keys (local with url:undefined, remote with command:undefined) parses cleanly — covered in profile-schema.test.ts and a real serialization concern. The security skip of disabled servers is provably safe because the disabled schema (agent-profile.ts:259-26
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 PR carries three logical changes under a title that names one [proportion] ``
git log shows 4074c47 (lossy-profile rejection, the titled change), eb69427 (reasoning-effort sets realigned to native CLI flags — harness-capabilities.ts:153-179, ~56 lines), and cf5eade (candidate systemPrompt plan material). The latter two are unrelated to validation strictness and ride in under the same changeset. Each is small, tested, and in the same package, so this is an FYI, not a gate — but a reviewer reading only the title would miss that harness capability behavior changed (codex/pi/
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
✅ No Blockers —
|
| glm | kimi-code | aggregate | |
|---|---|---|---|
| Readiness | 86 | 79 | 79 |
| Confidence | 70 | 70 | 70 |
| Correctness | 86 | 79 | 79 |
| Security | 86 | 79 | 79 |
| Testing | 86 | 79 | 79 |
| Architecture | 86 | 79 | 79 |
Reviewer score is advisory once the run is complete and the verdict has no blockers.
Full multi-shot audit completed 2/2 planned shots over 11 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 2/2 planned shots over 11 changed files. Global verifier still owns final merge decision.
🟠 MEDIUM Disabled-MCP early-skip bypasses the local-process gate for unvalidated profiles — packages/agent-interface/src/profile-security.ts
The new
if (server.enabled === false) continue;runs BEFOREisLocalMcpServer(server). For schema-validated input this is safe (the new union schema forbids disabled servers carrying command/url). But this function's own docstring says it is run 'again at dispatch as defense in depth' and its signature accepts a plainAgentProfile— a caller that JSON.parses a profile without running agentProfileSchema can now smuggle{enabled: false, command: 'evil'}past the BLOCKED_LOCAL_MCP gate, which the pre-change code flagged. Fix: only skip when the server carries no process/remote fields, e.g.if (server.enabled === false && !isLocalMcpServer(server) && !('url' in server && server.url)) continue;— or emit an INVALID_MCP issue for disabled servers with residual fields instead of silently
🟡 LOW Changeset description is a single run-on sentence — .changeset/strict-agent-profile.md
The body crams four distinct changes (reject unknown fields, exact MCP config union, reasoning-control alignment, system-prompt binding) into one sentence joined by 'and'. Splitting into a short bullet list would improve changelog readability. Pure stylistic nit, does not block.
🟡 LOW kimi-code 'off' level renamed minimal→none is a silent behavioral break for external consumers — packages/agent-interface/src/harness-capabilities.ts
harnessReasoningEfforts('kimi-code') changed from ['minimal','high'] to ['none','high']. The docblock says this layer is read by cli-bridge adapters and sandbox UI pickers that live outside this repo; any consumer that mapped canonical 'minimal' →
--no-thinkingfor kimi now never emits the off flag (and 'none' may be unhandled downstream). The changeset is only a minor bump. Internal tests are updated and consistent, so this is intentional — but downstream adapters must be updated in lockstep or kimi's thinking-off path silently regresses to thinking-on.
🟡 LOW Subagent strict-mode test case uses an inadvertently-suspicious field name — packages/agent-interface/src/profile-schema.test.ts
The 'subagent' case
{ subagents: { reviewer: { prompt: 'Review carefully', permission: { bash: 'deny' } } } }usespermission(singular) — which is genuinely not a field of AgentSubagentProfileSchema (the schema haspermissionsplural). The assertion holds because strictObject rejects the unknown key, but the test would still pass even if a future change introduced a singularpermissionfield. Recommend changing to a clearly-unknown key (e.g.unknownField: true) or the correct pluralpermissionspaired with an additional unknown key, so the test's intent is unambiguous. Cosmetic; does not block.
🟡 LOW cosmetic: misaligned indentation in capabilitySchema after strictObject conversion — packages/agent-interface/src/profile-schema.ts
The z.strictObject body uses 4-space continuation indent (
id: z.string().min(1),) inconsistent with the 2-space style used throughout the rest of the file. No functional impact; typecheck and tests pass.
🟡 LOW z.union (not discriminatedUnion) on MCP server schemas yields weaker error messages and no single-tag narrowing — packages/agent-interface/src/profile-schema.ts
The three MCP variants share no common required literal discriminator (
enabledisz.literal(true).optional()on local+remote,z.literal(false)on disabled;transportis a different literal-set per variant and is optional). z.union is therefore the correct structural choice and runtime behavior is right (verified: ambiguous {command,url} is rejected, incomplete servers are rejected). Trade-off: when parsing fails, Zod reports all branch errors instead of one targeted message, and downstream consumers narrowing the union must rely on thenever-typed fields rather than a tag. If clearer error UX is desired later, an explicitkind: 'local' | 'remote' | 'disabled'discriminator could be added — but that is a public-API change, not in scope here. Note only; no action required for thi
tangletools · 2026-07-16T09:52:36Z · trace
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 6 non-blocking findings — cf5eade3
Full multi-shot audit completed 2/2 planned shots over 11 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 2/2 planned shots over 11 changed files. Global verifier still owns final merge decision.
Full immutable report for this review: trace
Summary comment for this run: full summary
tangletools · 2026-07-16T09:52:36Z · immutable trace
Problem
AgentProfile validation rejected unknown root fields but silently stripped unknown nested behavior. MCP definitions also allowed ambiguous local/remote shapes and diverged between TypeScript and runtime validation.
Solution
Verification
pnpm test(all 8 workspace packages)pnpm buildpnpm --filter @tangle-network/agent-interface check-typespnpm check:package-artifacts(8 isolated packages, 18 exports, 36 targets)