Skip to content

fix(agent-editor): stop dropping behavior_policy.supports_team - #4176

Open
arnaud-moona wants to merge 3 commits into
iOfficeAI:mainfrom
arnaud-moona:fix/advanced-editor-drops-supports-team
Open

arnaud-moona wants to merge 3 commits into
iOfficeAI:mainfrom
arnaud-moona:fix/advanced-editor-drops-supports-team

Conversation

@arnaud-moona

@arnaud-moona arnaud-moona commented Aug 25, 2026

Copy link
Copy Markdown

Description

The advanced panel of the custom agent editor is a free-form JSON editor, but handleJsonChange
whitelists the keys it keeps. For behavior_policy it preserved only supports_side_question, so
typing

{ "behavior_policy": { "supports_team": true } }

validated, showed no error, and was silently discarded on save — the value never reached
/api/agents/custom, which does accept it.

That flag gates team-mode eligibility and is seeded false for custom agents, so a custom ACP agent
could not be made team-selectable through the UI at all, even when it advertises
agentCapabilities.mcpCapabilities.stdio: true at initialize. The only remaining routes were the
HTTP API (business endpoints require a session, so unreachable from outside the app) or editing
aioncore's SQLite directly.

The change parses supports_team alongside supports_side_question, building the policy object
incrementally so neither key clobbers the other; surfaces both keys in the skeleton JSON so the panel
is self-documenting; and declares the field on CustomAgentAdvancedOverrides and BehaviorPolicy,
where it had been dropped while the backend still reads it.

Serialization moved to advancedOverrides.ts so the round-trip is unit-testable — the parsing lived
in an inline useCallback and was unreachable from a test. Behaviour is preserved, including two
pre-existing quirks now pinned by tests rather than left implicit: valid JSON that is not an object
("s", null) clears the error and keeps the previous bag, hence a three-way outcome
(ok / ignored / invalid); and an array parses as an empty bag, because typeof [] === 'object'
has always let it through.

Related Issues

  • Companion issue on deriving team_capable from the handshake (backend-side): to be linked

Type of Change

  • fix — Bug fix (non-breaking change which fixes an issue)
  • feat — New feature (non-breaking change which adds functionality)
  • perf — Performance improvement
  • refactor — Code restructuring (no behavior change)
  • Breaking change (fix or feature that would break existing functionality)
  • docs — Documentation update

Atomic PR Checklist (Rule 1)

  • This PR contains exactly one feature or bug fix that cannot be further decomposed
  • The PR title follows Conventional Commit format, for example fix(scope): subject (English)

The three commits are the fix, its regression test, and the formatting pass — one change, not three.

Local Checks (Rule 3)

  • bun run format — changed files formatted; full format:check passes (2084 files)
  • bun run lint — 0 errors (912 pre-existing repository warnings only)
  • bunx tsc --noEmit — no type errors
  • bunx vitest run — 518 files passed, 1 skipped; 4,957 tests passed, 5 skipped, 0 failed
  • i18n validated (node scripts/generate-i18n-types.js + node scripts/check-i18n.js)
  • New/changed user-facing text uses i18n keys (no new user-facing text)

Runtime Verification

  • Verified on macOS
  • Verified on Windows
  • Verified on Linux
  • I have performed a self-review of my own code

Screenshots

Not applicable; the panel already existed and its rendering is unchanged. The only visible difference
is that the skeleton JSON now lists supports_team alongside supports_side_question.

Additional Context

Tests. tests/unit/settings/advancedOverrides.test.ts covers the round-trip: supports_team
survives alone and alongside supports_side_question, non-boolean flags are ignored rather than
coerced, blanks are still dropped, and buildAdvancedJsonparseAdvancedOverrides preserves what
the panel displays. Writing them caught a mistake in the first draft, which asserted arrays were
ignored — that would have been a silent behaviour change smuggled into a bug fix.

Coverage. Codecov flags the patch at 16%. The uncovered lines are the thin component-side binding
in InlineAgentEditor.tsx, not the logic, which now lives in a separate module and is covered.
Covering the remaining branches would require a DOM test mounting the whole editor (CodeMirror, Arco,
ipcBridge, the emoji picker) to exercise three ifs. Happy to add one if you would rather have it.

Flaky test in CI, green locally. An earlier CI run on this branch failed on
tests/unit/renderer/mermaidBlockPanZoom.dom.test.tsx. The full suite passes locally on the same
commit (4,957 passed, 0 failed), so the failure is load-sensitive rather than a regression — and it
is unrelated either way: the test exists on main, this branch neither adds nor modifies it, and
MermaidBlock shares no code with the files changed here.

The trace points at a race: the test captures inner via diagram.firstElementChild immediately
after findByTestId, so if the async mermaid render replaces that node the reference is detached and
its transform never updates — which matches the observed scale(1) after a zoom-in click. Flagging
it in case it is worth hardening separately.

Scope. This makes the existing flag reachable. It does not address why a custom agent that
declares MCP stdio needs a manual flag at all, when team_capable is documented as "MCP stdio
capable, computed by backend" — the stored mcp_capabilities currently keeps only http and sse,
so the advertised stdio never reaches the catalog. That belongs in the backend and is filed
separately.

@arnaud-moona
arnaud-moona force-pushed the fix/advanced-editor-drops-supports-team branch from 3de65a6 to d792166 Compare August 25, 2026 05:18
@codecov

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...pages/settings/AgentSettings/InlineAgentEditor.tsx 20.00% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

@arnaud-moona
arnaud-moona force-pushed the fix/advanced-editor-drops-supports-team branch 3 times, most recently from e119786 to b143ef5 Compare August 25, 2026 10:46
The custom agent editor's advanced panel is a free-form JSON editor, but
`handleJsonChange` whitelists the keys it keeps. For `behavior_policy` it
only preserved `supports_side_question`, so typing

    { "behavior_policy": { "supports_team": true } }

looked accepted and was silently discarded on save — the field never
reached `/api/agents/custom`, which does accept it.

That flag gates team-mode eligibility and is seeded `false` for custom
agents, so a custom ACP agent could not be made team-selectable through
the UI at all. The only remaining routes are the HTTP API (unreachable
from outside the app since business endpoints require a session) or
editing aioncore's SQLite directly.

- parse `supports_team` alongside `supports_side_question`, building the
  policy object incrementally so neither key clobbers the other;
- surface both keys in the skeleton JSON so the panel is
  self-documenting — `supports_team` was otherwise undiscoverable;
- declare the field in `CustomAgentAdvancedOverrides` and
  `BehaviorPolicy`, where it had been dropped while the backend still
  reads it.

No behaviour change for agents that do not set the flag.
Follow-up to the previous commit, which fixed the dropped key without a
regression test — the parsing lived in an inline `useCallback`, so it was
not reachable from a unit test.

- move the serialize/parse pair to `advancedOverrides.ts` (a .ts module,
  so the node-environment test does not pull React);
- cover it in `tests/unit/settings/`: `supports_team` survives, both
  policy flags coexist, non-booleans are ignored, blanks are dropped, and
  the build → parse round-trip preserves what the panel shows.

Behaviour is preserved, including two quirks now pinned by tests rather
than left implicit:
- valid JSON that is not an object (`"s"`, `null`) clears the error and
  keeps the previous bag, so the result is a three-way outcome
  (`ok` / `ignored` / `invalid`) rather than a nullable value;
- an array parses as an empty bag, because `typeof [] === 'object'` has
  always let it through. Documented, not changed: this PR fixes a dropped
  key, not the panel's tolerance for odd input.

Writing the tests caught the second point — the first draft asserted
arrays were ignored, which would have been a silent behaviour change.
CI feedback:

- Oxfmt reformatted `InlineAgentEditor.tsx` (one over-long line). Applied
  with the pinned oxfmt 0.41.0 so the hook is a no-op now.
- Codecov flagged the patch: the remaining uncovered lines are the thin
  component-side binding, not the logic. `buildJsonFromAdvanced` was a
  `useCallback` wrapper around a module-level pure function — already
  stable across renders, so the indirection bought nothing and only added
  untestable lines. Calling `buildAdvancedJson` directly removes it and
  simplifies the effect's dependency array.

oxlint reports 0 warnings / 0 errors on both new files.
@arnaud-moona
arnaud-moona force-pushed the fix/advanced-editor-drops-supports-team branch from 519789c to c8acef2 Compare September 13, 2026 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant