From afd58e31d84ddbfc7fbb34793c585c284a863773 Mon Sep 17 00:00:00 2001 From: SaladDay Date: Mon, 3 Aug 2026 11:49:56 +0000 Subject: [PATCH] fix(pi): close final ownership and native UX gaps --- scripts/pi-transport-capture.mjs | 61 +++++++++++-------- src-tauri/src/commands/import_export.rs | 3 +- src-tauri/src/config.rs | 53 ++++++++++++---- src-tauri/src/services/pi_prompt_files.rs | 57 +++++++++++++++-- src-tauri/src/services/skill_deployment.rs | 55 +++++++++++++++++ src-tauri/src/services/sync_protocol.rs | 41 +++++++++++++ src-tauri/src/settings.rs | 49 +++++++++++++++ .../prompts/PiNativePromptResources.tsx | 12 +++- src/i18n/locales/en.json | 1 + src/i18n/locales/ja.json | 1 + src/i18n/locales/zh-TW.json | 1 + src/i18n/locales/zh.json | 1 + src/lib/piPromptSlug.ts | 17 ++++++ .../PiNativePromptResources.test.tsx | 27 +++++++- tests/lib/piPromptSlug.test.ts | 33 ++++++++++ 15 files changed, 364 insertions(+), 48 deletions(-) create mode 100644 src/lib/piPromptSlug.ts create mode 100644 tests/lib/piPromptSlug.test.ts diff --git a/scripts/pi-transport-capture.mjs b/scripts/pi-transport-capture.mjs index 3ab5d5806..6069c1780 100644 --- a/scripts/pi-transport-capture.mjs +++ b/scripts/pi-transport-capture.mjs @@ -141,7 +141,7 @@ writeFileSync( `export { composeModelProvider } from "${PI}/packages/coding-agent/src/core/provider-composer.ts";`, `export { resolveConfigValueOrThrow } from "${PI}/packages/coding-agent/src/core/resolve-config-value.ts";`, `export { loadSkills } from "${PI}/packages/coding-agent/src/core/skills.ts";`, - `export { loadPromptTemplates } from "${PI}/packages/coding-agent/src/core/prompt-templates.ts";`, + `export { loadPromptTemplates, expandPromptTemplate } from "${PI}/packages/coding-agent/src/core/prompt-templates.ts";`, `export { SessionManager } from "${PI}/packages/coding-agent/src/core/session-manager.ts";`, `export { parseArgs } from "${PI}/packages/coding-agent/src/cli/args.ts";`, `export { createAllToolDefinitions } from "${PI}/packages/coding-agent/src/core/tools/index.ts";`, @@ -529,33 +529,47 @@ writeFileSync( join(promptAgentDir, "prompts", "review.md"), "---\ndescription: Review captured changes\nargument-hint: \n---\nReview $1\n", ); +writeFileSync( + join(promptAgentDir, "prompts", "release notes.md"), + "This spaced filename cannot be addressed as one slash-command token.\n", +); +writeFileSync(join(promptAgentDir, "prompts", "release.v2.md"), "Release $1\n"); +writeFileSync(join(promptAgentDir, "prompts", "评审.md"), "评审 $1\n"); writeFileSync(join(promptAgentDir, "prompts", "empty.md"), ""); writeFileSync( join(promptAgentDir, "prompts", "nested", "ignored.md"), "nested", ); +const loadedPromptTemplates = adapters.loadPromptTemplates({ + cwd: promptProjectDir, + agentDir: promptAgentDir, + promptPaths: [], + includeDefaults: true, +}); const promptTemplateDiscovery = jsonSafeJavaScriptValue( - adapters - .loadPromptTemplates({ - cwd: promptProjectDir, - agentDir: promptAgentDir, - promptPaths: [], - includeDefaults: true, - }) - .map((template) => ({ - name: template.name, - description: template.description, - argumentHint: template.argumentHint, - content: template.content, - source: template.sourceInfo?.source, - scope: template.sourceInfo?.scope, - relativeFile: - template.filePath === - join(promptAgentDir, "prompts", `${template.name}.md`) - ? `prompts/${template.name}.md` - : template.filePath, - })), + loadedPromptTemplates.map((template) => ({ + name: template.name, + description: template.description, + argumentHint: template.argumentHint, + content: template.content, + source: template.sourceInfo?.source, + scope: template.sourceInfo?.scope, + relativeFile: + template.filePath === + join(promptAgentDir, "prompts", `${template.name}.md`) + ? `prompts/${template.name}.md` + : template.filePath, + })), ); +const promptTemplateExpansion = [ + "/review captured-range", + "/release notes", + "/release.v2 captured-range", + "/评审 变更", +].map((input) => ({ + input, + result: adapters.expandPromptTemplate(input, loadedPromptTemplates), +})); // File presence, including a zero-byte file, is the native activation state // for Pi's global instruction resources. Execute the real resource loader so @@ -717,9 +731,7 @@ writeFileSync( [ JSON.stringify(capturedSession.getHeader()), "{not valid json", - ...capturedSession - .getEntries() - .map((entry) => JSON.stringify(entry)), + ...capturedSession.getEntries().map((entry) => JSON.stringify(entry)), "", ].join("\n"), ); @@ -763,6 +775,7 @@ console.log( resolverCases, skillDiscovery, promptTemplateDiscovery, + promptTemplateExpansion, emptyInstructionFiles, sessionDirectorySemantics, sessionCliSemantics, diff --git a/src-tauri/src/commands/import_export.rs b/src-tauri/src/commands/import_export.rs index b5b5c85db..dfa8303b9 100644 --- a/src-tauri/src/commands/import_export.rs +++ b/src-tauri/src/commands/import_export.rs @@ -12,6 +12,7 @@ use crate::database::backup::BackupEntry; use crate::database::Database; use crate::error::AppError; use crate::services::provider::ProviderService; +use crate::services::skill_deployment::PiSkillDeploymentService; use crate::store::AppState; // ─── File import/export ────────────────────────────────────── @@ -57,7 +58,7 @@ pub async fn import_config_from_file( let import_path = filePath.clone(); let import_result = tauri::async_runtime::spawn_blocking(move || { - db.import_portable_sql(&PathBuf::from(import_path)) + PiSkillDeploymentService::import_portable_sql(&db, &PathBuf::from(import_path)) }) .await .map_err(|error| AppError::Message(format!("SQL import task failed: {error}"))) diff --git a/src-tauri/src/config.rs b/src-tauri/src/config.rs index ac33dc66c..d06e59c46 100644 --- a/src-tauri/src/config.rs +++ b/src-tauri/src/config.rs @@ -300,17 +300,18 @@ pub fn atomic_write(path: &Path, data: &[u8]) -> Result<(), AppError> { /// Durable same-directory atomic replacement. /// -/// Existing permissions are preserved. `new_file_mode` controls only a newly -/// created Unix file (settings and other local secrets pass `0o600`). The -/// temporary file is created exclusively, synced before replacement, and the -/// containing directory is synced afterwards on Unix. +/// Existing permissions are preserved when `required_file_mode` is `None`. +/// Sensitive callers pass an explicit mode (for example `0o600`), which is +/// enforced for both new and existing Unix files before the replacement +/// becomes visible. The temporary file is created exclusively, synced before +/// replacement, and the containing directory is synced afterwards on Unix. pub(crate) fn atomic_write_durable( path: &Path, data: &[u8], - new_file_mode: Option, + required_file_mode: Option, ) -> Result<(), AppError> { #[cfg(not(unix))] - let _ = new_file_mode; + let _ = required_file_mode; if let Some(parent) = path.parent() { fs::create_dir_all(parent).map_err(|e| AppError::io(parent, e))?; } @@ -334,7 +335,7 @@ pub(crate) fn atomic_write_durable( #[cfg(unix)] { use std::os::unix::fs::OpenOptionsExt; - options.mode(new_file_mode.unwrap_or(0o666)); + options.mode(required_file_mode.unwrap_or(0o666)); } let mut file = options .open(&tmp) @@ -342,18 +343,20 @@ pub(crate) fn atomic_write_durable( file.write_all(data) .map_err(|error| AppError::io(&tmp, error))?; file.flush().map_err(|error| AppError::io(&tmp, error))?; - file.sync_all().map_err(|error| AppError::io(&tmp, error))?; - drop(file); #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; - let mode = fs::metadata(path) - .map(|metadata| metadata.permissions().mode()) - .unwrap_or_else(|_| new_file_mode.unwrap_or(0o666)); - fs::set_permissions(&tmp, fs::Permissions::from_mode(mode)) + let mode = required_file_mode.unwrap_or_else(|| { + fs::metadata(path) + .map(|metadata| metadata.permissions().mode()) + .unwrap_or(0o666) + }); + file.set_permissions(fs::Permissions::from_mode(mode)) .map_err(|error| AppError::io(&tmp, error))?; } + file.sync_all().map_err(|error| AppError::io(&tmp, error))?; + drop(file); replace_file_atomically(&tmp, path)?; #[cfg(unix)] @@ -560,6 +563,30 @@ mod tests { ); } + #[cfg(unix)] + #[test] + fn sensitive_atomic_write_tightens_an_existing_file_before_publish() { + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().expect("tempdir"); + let path = temp.path().join("settings.json"); + fs::write(&path, b"old").expect("seed settings"); + fs::set_permissions(&path, fs::Permissions::from_mode(0o644)) + .expect("make legacy settings permissive"); + + atomic_write_durable(&path, b"new-secret", Some(0o600)).expect("replace settings"); + + assert_eq!(fs::read(&path).expect("read settings"), b"new-secret"); + assert_eq!( + fs::metadata(&path) + .expect("settings metadata") + .permissions() + .mode() + & 0o777, + 0o600 + ); + } + #[test] fn sort_json_keys_produces_identical_output_for_different_insertion_orders() { // 核心保证：同一逻辑配置无论键的插入顺序如何，写出的字节序列必须一致。 diff --git a/src-tauri/src/services/pi_prompt_files.rs b/src-tauri/src/services/pi_prompt_files.rs index 5b1520011..871f25594 100644 --- a/src-tauri/src/services/pi_prompt_files.rs +++ b/src-tauri/src/services/pi_prompt_files.rs @@ -285,21 +285,60 @@ fn validate_direct_instruction_content(content: &str) -> Result<(), AppError> { } fn validate_template_slug(slug: &str) -> Result<(), AppError> { + // Pinned Pi's request-capture discovers `release notes.md`, but + // expandPromptTemplate("/release notes", ...) leaves the command + // unchanged because slash-command names are one token. Keep the managed + // namespace both callable and portable across Unix and Windows. + let windows_basename = slug + .split_once('.') + .map_or(slug, |(basename, _extension)| basename); + let windows_basename = windows_basename.to_ascii_lowercase(); + let windows_reserved = matches!( + windows_basename.as_str(), + "con" + | "prn" + | "aux" + | "nul" + | "com1" + | "com2" + | "com3" + | "com4" + | "com5" + | "com6" + | "com7" + | "com8" + | "com9" + | "lpt1" + | "lpt2" + | "lpt3" + | "lpt4" + | "lpt5" + | "lpt6" + | "lpt7" + | "lpt8" + | "lpt9" + ); let valid = !slug.is_empty() && slug.len() <= MAX_TEMPLATE_SLUG_BYTES && slug != "." && slug != ".." - && slug.trim() == slug && !slug.starts_with('.') && !slug.ends_with('.') - && !slug - .chars() - .any(|character| character.is_control() || matches!(character, '/' | '\\')); + && !windows_reserved + && !slug.chars().any(|character| { + character.is_control() + || character.is_whitespace() + || matches!( + character, + '<' | '>' | ':' | '"' | '/' | '\\' | '|' | '?' | '*' + ) + }); if valid { Ok(()) } else { Err(AppError::InvalidInput( - "Pi prompt-template slug must be one visible filename (1-128 UTF-8 bytes)".to_string(), + "Pi prompt-template slug must be one portable slash-command token (1-128 UTF-8 bytes)" + .to_string(), )) } } @@ -408,8 +447,16 @@ mod tests { ".hidden", "trailing.", " padded", + "internal space", + "tab\tname", "a/b", r"a\b", + "bad:name", + "bad*name", + "CON", + "con.anything", + "LPT9", + "nul.json", ] { assert!(validate_template_slug(slug).is_err(), "{slug:?}"); } diff --git a/src-tauri/src/services/skill_deployment.rs b/src-tauri/src/services/skill_deployment.rs index 3d7334151..7fff183d3 100644 --- a/src-tauri/src/services/skill_deployment.rs +++ b/src-tauri/src/services/skill_deployment.rs @@ -141,6 +141,23 @@ impl PiSkillDeploymentService { deployment_lock() } + /// Serialize a portable database/SSOT replacement with every Pi Skill + /// deployment mutation. Callers already hold Pi's switch boundary, so the + /// global lock order remains `Pi switch -> Skill deployment`. + pub(crate) fn coordinate_portable_import( + operation: impl FnOnce() -> Result, + ) -> Result { + let _guard = Self::operation_guard(); + operation() + } + + pub(crate) fn import_portable_sql( + db: &Database, + source_path: &Path, + ) -> Result { + Self::coordinate_portable_import(|| db.import_portable_sql(source_path)) + } + pub(crate) fn reconcile_skill_under_guard( _guard: &MutexGuard<'static, ()>, db: &Arc, @@ -1126,6 +1143,44 @@ mod tests { use super::*; use crate::app_config::SkillApps; + #[test] + #[serial_test::serial] + fn portable_import_waits_for_the_skill_ownership_boundary() { + use std::sync::mpsc; + use std::time::Duration; + + let db = Database::memory().expect("database"); + let missing = tempfile::tempdir() + .expect("tempdir") + .path() + .join("missing.sql"); + let guard = PiSkillDeploymentService::operation_guard(); + let (ready_tx, ready_rx) = mpsc::channel(); + let (result_tx, result_rx) = mpsc::channel(); + let worker = std::thread::spawn(move || { + ready_tx.send(()).expect("signal worker ready"); + let result = PiSkillDeploymentService::import_portable_sql(&db, &missing); + result_tx.send(result).expect("signal import result"); + }); + + ready_rx + .recv_timeout(Duration::from_secs(2)) + .expect("worker reaches import entry"); + assert!( + result_rx.recv_timeout(Duration::from_millis(100)).is_err(), + "portable import must not pass capture/publish while a Skill mutation owns the boundary" + ); + drop(guard); + let result = result_rx + .recv_timeout(Duration::from_secs(2)) + .expect("import proceeds after ownership boundary release"); + assert!( + result.is_err(), + "the intentionally missing import must fail" + ); + worker.join().expect("worker"); + } + #[test] fn digest_includes_hidden_files_and_rejects_symlinks() { let temp = tempfile::tempdir().expect("tempdir"); diff --git a/src-tauri/src/services/sync_protocol.rs b/src-tauri/src/services/sync_protocol.rs index 4de9bb148..80dda79fe 100644 --- a/src-tauri/src/services/sync_protocol.rs +++ b/src-tauri/src/services/sync_protocol.rs @@ -310,6 +310,16 @@ pub(crate) fn apply_snapshot( db: &crate::database::Database, db_sql: &[u8], skills_zip: &[u8], +) -> Result<(), AppError> { + crate::services::skill_deployment::PiSkillDeploymentService::coordinate_portable_import(|| { + apply_snapshot_under_pi_skill_guard(db, db_sql, skills_zip) + }) +} + +fn apply_snapshot_under_pi_skill_guard( + db: &crate::database::Database, + db_sql: &[u8], + skills_zip: &[u8], ) -> Result<(), AppError> { let sql_str = std::str::from_utf8(db_sql).map_err(|e| { localized( @@ -419,6 +429,37 @@ where mod tests { use super::*; + #[test] + #[serial_test::serial] + fn snapshot_application_waits_for_the_pi_skill_ownership_boundary() { + use std::sync::mpsc; + use std::time::Duration; + + let db = crate::database::Database::memory().expect("database"); + let guard = crate::services::skill_deployment::PiSkillDeploymentService::operation_guard(); + let (ready_tx, ready_rx) = mpsc::channel(); + let (result_tx, result_rx) = mpsc::channel(); + let worker = std::thread::spawn(move || { + ready_tx.send(()).expect("signal worker ready"); + let result = apply_snapshot(&db, &[0xff], &[]); + result_tx.send(result).expect("signal snapshot result"); + }); + + ready_rx + .recv_timeout(Duration::from_secs(2)) + .expect("worker reaches snapshot entry"); + assert!( + result_rx.recv_timeout(Duration::from_millis(100)).is_err(), + "WebDAV/S3 snapshot application must wait for a concurrent Pi Skill mutation" + ); + drop(guard); + let result = result_rx + .recv_timeout(Duration::from_secs(2)) + .expect("snapshot proceeds after ownership boundary release"); + assert!(result.is_err(), "the intentionally invalid SQL must fail"); + worker.join().expect("worker"); + } + fn artifact(sha256: &str, size: u64) -> ArtifactMeta { ArtifactMeta { sha256: sha256.to_string(), diff --git a/src-tauri/src/settings.rs b/src-tauri/src/settings.rs index f7d090a7d..0aa040ff6 100644 --- a/src-tauri/src/settings.rs +++ b/src-tauri/src/settings.rs @@ -1373,6 +1373,55 @@ mod tests { use super::*; use crate::app_config::AppType; + #[cfg(unix)] + #[test] + #[serial_test::serial] + fn saving_a_gateway_token_tightens_legacy_settings_permissions() { + use std::os::unix::fs::PermissionsExt; + + struct EnvGuard(Option); + impl Drop for EnvGuard { + fn drop(&mut self) { + match self.0.take() { + Some(value) => std::env::set_var("CC_SWITCH_TEST_HOME", value), + None => std::env::remove_var("CC_SWITCH_TEST_HOME"), + } + } + } + + let temp = tempfile::tempdir().expect("tempdir"); + let _home = EnvGuard(std::env::var_os("CC_SWITCH_TEST_HOME")); + std::env::set_var("CC_SWITCH_TEST_HOME", temp.path()); + let path = AppSettings::settings_path().expect("settings path"); + fs::create_dir_all(path.parent().expect("settings parent")).expect("settings directory"); + fs::write(&path, b"{}").expect("legacy settings"); + fs::set_permissions(&path, fs::Permissions::from_mode(0o644)) + .expect("make legacy settings permissive"); + + let token = GatewayToken::generate(); + let expected = token.expose().to_string(); + let settings = AppSettings { + pi_gateway_token: Some(token), + ..AppSettings::default() + }; + save_settings_file(&settings).expect("save sensitive settings"); + + assert_eq!( + fs::metadata(&path) + .expect("settings metadata") + .permissions() + .mode() + & 0o777, + 0o600 + ); + assert!( + fs::read_to_string(&path) + .expect("settings body") + .contains(&expected), + "the permission assertion must exercise a persisted Pi gateway token" + ); + } + #[test] fn visible_apps_old_settings_default_claude_desktop_visible() { let visible: VisibleApps = serde_json::from_value(serde_json::json!({ diff --git a/src/components/prompts/PiNativePromptResources.tsx b/src/components/prompts/PiNativePromptResources.tsx index bdaadd40d..b81015cc8 100644 --- a/src/components/prompts/PiNativePromptResources.tsx +++ b/src/components/prompts/PiNativePromptResources.tsx @@ -14,6 +14,7 @@ import { type PiPromptFileSnapshot, type PiPromptTemplate, } from "@/lib/api/prompts"; +import { isValidPiPromptTemplateSlug } from "@/lib/piPromptSlug"; import { extractErrorMessage } from "@/utils/errorUtils"; const EDITABLE_FILES: Array<{ @@ -304,13 +305,14 @@ export function PiNativePromptResources() { const queryClient = useQueryClient(); const [slug, setSlug] = useState(""); const [content, setContent] = useState(""); + const slugIsValid = isValidPiPromptTemplateSlug(slug); const templates = useQuery({ queryKey: ["pi", "promptTemplates"], queryFn: () => promptsApi.listPiPromptTemplates(), }); const createTemplate = useMutation({ mutationFn: () => - promptsApi.upsertPiPromptTemplate(slug.trim(), "missing", content), + promptsApi.upsertPiPromptTemplate(slug, "missing", content), onSuccess: async () => { setSlug(""); setContent(""); @@ -383,7 +385,13 @@ export function PiNativePromptResources() { value={slug} onChange={(event) => setSlug(event.target.value)} placeholder={t("pi.prompts.templateSlug")} + aria-invalid={slug.length > 0 && !slugIsValid} /> + {slug.length > 0 && !slugIsValid && ( +

+ {t("pi.prompts.templateSlugInvalid")} +

+ )}