fix(desktop): validate team snapshot text before import - #5824
Open
Chessing234 wants to merge 2 commits into
Open
fix(desktop): validate team snapshot text before import#5824Chessing234 wants to merge 2 commits into
Chessing234 wants to merge 2 commits into
Conversation
…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>
`build_import_team` checked only that the snapshot's team name was non-empty and copied `instructions` straight through. The agent-snapshot path already validates its definition text before import, so the same concealed characters were rejected inside a member and accepted in the team wrapped around them -- and a snapshot is a file from outside this install, which makes import the one place that text is reviewed before it can execute. `build_import_team` runs in `confirm_team_snapshot_import`'s Phase 1, before key minting and before any store write, so a rejection leaves nothing behind. The pre-existing empty-name check runs first and keeps its own wording. Refs block#5734 Signed-off-by: Taksh <takshkothari09@gmail.com>
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
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.
Refs #5734 — the third of its four ingresses, and the follow-up I offered at the end of #5775.
What happens
build_import_teamvalidated exactly one thing:instructionswas then copied straight into theTeamRecordunexamined.The agent-snapshot path does not work that way —
agent_snapshot.rscallsvalidate_agent_definition_texton the member's display name and system promptand refuses the import with "Snapshot definition is unsafe". So the same
concealed characters were rejected inside a member and accepted in the team
wrapped around them, even though the team's
instructionsare runtime-layeredinto every member deployment.
That asymmetry matters more here than at the other ingresses: a snapshot is a
file from outside this install. Import is the one point at which that text is
reviewed before it can execute.
Fix
One
validate_team_definition_textcall inbuild_import_team, using the same"Team snapshot is unsafe: …" error shape the agent path uses for its own
snapshots.
Placement is load-bearing and already correct:
build_import_teamruns insideconfirm_team_snapshot_import's Phase 1 — validate (no I/O), before keyminting and before the store lock. A rejected snapshot therefore mints no
keypairs and writes nothing, which is #5734's "prove a failed update leaves no
partial write" for this ingress.
The pre-existing empty-name check stays ahead of it and keeps its own wording, so
that error message does not move.
Evidence
5 new tests in
team_snapshot/tests.rs, driving the purebuild_import_team:U+200Bis refused, with the codepoint inthe message
instructions
message, not the new one
Ran:
cargo test --manifest-path desktop/src-tauri/Cargo.toml --lib— 2440 passed,0 failed, 14 ignored (2435 on the base branch + 5)
cargo clippy --manifest-path desktop/src-tauri/Cargo.toml --all-targets— nowarnings, no errors
cargo fmt --manifest-path desktop/src-tauri/Cargo.toml --all -- --check— cleanNot run locally: the import command end to end.
confirm_team_snapshot_importisa Tauri command needing an
AppHandle, so this is verified at the pure functionit calls and by reading the phase ordering around it.
Remaining
That leaves one ingress from #5734: publication. Outbound publish is fed by
the local store, which #5775 and this PR now gate on the way in from every
direction a team can arrive — local edit, relay sync, and file import — so it is
defense in depth rather than an open hole. Happy to add it if you would rather
the contract be closed at the write side too.