Skip to content

fix(desktop): put team name and instructions under the definition review contract - #5775

Open
Chessing234 wants to merge 2 commits into
block:mainfrom
Chessing234:fix/validate-team-definition-text
Open

fix(desktop): put team name and instructions under the definition review contract#5775
Chessing234 wants to merge 2 commits into
block:mainfrom
Chessing234:fix/validate-team-definition-text

Conversation

@Chessing234

Copy link
Copy Markdown
Contributor

Refs #5734 — the agreed follow-up from #4220. This covers two of the four listed ingresses; see "Scope".

What happens

#4220 put agent and persona definition text under one literal review and
validation contract. Teams stayed outside it. A team's instructions are
runtime-layered into every member deployment and delivered to the harness at
launch, so they are executable text by exactly the same argument — and yet
create_team / update_team ran trim_required / trim_optional and nothing
else. The result is that the same concealed characters were safer inside an
agent definition than in the team wrapped around it.

Inbound kind:30176 had a second, structural gap. Persona and managed-agent
content are parsed and validated together near the top of the reconcile, with
the comment:

Definition-bearing content is parsed and validated once here, before
retention, then reused in the apply branch below. This keeps an unsafe event
out of both the retention database and the local store.

Team was the one definition-bearing kind that did not follow that: it was parsed
inside the apply branch (team_content_from_event(&event)? at the KIND_TEAM
arm), after retain_inbound_event had already written it.

Fix

Local create/update. Both commands call validate_team_definition_text
immediately after trimming — before the store lock and before any load_teams /
save_teams. That ordering is what answers the issue's "prove a failed update
leaves no partial write": there is no write to be partial, because nothing has
been read or locked yet.

Inbound. Team content is parsed and validated alongside persona and
managed-agent content, before retention, and the apply branch reuses it instead
of re-parsing. This also removes a redundant parse.

One contract, two label sets. validate_agent_definition_text and
validate_team_definition_text both delegate to a shared
validate_reviewed_text. The limits (MAX_DISPLAY_NAME_CHARS,
MAX_SYSTEM_PROMPT_BYTES) and the invisible-character rules are deliberately
identical; only the labels differ, so the error names the field the person was
editing ("Team instructions are too long", not "Agent instructions"). Ordinary
whitespace and the existing emoji-format allowances carry over unchanged,
because it is literally the same code path.

One wire-semantics detail: TeamEventContent.instructions is a double option.
Absent means "publisher predates always-publish, preserve local"; null means
"explicitly cleared". Neither delivers text, so neither is rejected as if it
had — there is a test for both.

Evidence

9 new tests.

definition_validation.rs (5):

  • team text rejects the same 8 default-ignorable characters an agent's does
  • team text accepts ordinary whitespace and emoji
  • a team with no instructions carries no executable text
  • team errors are team-labelled for both the empty-name and over-length cases
  • agent error wording is unchanged by the refactor — this is the regression
    guard on sharing the contract

inbound_tests.rs (4):

  • an inbound team with a zero-width character in its instructions is rejected,
    with the U+200B codepoint in the message
  • an inbound team with a bidi override in its name is rejected
  • an ordinary inbound team is accepted
  • omitted and explicitly-cleared instructions both pass

Ran:

  • cargo test --manifest-path desktop/src-tauri/Cargo.toml --lib2435
    passed, 0 failed, 14 ignored
    (2426 on main + 9)
  • cargo clippy --manifest-path desktop/src-tauri/Cargo.toml --all-targets — no
    warnings, no errors
  • cargo fmt --manifest-path desktop/src-tauri/Cargo.toml --all -- --check — clean

Not run locally: the Tauri command layer end to end. create_team / update_team
need an AppHandle, so the validation is verified at the function it calls and
by reading the call order, not by driving the command.

Scope

The issue lists four ingresses. This PR does the first two — local create/update
and inbound before retention. Left for a follow-up:

  • Team snapshot preview/confirm (commands/team_snapshot.rs). Its phase-1
    "validate (no I/O)" block is the natural home, but the snapshot path carries
    its own member-level validation and staging semantics, and folding a new field
    check into it deserves its own diff and its own tests rather than riding along
    here.
  • Publication. Outbound publish is fed by the local store, which this PR now
    gates on the way in, so it is defense in depth rather than an open hole —
    worth adding, not urgent, and better argued separately.

Happy to do either or both in this PR instead if you would rather land the whole
contract at once.

…iew contract

block#4220 put agent and persona definition text under one literal review and
validation contract. Teams stayed outside it, even though a team's
`instructions` are runtime-layered into every member deployment and
executed at launch -- so the same concealed text was safer inside an
agent definition than in the team wrapped around it.

Two production ingresses now validate:

- `create_team` / `update_team` check the trimmed name and instructions
  before taking the store lock and before any load or save, so a rejected
  team cannot leave a partial write behind.
- Inbound kind:30176 is parsed and validated next to persona and
  managed-agent content, before retention, keeping an unsafe team out of
  both the retention database and the local store. It was previously the
  only definition-bearing kind parsed after retention, inside the apply
  branch.

`validate_agent_definition_text` and the new
`validate_team_definition_text` share one contract with per-surface
labels: identical limits and identical invisible-character rules, so the
error a person sees names the field they were editing. Agent-facing
wording is unchanged and there is a test pinning that.

Refs block#5734

Signed-off-by: Taksh <takshkothari09@gmail.com>
@Chessing234
Chessing234 requested a review from a team as a code owner August 13, 2026 16:42

@wolfyy970 wolfyy970 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scope is right. Team instructions reach every member at launch, so local authoring and inbound retention should use the same reviewed-text contract as agent definitions. Keeping snapshot import and outbound defense-in-depth as separate increments is sensible.

One small change before approval: the new KIND_TEAM branch adds inbound_team.expect("team parsed above") in production code. This repository does not allow new expect() calls on production paths. Please mirror the adjacent managed-agent branch and convert the invariant to an ok_or_else(...)? error before applying the team.

I ran the focused definition-validation and inbound-team tests at exact head 0f15d4c; they pass, and git diff --check is clean. I found no other blocker.

Review on block#5775: production paths here do not take new `expect()` calls.
The managed-agent branch two arms down already answers the same invariant
with `ok_or_else(...)?`; do the same for the team arm.

Signed-off-by: Taksh <takshkothari09@gmail.com>
@Chessing234

Copy link
Copy Markdown
Contributor Author

dropped the expect — the team arm now mirrors the managed-agent one with ok_or_else(...)? on the same invariant. verified with cargo check and cargo clippy --workspace --all-targets -D warnings on the tauri manifest (both clean); i couldn't run the tauri test suite locally, disk ran out mid-build, so that one's on ci

@Chessing234

Copy link
Copy Markdown
Contributor Author

follow-up on my last note: disk freed up, so i ran the tauri suite on this tip after all — cargo test --manifest-path desktop/src-tauri/Cargo.toml --lib, 2435 passed / 0 failed. clippy --workspace --all-targets -D warnings still clean

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.

2 participants