From 93b811fd8d007e2aa715b458194cfdc35a0bb1d6 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Thu, 13 Aug 2026 13:02:59 +0200 Subject: [PATCH 01/10] fix: git sync missed metadata-only deploys, deploy check missed job link (#10662) * fix: git sync missed metadata-only deploys, deploy check missed job link Co-Authored-By: Claude Opus 5 (1M context) * fix: skip the deploy hook when the mute toggle matched no row Co-Authored-By: Claude Opus 5 (1M context) * chore: pin ee ref forward of main so the bump only adds this change Co-Authored-By: Claude Opus 5 (1M context) * chore: update ee-repo-ref to a65162b22b127b54c0686095ee1b16b04e3111f7 This commit updates the EE repository reference after PR #724 was merged in windmill-ee-private. Previous ee-repo-ref: ac5f646c3ace7e5841200c6b83b34fb4371340d9 New ee-repo-ref: a65162b22b127b54c0686095ee1b16b04e3111f7 Automated by sync-ee-ref workflow. --------- Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: windmill-internal-app[bot] --- backend/ee-repo-ref.txt | 2 +- backend/windmill-api-flows/src/flows.rs | 38 ++++++++++++++- backend/windmill-api-groups/src/folders.rs | 26 ++++++++++ backend/windmill-api-scripts/src/scripts.rs | 35 +++++++++++++- .../windmill-worker/src/result_processor.rs | 48 +++++++++++++++---- 5 files changed, 136 insertions(+), 13 deletions(-) diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 436566218b..6f6c280541 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -60c20e686cead73ff075512b15c6e2d6232beca6 +a65162b22b127b54c0686095ee1b16b04e3111f7 diff --git a/backend/windmill-api-flows/src/flows.rs b/backend/windmill-api-flows/src/flows.rs index 71b98b22bd..3c37523082 100644 --- a/backend/windmill-api-flows/src/flows.rs +++ b/backend/windmill-api-flows/src/flows.rs @@ -381,6 +381,7 @@ async fn toggle_workspace_error_handler( async fn toggle_workspace_error_handler( authed: ApiAuthed, Extension(user_db): Extension, + Extension(db): Extension, Path((w_id, path)): Path<(String, StripPath)>, Json(req): Json, ) -> Result { @@ -401,9 +402,10 @@ async fn toggle_workspace_error_handler( .await? .unwrap_or(None); + let mut updated_rows = 0; let response = match error_handler_maybe { Some(_) => { - sqlx::query_scalar!( + updated_rows = sqlx::query_scalar!( r#" UPDATE flow @@ -418,7 +420,8 @@ async fn toggle_workspace_error_handler( req.muted, ) .execute(&mut *tx) - .await?; + .await? + .rows_affected(); Ok("".to_string()) } None => Err(Error::BadRequest( @@ -428,6 +431,37 @@ async fn toggle_workspace_error_handler( tx.commit().await?; + // `ws_error_handler_muted` is part of the synced flow metadata, so the + // toggle is a deploy like any other edit of it. The version is a + // placeholder: git sync keys off the path and kind alone. The update runs + // under RLS against an unchecked path, so it can match nothing — deploy + // only what it actually wrote. + if updated_rows > 0 { + handle_deployment_metadata( + &authed.email, + &authed.username, + &db, + &w_id, + DeployedObject::Flow { + path: path.to_path().to_string(), + parent_path: None, + version: 0, + }, + Some(format!( + "Flow '{}' {} the workspace error handler", + path.to_path(), + if req.muted.unwrap_or(false) { + "muted" + } else { + "unmuted" + } + )), + true, + None, + ) + .await?; + } + return response; } diff --git a/backend/windmill-api-groups/src/folders.rs b/backend/windmill-api-groups/src/folders.rs index 8736f7b60f..646f764ec6 100644 --- a/backend/windmill-api-groups/src/folders.rs +++ b/backend/windmill-api-groups/src/folders.rs @@ -851,6 +851,7 @@ async fn delete_folder( async fn add_owner( authed: ApiAuthed, Extension(user_db): Extension, + Extension(db): Extension, Extension(webhook): Extension, Path((w_id, name)): Path<(String, String)>, Json(Owner { owner, .. }): Json, @@ -905,6 +906,18 @@ async fn add_owner( tx.commit().await?; + handle_deployment_metadata( + &authed.email, + &authed.username, + &db, + &w_id, + DeployedObject::Folder { path: format!("f/{}", name) }, + Some(format!("Folder '{}' changed permissions", name)), + true, + None, + ) + .await?; + webhook.send_message( w_id.clone(), WebhookMessage::UpdateFolder { workspace: w_id, name: name.clone() }, @@ -916,6 +929,7 @@ async fn add_owner( async fn remove_owner( authed: ApiAuthed, Extension(user_db): Extension, + Extension(db): Extension, Extension(webhook): Extension, Path((w_id, name)): Path<(String, String)>, Json(Owner { owner, write }): Json, @@ -999,6 +1013,18 @@ async fn remove_owner( tx.commit().await?; + handle_deployment_metadata( + &authed.email, + &authed.username, + &db, + &w_id, + DeployedObject::Folder { path: format!("f/{}", name) }, + Some(format!("Folder '{}' changed permissions", name)), + true, + None, + ) + .await?; + webhook.send_message( w_id.clone(), WebhookMessage::UpdateFolder { workspace: w_id, name: name.clone() }, diff --git a/backend/windmill-api-scripts/src/scripts.rs b/backend/windmill-api-scripts/src/scripts.rs index ef33169156..3f23d0416f 100644 --- a/backend/windmill-api-scripts/src/scripts.rs +++ b/backend/windmill-api-scripts/src/scripts.rs @@ -2827,6 +2827,7 @@ async fn toggle_workspace_error_handler( async fn toggle_workspace_error_handler( authed: ApiAuthed, Extension(user_db): Extension, + Extension(db): Extension, Path((w_id, path)): Path<(String, StripPath)>, Json(req): Json, ) -> Result { @@ -2842,7 +2843,7 @@ async fn toggle_workspace_error_handler( match error_handler_maybe { Some(_) => { - sqlx::query_scalar!( + let updated = sqlx::query_scalar!( "UPDATE script SET ws_error_handler_muted = $3 WHERE ctid = ( @@ -2859,6 +2860,38 @@ async fn toggle_workspace_error_handler( .execute(&mut *tx) .await?; tx.commit().await?; + + // `ws_error_handler_muted` is part of the synced script metadata, so + // the toggle is a deploy like any other edit of it. The hash is a + // placeholder: git sync keys off the path and kind alone. The update + // runs under RLS against an unchecked path, so it can match nothing — + // deploy only what it actually wrote. + if updated.rows_affected() > 0 { + handle_deployment_metadata( + &authed.email, + &authed.username, + &db, + &w_id, + DeployedObject::Script { + hash: ScriptHash(0), + path: path.to_path().to_string(), + parent_path: None, + }, + Some(format!( + "Script '{}' {} the workspace error handler", + path.to_path(), + if req.muted.unwrap_or(false) { + "muted" + } else { + "unmuted" + } + )), + true, + None, + ) + .await?; + } + Ok("".to_string()) } None => { diff --git a/backend/windmill-worker/src/result_processor.rs b/backend/windmill-worker/src/result_processor.rs index 5d00a4e515..082a8dc67c 100644 --- a/backend/windmill-worker/src/result_processor.rs +++ b/backend/windmill-worker/src/result_processor.rs @@ -884,6 +884,14 @@ fn parse_pr_check_error(result_raw: &str) -> Option { }) } +/// The run page for `job_id`, or `None` when the instance has no `BASE_URL` set +/// (it defaults to empty) — a check must not carry a link that goes nowhere. +#[cfg(all(feature = "enterprise", feature = "private"))] +fn job_run_url(base_url: &str, job_id: &uuid::Uuid, workspace_id: &str) -> Option { + let base = base_url.trim_end_matches('/'); + (!base.is_empty()).then(|| format!("{base}/run/{job_id}?workspace={workspace_id}")) +} + #[cfg(all(feature = "enterprise", feature = "private"))] fn format_change_list(changes: &[(String, String)]) -> Vec { let mut lines = Vec::new(); @@ -898,7 +906,19 @@ fn format_change_list(changes: &[(String, String)]) -> Vec { #[cfg(all(test, feature = "enterprise", feature = "private"))] mod git_sync_check_tests { - use super::{format_change_list, parse_git_sync_changes, parse_pr_check_error}; + use super::{format_change_list, job_run_url, parse_git_sync_changes, parse_pr_check_error}; + + #[test] + fn job_run_url_is_none_without_a_base_url() { + let id = uuid::Uuid::nil(); + assert_eq!( + job_run_url("https://app.windmill.dev/", &id, "w").as_deref(), + Some("https://app.windmill.dev/run/00000000-0000-0000-0000-000000000000?workspace=w") + ); + // BASE_URL defaults to empty; a link built from it would 404 the reader. + assert_eq!(job_run_url("", &id, "w"), None); + assert_eq!(job_run_url("/", &id, "w"), None); + } #[test] fn pr_check_error_is_a_field_not_a_substring() { @@ -1376,6 +1396,10 @@ async fn maybe_post_git_sync_check( } else { None }; + // The creating call could only link the check to the workspace's run list — + // the check predates the job fulfilling it. Now that the job is known, point + // both the summary and the check's "Details" link at its logs. + let job_url = job_run_url(&windmill_common::BASE_URL.load(), job_id, workspace_id); let (conclusion, title, summary): (&str, String, String) = if is_deploy { // Phase 6: real deploy pull -> "Deployed N changes" / "In sync" / failure. @@ -1383,8 +1407,7 @@ async fn maybe_post_git_sync_check( ( "failure", format!("Deploy to {} failed", workspace_id), - "Deploying the latest commit failed. See the job in Windmill for details." - .to_string(), + "Deploying the latest commit failed.".to_string(), ) } else { match parse_git_sync_changes(result_raw) { @@ -1447,15 +1470,13 @@ async fn maybe_post_git_sync_check( ( "failure", "Windmill diff failed".to_string(), - "The dry-run pull reported an unrecognized error. See the job in Windmill for details." - .to_string(), + "The dry-run pull reported an unrecognized error.".to_string(), ) } else if !success { ( "failure", "Windmill diff failed".to_string(), - "The dry-run pull to compute the diff failed. See the job in Windmill for details." - .to_string(), + "The dry-run pull to compute the diff failed.".to_string(), ) } else { match parse_git_sync_changes(result_raw) { @@ -1495,6 +1516,10 @@ async fn maybe_post_git_sync_check( } }; + let check_summary = match job_url.as_deref() { + Some(url) => format!("{summary}\n\n[See the job in Windmill]({url})"), + None => summary.clone(), + }; if let Err(e) = windmill_common::git_sync_ee::update_check_run( db, workspace_id, @@ -1502,7 +1527,8 @@ async fn maybe_post_git_sync_check( check.check_run_id, conclusion, &title, - &summary, + &check_summary, + job_url.as_deref(), ) .await { @@ -1520,8 +1546,12 @@ async fn maybe_post_git_sync_check( .as_deref() .map(|s| &s[..s.len().min(7)]) .unwrap_or("latest"); + let job_row = job_url + .as_deref() + .map(|url| format!("\n| **Job** | [See the logs]({url}) |")) + .unwrap_or_default(); let body = format!( - "{marker}\n### Windmill deploy preview\n\n| | |\n|---|---|\n| **Workspace** | `{workspace_id}` |\n| **Status** | {title} |\n| **Commit** | `{head}` |\n\n
Details\n\n{summary}\n\n
" + "{marker}\n### Windmill deploy preview\n\n| | |\n|---|---|\n| **Workspace** | `{workspace_id}` |\n| **Status** | {title} |\n| **Commit** | `{head}` |{job_row}\n\n
Details\n\n{summary}\n\n
" ); if let Err(e) = windmill_common::git_sync_ee::upsert_pr_comment( db, From ef99a739dda73fba60df011e34981c2cb5e23a3c Mon Sep 17 00:00:00 2001 From: hugocasa Date: Thu, 13 Aug 2026 13:03:42 +0200 Subject: [PATCH 02/10] fix(github-app): complete the self-managed setup instructions, render the page header (#10683) * docs(github-app): state the pull-direction permissions and the App owner field The in-product "How to create a GitHub App" panel only listed Contents and Metadata, which covers the push direction of git sync. Webhooks, pull requests and checks are what the git to Windmill direction needs, and a GHE Cloud (*.ghe.com) app also needs App owner, whose field hint was the only place saying so. Co-Authored-By: Claude Opus 5 (1M context) * fix(instance-settings): render the GitHub App page header The branch tested the pre-rename category name, so the page rendered with no header at all. Naming the header after the category duplicates the card below it, so the card that holds the app credentials is now labelled for what it is, next to the webhook base url card. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../lib/components/InstanceSettings.svelte | 7 ++-- .../src/lib/components/instanceSettings.ts | 6 ++-- .../instanceSettings/GhesAppSettings.svelte | 36 +++++++++++++++++-- 3 files changed, 42 insertions(+), 7 deletions(-) diff --git a/frontend/src/lib/components/InstanceSettings.svelte b/frontend/src/lib/components/InstanceSettings.svelte index bbe5432f06..0a21e8897e 100644 --- a/frontend/src/lib/components/InstanceSettings.svelte +++ b/frontend/src/lib/components/InstanceSettings.svelte @@ -1144,10 +1144,11 @@ description="Configure where secrets (secret variables) are stored." link="https://www.windmill.dev/docs/core_concepts/workspace_secret_encryption" /> - {:else if category == 'GitHub Enterprise App'} + {:else if category == 'GitHub App'} {:else if category == 'DB Health'} = { ], 'GitHub App': [ { - label: 'GitHub App', + // The category header above already names the section; this labels the + // card that holds the app credentials, next to the webhook base url one. + label: 'App configuration', description: - 'Configure a self-managed GitHub App to enable git sync without stats.windmill.dev.', + 'Use your own GitHub App instead of the Windmill-managed one on stats.windmill.dev.', key: 'github_enterprise_app', fieldType: 'github_enterprise_app', storage: 'setting', diff --git a/frontend/src/lib/components/instanceSettings/GhesAppSettings.svelte b/frontend/src/lib/components/instanceSettings/GhesAppSettings.svelte index ae5347d278..da63e8d64a 100644 --- a/frontend/src/lib/components/instanceSettings/GhesAppSettings.svelte +++ b/frontend/src/lib/components/instanceSettings/GhesAppSettings.svelte @@ -194,13 +194,34 @@
  • Callback URL: <your-windmill-url>/gh_success
    • -
  • Uncheck Active under Webhook (not needed)
    • +
  • + Uncheck Active under Webhook. Windmill registers the webhooks it + needs per repository, so the app-level webhook stays unused. +

    • 3. Set repository permissions:

    • Contents: Read & write
    • Metadata: Read-only
    +

    + Those two are the minimum, for the push direction (Windmill → git). Add these for + the pull direction (git → Windmill), all read & write: +

    +
      +
    • + Repository webhooks: deploy commits within seconds instead of + polling the repository +
      • +
    • + Pull requests: open pull requests for the branches Windmill pushes, + and maintain the deploy-preview comment +
      • +
    • + Checks: post the "Windmill diff" and deploy status checks on commits + and pull requests +
      • +

    4. Under "Where can this GitHub App be installed?", choose Any account (or restrict to your organization). @@ -219,7 +240,18 @@

    8. The Base URL is your GitHub instance root (e.g. - https://github.com or https://github.mycompany.com). + https://github.com, https://mycompany.ghe.com or + https://github.mycompany.com). On GHE Cloud (*.ghe.com), also + set App owner to the organization or user that owns the app: its + installation urls carry the owner. +

    +

    + Full setup guide: Self-managed GitHub App.

    From 6fbc3fccb607a8d80885f0face284c01871a3162 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Thu, 13 Aug 2026 13:03:55 +0200 Subject: [PATCH 03/10] fix(flow): pass the flow's worker tag when testing a loop iteration (#10680) --- frontend/src/lib/components/FlowLoopIterationPreview.svelte | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/frontend/src/lib/components/FlowLoopIterationPreview.svelte b/frontend/src/lib/components/FlowLoopIterationPreview.svelte index 5bea6bb6ff..1378e20f04 100644 --- a/frontend/src/lib/components/FlowLoopIterationPreview.svelte +++ b/frontend/src/lib/components/FlowLoopIterationPreview.svelte @@ -89,7 +89,7 @@ runPreview(previewArgs, undefined) } - const { flowStateStore, pathStore, opWorkspace } = + const { flowStateStore, flowStore, pathStore, opWorkspace } = getContext('FlowEditorContext') const dispatch = createEventDispatcher() @@ -98,7 +98,9 @@ restartedFrom: RestartedFrom | undefined ) { progressBar?.reset() - const newFlow = { value: { modules }, summary: '' } + // The preview flow holds only the loop body, so it inherits none of the flow's settings: + // carry the tag over so the iteration lands on the worker group the flow runs on. + const newFlow = { value: { modules }, summary: '', tag: flowStore.val.tag } jobId = await runFlowPreview( whileLoop ? withWhileLoopIter(args) : args, newFlow, From a91d55769d7ea4088d056256389214d46439bdc4 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Thu, 13 Aug 2026 13:04:12 +0200 Subject: [PATCH 04/10] chore: pin git-sync scripts to hub 28903/28904 (cli 1.787.0) (#10682) --- backend/windmill-common/src/workspaces.rs | 4 ++-- frontend/src/lib/hubPaths.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index d570e7ec32..7ba930c3ea 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -175,7 +175,7 @@ pub enum ObjectType { DatatableMigration, } -pub const LATEST_GIT_SYNC_SCRIPT_PATH: &str = "hub/28871/sync-script-to-git-repo-windmill"; +pub const LATEST_GIT_SYNC_SCRIPT_PATH: &str = "hub/28904/sync-script-to-git-repo-windmill"; /// Hub script that applies a repository's state back into a workspace /// (the repo → Windmill / "pull" direction). Same script the UI runs from @@ -183,7 +183,7 @@ pub const LATEST_GIT_SYNC_SCRIPT_PATH: &str = "hub/28871/sync-script-to-git-repo /// ignores the slug, so the slug is kept free of characters that would be /// percent-encoded into the run URL (a `:` becomes `%3A`, which some hardened /// reverse proxies reject as double-encoding when the client re-encodes it). -pub const GIT_SYNC_PULL_SCRIPT_PATH: &str = "hub/28890/git-sync-init-repository-windmill"; +pub const GIT_SYNC_PULL_SCRIPT_PATH: &str = "hub/28903/git-sync-init-repository-windmill"; /// Prefix used to identify fork workspaces. A workspace whose id starts with this string is a /// fork of another workspace. diff --git a/frontend/src/lib/hubPaths.json b/frontend/src/lib/hubPaths.json index 1185d0ef93..171e2f7fa9 100644 --- a/frontend/src/lib/hubPaths.json +++ b/frontend/src/lib/hubPaths.json @@ -1,6 +1,6 @@ { "gitSyncTest": "hub/28184/git-repo-test-read-write-windmill", - "gitInitRepo": "hub/28890/git-sync-init-repository-windmill", + "gitInitRepo": "hub/28903/git-sync-init-repository-windmill", "slackErrorHandler": "hub/28794/workspace-or-schedule-error-handler-slack", "emailErrorHandler": "hub/19795/workspace-or-error-handler-email", "slackRecoveryHandler": "hub/28791/slack/schedule-recovery-handler-slack", From 2714210d7c74aa9375ecbb8742e16d007d991ea4 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Thu, 13 Aug 2026 13:06:55 +0200 Subject: [PATCH 05/10] fix: expand AZURE_DEVOPS_TOKEN placeholder in backend git probes (#10677) * fix: expand AZURE_DEVOPS_TOKEN placeholder in backend git probes * fix: require azure token placeholder to be http userinfo * fix: scrub probe credentials from git stderr and harden token mint * fix: confine azure token placeholder to azure devops hosts * fix: require https and authorize azure reference at write time * fix: require workspace admin to configure an azure token reference * fix: name the azure reference in the admin-required error --- backend/tests/asset_trigger_dispatch.rs | 1 + backend/windmill-store/src/resources.rs | 565 ++++++++++++++++++++++-- 2 files changed, 526 insertions(+), 40 deletions(-) diff --git a/backend/tests/asset_trigger_dispatch.rs b/backend/tests/asset_trigger_dispatch.rs index 7184d4810d..c165958a33 100644 --- a/backend/tests/asset_trigger_dispatch.rs +++ b/backend/tests/asset_trigger_dispatch.rs @@ -244,6 +244,7 @@ fn make_mini(id: Uuid, runnable_path: &str) -> MiniCompletedJob { cache_ttl: None, cache_ignore_s3_path: None, runnable_settings_handle: None, + build_binary_only: false, } } diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index 1dcfa6429e..a3b5d2a1a7 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -1140,6 +1140,19 @@ async fn create_resource( } let authed = maybe_refresh_folders(&resource.path, &w_id, authed, &db).await; + authorize_azure_devops_reference( + &authed, + &db, + &user_db, + &w_id, + resource + .value + .as_deref() + .and_then(|v| serde_json::from_str::(v.get()).ok()) + .as_ref(), + ) + .await?; + let mut tx = user_db.begin(&authed).await?; let update_if_exists = q.update_if_exists.unwrap_or(false); @@ -1821,6 +1834,18 @@ async fn update_resource( sqlb.returning("path"); let authed = maybe_refresh_folders(path, &w_id, authed, &db).await; + authorize_azure_devops_reference( + &authed, + &db, + &user_db, + &w_id, + ns.value + .as_deref() + .and_then(|v| serde_json::from_str::(v.get()).ok()) + .as_ref(), + ) + .await?; + let mut tx = user_db.begin(&authed).await?; if let Some(npath) = ns.path.clone() { @@ -2098,6 +2123,8 @@ async fn set_resource_value( { return Err(Error::PermissionDenied(msg)); } + authorize_azure_devops_reference(authed, db, user_db, w_id, value.as_ref()).await?; + let mut tx = user_db.clone().begin(authed).await?; // `RETURNING resource_type` rather than a second lookup: the advisory below has to know the @@ -2954,6 +2981,50 @@ fn extract_host_from_git_url(url: &str) -> Option { None } +/// Strip the userinfo from a git URL. These probes run against URLs that embed a +/// credential (a `$var:` token, or one minted from an `AZURE_DEVOPS_TOKEN(...)` +/// placeholder), and their errors are persisted as the repository's sync status and +/// rendered in the UI. git's own redaction cannot be relied on — it drops the userinfo +/// from `unable to access ''` but echoes it in `could not read Password for +/// ''` — so anything that formats a probe URL has to strip it here. +fn redact_git_url_credentials(url: &str) -> String { + match git_url_userinfo_range(url) { + Some(r) => format!("{}***{}", &url[..r.start], &url[r.end..]), + None => url.to_string(), + } +} + +/// Byte range of a git URL's userinfo (the credentials before the authority's '@'), +/// for both `scheme://user[:pass]@host/path` and SCP-style `user@host:path`. +/// +/// The authority ends at the first '/', '?' or '#' and the credentials are the *last* +/// '@' within it, so an '@' planted in the path cannot mis-scope the split +/// (GHSA-p5cj-8cfh-mjv6). +fn git_url_userinfo_range(url: &str) -> Option> { + let (authority_start, authority) = match url.find("://") { + Some(scheme_sep) => { + let start = scheme_sep + 3; + let after = &url[start..]; + let end = after + .find(|c| c == '/' || c == '?' || c == '#') + .unwrap_or(after.len()); + (start, &after[..end]) + } + // SCP-style `[user@]host:path` has no scheme, and its authority is bounded by + // the first ':' — never by the last '@', which an '@' in the path would move + // (the same mis-scoping `extract_host_from_git_url` guards against). scp syntax + // has no password field, so bounding this way cannot cut a credential in half. + None if url.contains('@') => (0, url.split(':').next().unwrap_or(url)), + None => return None, + }; + let at = authority.rfind('@')?; + (at > 0).then(|| authority_start..authority_start + at) +} + +fn git_url_userinfo(url: &str) -> Option<&str> { + git_url_userinfo_range(url).map(|r| &url[r]) +} + /// Validates a git URL to prevent option injection, SSRF, and local file read. async fn validate_git_url(url: &str) -> Result<()> { let url = url.trim(); @@ -2979,6 +3050,14 @@ async fn validate_git_url(url: &str) -> Result<()> { "Git URL cannot contain '?' or '#' characters".to_string(), )); } + // Every probe URL is validated, so this catches a caller that reached git without + // expanding the placeholder — which git would otherwise report as an unresolvable + // host, the placeholder's own '/' having truncated the authority. + if url.contains(AZURE_DEVOPS_TOKEN_PLACEHOLDER) { + return Err(Error::BadRequest( + "Git URL still contains an unexpanded AZURE_DEVOPS_TOKEN(...) placeholder".to_string(), + )); + } let lower = url.to_lowercase(); @@ -3112,12 +3191,14 @@ async fn get_git_commit_hash( .await .map_err(|e| Error::NotFound(format!("Access to resource {} denied: ({e})", path)))?; - let git_resource: GitRepositoryResource = match git_repo_resource_value { + let mut git_resource: GitRepositoryResource = match git_repo_resource_value { Some(value) => serde_json::from_value(value).map_err(|e| { Error::BadRequest(format!("Invalid git repository resource format: {}", e)) })?, None => return Err(Error::NotFound(format!("Resource {} not found", path)).into()), }; + git_resource.url = + resolve_azure_devops_url(&db_with_opt_authed, &w_id, &git_resource.url, false).await?; let identities: Vec = query .git_ssh_identity @@ -3259,9 +3340,21 @@ fn is_refused_redirect(stderr: &str) -> bool { /// Decode a failed probe's stderr, naming the remedy when the remote redirected /// somewhere the `.git` retry could not reach (an `http://` URL upgraded to https, /// say) — `git_probe_command` refuses redirects, so nothing else explains the status. -fn git_probe_stderr(stderr: Vec) -> String { +/// +/// `probe_url` is the URL the probe ran against, and its userinfo is scrubbed from the +/// output: git strips credentials from some messages but not all — a token in the +/// username position comes back verbatim in `could not read Password for +/// 'https://@host'` — and these strings are persisted as a repository's sync +/// status and rendered in the UI. +fn git_probe_stderr(stderr: Vec, probe_url: &str) -> String { let stderr = String::from_utf8(stderr).unwrap_or_else(|_| "Failed to decode stderr".to_string()); + // Scrub the `@` form git prints, not the bare userinfo: a one-character + // username would otherwise be replaced everywhere it happens to occur. + let stderr = match git_url_userinfo(probe_url) { + Some(userinfo) => stderr.replace(&format!("{userinfo}@"), "***@"), + None => stderr, + }; if is_refused_redirect(&stderr) { format!( "{} (the remote redirects, and redirects are not followed; set the repository URL to the address it redirects to)", @@ -3333,6 +3426,317 @@ async fn run_git_probe(mut git_cmd: Command, what: &str) -> Result)` +/// where a credential belongs: an Azure DevOps access token minted at use time from +/// that `azure` resource's client credentials. The hub sync scripts expand it in +/// TypeScript before running git; the probes below shell out to git from the backend, +/// so they must expand it too or git is handed the literal placeholder (whose '/' +/// truncates the authority, and curl rejects the resulting hostname). +const AZURE_DEVOPS_TOKEN_PLACEHOLDER: &str = "AZURE_DEVOPS_TOKEN("; + +/// Azure DevOps resource id the token is minted for, and the endpoint that mints it — +/// both identical to the hub sync scripts', so a repository that authenticates for a +/// sync job authenticates for these probes too. +const AZURE_DEVOPS_RESOURCE_ID: &str = "499b84ac-1321-427f-aa17-267ca6975798/.default"; +const AZURE_LOGIN_HOST: &str = "https://login.microsoftonline.com"; + +/// Minted tokens, keyed by a digest of the credentials they came from — never by +/// resource path, so a cache hit cannot hand a token to a caller who was not able to +/// read the resource itself. Auto-pull probes every repository on an interval; without +/// this, every tick would mint a fresh token. +static AZURE_DEVOPS_TOKEN_CACHE: LazyLock> = + LazyLock::new(DashMap::new); + +/// Shaved off a token's advertised lifetime so one is never handed out as it expires. +const AZURE_TOKEN_EXPIRY_MARGIN_S: i64 = 60; + +/// Lifetime assumed when the token response omits `expires_in`. +const AZURE_TOKEN_FALLBACK_LIFETIME_S: i64 = 300; + +/// Whether the span `start..end` of `url` is the userinfo of an https authority. +/// The placeholder contains '/', which truncates the authority for any left-to-right +/// parse, so terminators falling inside the span are skipped rather than honored. +/// +/// https only: over plaintext an on-path attacker answers the probe's first request +/// with a Basic challenge, and git retries carrying the minted token. +fn span_is_https_userinfo(url: &str, start: usize, end: usize) -> bool { + let Some(scheme_sep) = url.find("://") else { + return false; + }; + if !url[..scheme_sep].eq_ignore_ascii_case("https") { + return false; + } + let body_start = scheme_sep + 3; + if start < body_start { + return false; + } + let authority_end = url[body_start..] + .char_indices() + .map(|(i, c)| (body_start + i, c)) + .find(|&(i, c)| (i < start || i >= end) && (c == '/' || c == '?' || c == '#')) + .map_or(url.len(), |(i, _)| i); + // Taking the authority's *last* '@' is what makes one inside the span harmless: + // such a match sits before `end` and fails the comparison. + url[body_start..authority_end] + .rfind('@') + .is_some_and(|rel| end <= body_start + rel) +} + +/// Locate the placeholder in a git URL, returning `(whole placeholder, resource path)`. +fn parse_azure_devops_placeholder(url: &str) -> Result> { + let Some(start) = url.find(AZURE_DEVOPS_TOKEN_PLACEHOLDER) else { + return Ok(None); + }; + let after = &url[start + AZURE_DEVOPS_TOKEN_PLACEHOLDER.len()..]; + // Greedy to the last ')', matching the hub scripts' `AZURE_DEVOPS_TOKEN\((.+)\)`. + let end = after.rfind(')').ok_or_else(|| { + Error::BadRequest( + "Git repository URL has an unterminated AZURE_DEVOPS_TOKEN(...) placeholder" + .to_string(), + ) + })?; + let end = start + AZURE_DEVOPS_TOKEN_PLACEHOLDER.len() + end + 1; + // Anywhere but the userinfo, the minted token would be spliced into a part of the + // URL that git echoes verbatim in its failure messages (which are persisted as the + // repository's sync status) and that credential redaction does not cover. + if !span_is_https_userinfo(url, start, end) { + return Err(Error::BadRequest( + "The AZURE_DEVOPS_TOKEN(...) placeholder must be the credentials of an https git URL, i.e. directly before the '@'".to_string(), + )); + } + Ok(Some(( + &url[start..end], + &after[..end - start - AZURE_DEVOPS_TOKEN_PLACEHOLDER.len() - 1], + ))) +} + +/// Hosts an Azure DevOps token may be sent to. The minted token is an AAD token for +/// the Azure DevOps resource id, so Microsoft is the only party it is meaningful to. +fn is_azure_devops_host(host: &str) -> bool { + let host = host.trim_end_matches('.'); + host == "dev.azure.com" + || host.ends_with(".dev.azure.com") + || host == "visualstudio.com" + || host.ends_with(".visualstudio.com") +} + +/// Expand an `AZURE_DEVOPS_TOKEN(...)` placeholder in a git URL, or return the URL +/// unchanged when it has none. The referenced resource is read through `dba`, so an +/// authed caller only reaches credentials they can already read. +async fn resolve_azure_devops_url( + dba: &DbWithOptAuthed<'_, ApiAuthed>, + w_id: &str, + url: &str, + allow_cache: bool, +) -> Result { + // Trim first: the http(s) gates the callers apply trim too, so a stored URL with + // leading whitespace must not reach the scheme check here as a non-http one. + let url = url.trim(); + let Some((placeholder, resource_path)) = parse_azure_devops_placeholder(url)? else { + return Ok(url.to_string()); + }; + + // Vet the destination before minting: a URL the host checks would reject must not + // cost a live credential (nor cache one), and whoever can edit the URL would + // otherwise drive a token mint per poll tick. + let probe_url = url.replace(placeholder, "windmill"); + validate_git_url(&probe_url).await?; + + // The background poller reads the referenced resource under the system identity, + // which bypasses RLS. Confining the destination is what keeps that from becoming an + // exfiltration primitive: whoever can write this URL picks both the resource path + // and the host, so an unconfined splice would hand a credential they cannot read to + // a host they choose. Unlike `$var:`, which substitutes a whole value and so cannot + // place a secret inside a caller-chosen URL, this placeholder is a substring. + let host = extract_host_from_git_url(&probe_url) + .ok_or_else(|| Error::BadRequest("Could not parse hostname from git URL".to_string()))?; + if !is_azure_devops_host(&host) { + return Err(Error::BadRequest(format!( + "An AZURE_DEVOPS_TOKEN(...) placeholder is only allowed on an Azure DevOps URL (dev.azure.com or visualstudio.com), not '{host}'" + ))); + } + + let value = + get_resource_value_interpolated_internal(dba, w_id, resource_path, None, None, allow_cache) + .await + .map_err(|e| { + Error::BadRequest(format!( + "Azure resource '{resource_path}' referenced by the git repository URL could not be read: {e}" + )) + })? + .ok_or_else(|| { + Error::NotFound(format!( + "Azure resource '{resource_path}' referenced by the git repository URL was not found" + )) + })?; + + let field = |name: &str| -> Result { + value + .get(name) + .and_then(|v| v.as_str()) + .filter(|s| !s.is_empty()) + .map(|s| s.to_string()) + .ok_or_else(|| { + Error::BadRequest(format!( + "Azure resource '{resource_path}' referenced by the git repository URL has no '{name}'" + )) + }) + }; + let token = mint_azure_devops_token( + &field("azureTenantId")?, + &field("azureClientId")?, + &field("azureClientSecret")?, + allow_cache, + ) + .await?; + + Ok(url.replace(placeholder, &token)) +} + +/// Gate writing an `AZURE_DEVOPS_TOKEN(...)` reference into a resource value. +/// +/// The background probes mint from the named `azure` resource under the system identity, +/// which bypasses RLS, and no principal exists at that point to authorize against — so +/// authorization cannot be enforced where the credential is used, only where the +/// reference is introduced. A read check alone would not survive that gap: the reference +/// names a resource whose own value stays mutable, and repointing it at `$res:`/`$var:` +/// the writer cannot read would be a later write this never sees. +/// +/// Hence workspace admin, who can already read every resource in the workspace: the +/// escalation a mutable reference would otherwise buy is one the configurer already has. +/// The read check stays as a typo guard, so a reference to a nonexistent resource fails +/// at configuration time rather than as a puzzling sync error later. +/// +/// Only the `url` field is inspected, and with the same parser the probes use, so the +/// path checked here is exactly the path they will resolve. +pub async fn authorize_azure_devops_reference( + authed: &ApiAuthed, + db: &DB, + user_db: &UserDB, + w_id: &str, + value: Option<&serde_json::Value>, +) -> Result<()> { + let Some(url) = value.and_then(|v| v.get("url")).and_then(|u| u.as_str()) else { + return Ok(()); + }; + let Some((_, resource_path)) = parse_azure_devops_placeholder(url.trim())? else { + return Ok(()); + }; + + if !authed.is_admin { + return Err(Error::PermissionDenied(format!( + "Only a workspace admin can point a git repository URL at AZURE_DEVOPS_TOKEN({resource_path}): background sync mints that credential under an identity that bypasses resource permissions" + ))); + } + + let dba = DbWithOptAuthed::from_authed(authed, db.clone(), Some(user_db.clone())); + let readable = + get_resource_value_interpolated_internal(&dba, w_id, resource_path, None, None, false) + .await + .unwrap_or(None); + if readable.is_none() { + return Err(Error::PermissionDenied(format!( + "Cannot reference AZURE_DEVOPS_TOKEN({resource_path}) in a git repository URL: no such resource" + ))); + } + Ok(()) +} + +/// `allow_cache` carries the caller's freshness requirement through to the token, not +/// just to the resource read: an on-demand check must not succeed on a token minted +/// before the Azure app's permissions were last changed. +async fn mint_azure_devops_token( + tenant_id: &str, + client_id: &str, + client_secret: &str, + allow_cache: bool, +) -> Result { + use sha2::{Digest, Sha256}; + + let mut hasher = Sha256::new(); + for part in [tenant_id, client_id, client_secret] { + hasher.update(part.as_bytes()); + hasher.update([0u8]); + } + let cache_key = hex::encode(hasher.finalize()); + let now = chrono::Utc::now().timestamp(); + // Entries are only ever replaced by a later mint for the same credentials, so a + // rotated secret's entry would otherwise sit here for the process's lifetime. + AZURE_DEVOPS_TOKEN_CACHE.retain(|_, (_, expires_at)| *expires_at > now); + if allow_cache { + let cached = AZURE_DEVOPS_TOKEN_CACHE + .get(&cache_key) + .map(|e| e.value().0.clone()); + if let Some(token) = cached { + return Ok(token); + } + } + + let response = windmill_common::utils::HTTP_CLIENT + .post(format!("{AZURE_LOGIN_HOST}/{tenant_id}/oauth2/token")) + .form(&[ + ("client_id", client_id), + ("client_secret", client_secret), + ("grant_type", "client_credentials"), + ("resource", AZURE_DEVOPS_RESOURCE_ID), + ]) + .send() + .await + .map_err(|e| { + Error::BadRequest(format!("Failed to request an Azure DevOps token: {e:#}")) + })?; + + let status = response.status(); + let body = response.text().await.unwrap_or_default(); + if !status.is_success() { + return Err(Error::BadRequest(format!( + "Azure DevOps token request failed ({status}): {}", + windmill_common::utils::truncate_with_ellipsis(&body, 500) + ))); + } + + #[derive(Deserialize)] + struct AzureTokenResponse { + access_token: String, + expires_in: Option, + } + let parsed: AzureTokenResponse = serde_json::from_str(&body) + .map_err(|e| Error::BadRequest(format!("Unexpected Azure DevOps token response: {e}")))?; + + // The v1 token endpoint returns `expires_in` as a string, the v2 one as a number. + let lifetime = parsed + .expires_in + .as_ref() + .and_then(|v| { + v.as_i64() + .or_else(|| v.as_str().and_then(|s| s.parse::().ok())) + }) + .unwrap_or(AZURE_TOKEN_FALLBACK_LIFETIME_S); + AZURE_DEVOPS_TOKEN_CACHE.insert( + cache_key, + ( + parsed.access_token.clone(), + now + (lifetime - AZURE_TOKEN_EXPIRY_MARGIN_S).max(0), + ), + ); + + Ok(parsed.access_token) +} + +/// System identity used by background git-sync polling. SECURITY: bypasses resource +/// RLS — see [`resolve_git_repository_resource`] for the caller obligations. +fn git_sync_system_dba(db: &DB) -> DbWithOptAuthed<'static, ApiAuthed> { + DbWithOptAuthed::DB { + db: db.clone(), + audit_author: windmill_common::audit::AuditAuthor { + username: "git_sync_auto_pull".to_string(), + email: windmill_common::users::SUPERADMIN_SYNC_EMAIL.to_string(), + username_override: None, + token_prefix: None, + }, + } +} + async fn get_repo_latest_commit_hash( git_resource: &GitRepositoryResource, git_ssh_command: Option, @@ -3363,7 +3767,7 @@ async fn get_repo_latest_commit_hash( .await?; if !output.status.success() { - let stderr = git_probe_stderr(output.stderr); + let stderr = git_probe_stderr(output.stderr, &git_resource.url); return Err(Error::BadRequest(format!( "Error getting git repo commit hash: {}", stderr @@ -3378,7 +3782,8 @@ async fn get_repo_latest_commit_hash( if lines.is_empty() { return Err(Error::BadRequest(format!( "No commits found for reference '{}' in repository '{}'", - ref_spec, git_resource.url + ref_spec, + redact_git_url_credentials(&git_resource.url) ))); } @@ -3399,7 +3804,9 @@ async fn get_repo_latest_commit_hash( /// /// SECURITY: reads under the system identity (`SUPERADMIN_SYNC_EMAIL`), so it /// **bypasses resource RLS** and returns fully-interpolated JSON that **may -/// contain credentials** (an embedded `$var:` token in the URL). Callers must +/// contain credentials** — an embedded `$var:` token in the URL, or the `azure` +/// resource named by an `AZURE_DEVOPS_TOKEN(...)` placeholder. Both name a path +/// chosen by whoever can write the repository URL, not by the reader. Callers must /// have already authorized access to `w_id`, must use it only for git-sync /// `git_repository` resources, and must **not** return the resolved value to a /// client — derive and return only non-sensitive facts. Pass `allow_cache=true` @@ -3411,24 +3818,19 @@ pub async fn resolve_git_repository_resource( git_repo_resource_path: &str, allow_cache: bool, ) -> Result> { - use windmill_common::db::DbWithOptAuthed; - let resource_path = git_repo_resource_path .strip_prefix("$res:") .unwrap_or(git_repo_resource_path); - let dba: DbWithOptAuthed<'_, ApiAuthed> = DbWithOptAuthed::DB { - db: db.clone(), - audit_author: windmill_common::audit::AuditAuthor { - username: "git_sync_auto_pull".to_string(), - email: windmill_common::users::SUPERADMIN_SYNC_EMAIL.to_string(), - username_override: None, - token_prefix: None, - }, - }; - - get_resource_value_interpolated_internal(&dba, w_id, resource_path, None, None, allow_cache) - .await + get_resource_value_interpolated_internal( + &git_sync_system_dba(db), + w_id, + resource_path, + None, + None, + allow_cache, + ) + .await } /// Resolve a workspace git-sync repository and return its current head commit @@ -3459,19 +3861,22 @@ pub async fn get_git_repo_head_for_autopull( return Ok(None); } - let git_resource: GitRepositoryResource = serde_json::from_value(value) + let mut git_resource: GitRepositoryResource = serde_json::from_value(value) .map_err(|e| Error::BadRequest(format!("Invalid git repository resource: {}", e)))?; // The SSH identity is supplied per-call in the authed commit-hash path; the // background poller has none, so an SSH remote can't authenticate here. Fail // with an actionable message instead of a confusing ls-remote auth error — // these repos should use an HTTPS token URL or the GitHub App for auto-pull. - let url = git_resource.url.trim_start(); - if !url.starts_with("http://") && !url.starts_with("https://") { + if !git_resource.url.trim_start().starts_with("http://") + && !git_resource.url.trim_start().starts_with("https://") + { return Err(Error::BadRequest( "Automatic pull can't authenticate an SSH git remote in the background. Use an HTTPS URL with an embedded token, or connect the repository through the GitHub App.".to_string(), )); } + git_resource.url = + resolve_azure_devops_url(&git_sync_system_dba(db), w_id, &git_resource.url, true).await?; if let Some(branch) = git_resource.branch.as_deref().filter(|s| !s.is_empty()) { let branch = branch.to_string(); @@ -3491,7 +3896,7 @@ pub async fn get_git_repo_head_for_autopull( }) .await?; if !output.status.success() { - let stderr = git_probe_stderr(output.stderr); + let stderr = git_probe_stderr(output.stderr, &git_resource.url); return Err(Error::BadRequest(format!( "Error resolving git repo HEAD: {}", stderr @@ -3503,7 +3908,7 @@ pub async fn get_git_repo_head_for_autopull( let sha = sha.ok_or_else(|| { Error::BadRequest(format!( "No HEAD found in repository '{}'", - git_resource.url + redact_git_url_credentials(&git_resource.url) )) })?; Ok(Some((branch.unwrap_or_else(|| "HEAD".to_string()), sha))) @@ -3545,21 +3950,11 @@ pub async fn get_git_repo_fork_heads_for_autopull( base_branch: &str, extra_refs: &[String], ) -> Result>> { - use windmill_common::db::DbWithOptAuthed; - let resource_path = git_repo_resource_path .strip_prefix("$res:") .unwrap_or(git_repo_resource_path); - let dba: DbWithOptAuthed<'_, ApiAuthed> = DbWithOptAuthed::DB { - db: db.clone(), - audit_author: windmill_common::audit::AuditAuthor { - username: "git_sync_auto_pull".to_string(), - email: windmill_common::users::SUPERADMIN_SYNC_EMAIL.to_string(), - username_override: None, - token_prefix: None, - }, - }; + let dba = git_sync_system_dba(db); let value = get_resource_value_interpolated_internal(&dba, w_id, resource_path, None, None, true) .await? @@ -3578,14 +3973,16 @@ pub async fn get_git_repo_fork_heads_for_autopull( return Ok(None); } - let git_resource: GitRepositoryResource = serde_json::from_value(value) + let mut git_resource: GitRepositoryResource = serde_json::from_value(value) .map_err(|e| Error::BadRequest(format!("Invalid git repository resource: {}", e)))?; - let url = git_resource.url.trim_start(); - if !url.starts_with("http://") && !url.starts_with("https://") { + if !git_resource.url.trim_start().starts_with("http://") + && !git_resource.url.trim_start().starts_with("https://") + { return Err(Error::BadRequest( "Automatic pull can't authenticate an SSH git remote in the background. Use an HTTPS URL with an embedded token, or connect the repository through the GitHub App.".to_string(), )); } + git_resource.url = resolve_azure_devops_url(&dba, w_id, &git_resource.url, true).await?; validate_git_url(&git_resource.url).await?; validate_git_ref(base_branch)?; @@ -3608,7 +4005,7 @@ pub async fn get_git_repo_fork_heads_for_autopull( }) .await?; if !output.status.success() { - let stderr = git_probe_stderr(output.stderr); + let stderr = git_probe_stderr(output.stderr, &git_resource.url); return Err(Error::BadRequest(format!( "Error listing fork branches: {}", stderr @@ -4081,7 +4478,7 @@ mod tests { target_requests.lock().unwrap().is_empty(), "git followed the redirect to the unvalidated target" ); - let stderr = git_probe_stderr(output.stderr); + let stderr = git_probe_stderr(output.stderr, ""); assert!( stderr.contains("301") && stderr.contains("redirects are not followed"), "the failure should name the refused redirect and its remedy, got: {stderr}" @@ -4169,6 +4566,94 @@ mod tests { assert!(validate_git_url("--upload-pack=evil").await.is_err()); } + #[test] + fn test_parse_azure_devops_placeholder() { + // The resource path holds '/', so the placeholder must be cut at its own + // closing ')' rather than at the first path separator. + let url = "https://AZURE_DEVOPS_TOKEN(f/azure/devops)@dev.azure.com/org/proj/_git/repo"; + assert_eq!( + parse_azure_devops_placeholder(url).unwrap(), + Some(("AZURE_DEVOPS_TOKEN(f/azure/devops)", "f/azure/devops")) + ); + assert_eq!( + parse_azure_devops_placeholder("https://token@github.com/user/repo.git").unwrap(), + None + ); + assert!(parse_azure_devops_placeholder( + "https://AZURE_DEVOPS_TOKEN(f/azure@dev.azure.com/o" + ) + .is_err()); + // Outside the userinfo the minted token would land in a URL component that git + // echoes back in its errors and redaction does not cover. + assert!(parse_azure_devops_placeholder( + "https://dev.azure.com/org/AZURE_DEVOPS_TOKEN(f/azure)/repo" + ) + .is_err()); + assert!(parse_azure_devops_placeholder( + "ssh://AZURE_DEVOPS_TOKEN(f/azure)@dev.azure.com/o" + ) + .is_err()); + // Plaintext would let an on-path Basic challenge harvest the minted token. + assert!(parse_azure_devops_placeholder( + "http://AZURE_DEVOPS_TOKEN(f/azure)@dev.azure.com/o" + ) + .is_err()); + // A `user:token` userinfo is still the credentials position. + assert_eq!( + parse_azure_devops_placeholder("https://u:AZURE_DEVOPS_TOKEN(f/azure)@dev.azure.com/o") + .unwrap(), + Some(("AZURE_DEVOPS_TOKEN(f/azure)", "f/azure")) + ); + } + + #[test] + fn test_is_azure_devops_host() { + assert!(is_azure_devops_host("dev.azure.com")); + assert!(is_azure_devops_host("vssps.dev.azure.com")); + assert!(is_azure_devops_host("myorg.visualstudio.com")); + // The whole point: a token must never be splice-able onto a chosen host. + assert!(!is_azure_devops_host("attacker.example")); + assert!(!is_azure_devops_host("dev.azure.com.attacker.example")); + assert!(!is_azure_devops_host("notvisualstudio.com")); + assert!(!is_azure_devops_host("github.com")); + } + + #[test] + fn test_redact_git_url_credentials() { + assert_eq!( + redact_git_url_credentials("https://tok@dev.azure.com/o/p"), + "https://***@dev.azure.com/o/p" + ); + assert_eq!( + redact_git_url_credentials("https://user:tok@github.com/u/r.git"), + "https://***@github.com/u/r.git" + ); + // SCP-style `[user@]host:path` carries its credential in the user position. + assert_eq!( + redact_git_url_credentials("tok@github.com:u/r.git"), + "***@github.com:u/r.git" + ); + // A '@' in the path must not be mistaken for the credentials separator. + assert_eq!( + redact_git_url_credentials("https://github.com/u/r@v1.git"), + "https://github.com/u/r@v1.git" + ); + // A userinfo that also occurs in the scheme must not be redacted there. + assert_eq!( + redact_git_url_credentials("https://s@https.com/r"), + "https://***@https.com/r" + ); + } + + #[test] + fn test_git_probe_stderr_scrubs_the_probe_url_credentials() { + // git echoes a username-position token verbatim in this message, and the result + // is persisted as the repository's sync status. + let stderr = b"fatal: could not read Password for 'https://SECRET@dev.azure.com'".to_vec(); + let out = git_probe_stderr(stderr, "https://SECRET@dev.azure.com/o/p"); + assert!(!out.contains("SECRET"), "token survived redaction: {out}"); + } + #[tokio::test] async fn test_validate_git_url_blocks_fragment_query_ssrf() { // GHSA-p5cj-8cfh-mjv6: a loopback authority must stay blocked, and the From c3b22758645b05860f82f33da2c51d7c1d4afb1d Mon Sep 17 00:00:00 2001 From: hugocasa Date: Thu, 13 Aug 2026 13:12:20 +0200 Subject: [PATCH 06/10] docs(agents): rework agent context, fix dev-env docs, vendor skills (#10667) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * docs(agents): scope agent guidance to where it loads AGENTS.md loads in every session. Three of its sections only ever applied to one directory, and docs/autonomous-mode.md was unreferenced by anything in the repo, so none of its content was in effect. - Move "Verifying Backend Changes" to backend/CLAUDE.md, "Verifying Frontend Changes" and "Banned Patterns" to frontend/CLAUDE.md. They now load when working under those directories, which is when they apply. - Update the two cross-references that pointed at the moved sections (pr and svelte-frontend skills). - Delete docs/autonomous-mode.md. Its "don't stop early" half is already in .webmux.yaml's oneshot system prompt, which actually loads; its trigger was bypassPermissions, which does not imply an absent user; and it restated AGENTS.md and the pr skill with copies that had drifted (hardcoded ports, relative screenshot paths). Salvaged the UI traps it uniquely documented into frontend/CLAUDE.md and dropped the three stale profile references. AGENTS.md drops ~3.6k characters with no guidance lost. Co-Authored-By: Claude Opus 5 (1M context) * docs(agents): guidance for building a feature — reuse, telemetry, live verification Three recurring gaps, all cases where a pointer existed but nothing triggered on it. Component reuse. The svelte-frontend skill documented three components with props, which reads as the whole catalog; the barrel exports 23 and common/ has 34 subdirectories against those 23. So "never use raw HTML elements" was an instruction agents could not follow. Added a mandatory discovery step: read the barrel, grep the tree, and treat the documented three as examples. Brand guidelines. frontend/brand-guidelines.md is 34k characters referenced by bare path, which nothing opens speculatively. Added a table mapping what you are building to the section that governs it, entered with grep rather than a full read. Product telemetry. feature_usage has 14 registered actions across three features, and an unregistered (feature, kind) pair is dropped by valid_feature_usage_event with a bare continue — no error, still a 204 — so frontend-only instrumentation silently records nothing. New docs/feature-telemetry.md carries the criteria for when to instrument, the four-step recipe including the allowlist and the InstanceSettings disclosure, and the privacy rules. Raised in the plan for user-facing work, not as a separate question, and not at all for bugfixes or refactors. Also: validation now ends at exercising the change on the running instance, with standing permission to spin up whatever that takes. Co-Authored-By: Claude Opus 5 (1M context) * fix(dev): correct the worktree dev-environment guidance Several things agents were told to do did not match what the machine does. - Env discovery pointed at .env / .env.local / backend/.env. In a webmux worktree the real values are in $(git rev-parse --git-dir)/webmux/runtime.env (BACKEND_PORT, FRONTEND_PORT, DATABASE_URL, CARGO_FEATURES, WM_DB_NAME), sourced by every pane and undocumented. Reading it is also not blocked by the Read(**/.env) deny rules, which the old instruction walked straight into. - The database name rule said branch-with-underscores. worktree-common.sh uses the worktree directory basename, and Postgres truncates at 63 characters, so branch hugo/win-2340-… resolves to windmill_win_2340_…_and_eval with no hugo_ prefix and the tail chopped. A wrong DATABASE_URL guts the sqlx cache. - The restart procedure said "tmux pane 1" and sent keys to an undefined . Pane 1 is the backend under the full profile and the frontend under frontendOnly. Replaced with finding the pane by pane_current_command, recovering the live feature set from the running process (CARGO_FEATURES in runtime.env only records what the pane started with), and restarting in place. - Added recovery for an orphaned backend holding the port: it reparents to systemd when its shell dies, so it survives anything that looks like cleanup. Three checks before killing a single pid, because pkill -f windmill takes out every sibling worktree. - Agents spawned their own servers because AGENTS.md opened by telling them to. Now it checks for the existing panes first; the spawn commands are scoped to a plain checkout. - New EE worktrees branched from the EE repo's local main, which nothing fast-forwards, so they started behind the commit pinned in backend/ee-repo-ref.txt — the one CI builds against. They now base on the pin, falling back to main only when it is unreadable. - Enabled webmux autoPull so local main stays current; new worktrees are branched from it. Documented what WM_CLONE_DB does, including that it terminates every connection to the base windmill database. Co-Authored-By: Claude Opus 5 (1M context) * feat(skills): vendor grilling/architecture skills; tighten PR ready and review rounds Vendors five skills from https://github.com/mattpocock/skills (MIT, pinned at 84fdeffd12f2ee307994d1eb6feb48173b6e0502). They are one dependency closure: grill-me is a stub that runs grilling, and improve-codebase-architecture draws its vocabulary from codebase-design and its CONTEXT.md upkeep from domain-modeling. .agents/skills/UPSTREAM.md records the license, the pin, and the four local deltas so a refresh stays a diff: - flattened the upstream engineering/ and productivity/ split - rewrote bundled-file links to repo-root paths, since relative links break when read through the .claude/skills symlink - dropped the upstream agents/openai.yaml packaging metadata - removed every ADR path. This repo has not adopted ADRs, and a skill that offers to create them is how the practice arrives by side effect rather than by decision. PR workflow changes, all in the pr skill: - A round that never starts is usually a conflict with main, not a CI outage. Resolve by merging, not rebasing — a rebase rewrites the head SHA that round verdicts and the clean-round marker are keyed to. If the merge advances backend/ee-repo-ref.txt, the EE worktree has to follow or cargo check --features private compiles a tree neither the author nor CI intends. - A clean round no longer means an automatic flip to ready. Wide blast radius (*_ee.rs, migrations, OpenAPI or the generated client, auth paths, shared worker infrastructure, a new public surface) asks first; self-contained changes flip. Unattended, the judgement holds and the action degrades: flip the small ones, leave the rest at a clean draft with the reason in the PR body. - Rounds that never converge are usually structural. After three without convergence, stop, name the module the findings cluster around, and suggest improve-codebase-architecture rather than burning more CI. AGENTS.local.md (gitignored, with CLAUDE.local.md importing it) holds the ready/ask calibration, recorded as dated observations rather than a rule. Co-Authored-By: Claude Opus 5 (1M context) * docs(dev): state that each worktree gets its own fresh database The per-worktree section warned which DATABASE_URL to use but never said where the database comes from: the post-create hook creates and migrates a new one per worktree, so it starts with none of the main instance's workspaces, scripts or flows. WM_CLONE_DB was documented only as a comment in .webmux.yaml, which reads as how things work rather than as a per-project opt-in. Co-Authored-By: Claude Opus 5 (1M context) * chore(sqlx): script the cache backup/restore instead of documenting it The update-sqlx skill spelled out a cp/comm/rm dance around `cargo sqlx prepare`, which empties backend/.sqlx before regenerating — a failed run leaves the cache gutted (observed: 2350 -> 142 entries), and a --all-targets run in a CE checkout fails that way every time. Three problems with documenting it: - The backup path was the literal /tmp/sqlx_backup, shared by every worktree. Two concurrent runs overwrite each other's backup, which is the only thing standing between a failed prepare and a gutted cache. - The restore was a copy-pasted `rm -rf .sqlx && cp -r ... && cp ...` chain. - Skipping the backup is what turns a routine failure into a lost cache, and a convention is easier to skip than a command. sqlx-cache.sh has backup / newq / restore, keeps state in a per-worktree directory, and leaves the judgement call where it belongs: `newq` prints each added entry's query field for review, and only `restore` writes them in. Also adds the general rule that scratch files belong outside the checkout — anything written into the tree has to be deleted again, and rm prompts each time. Co-Authored-By: Claude Opus 5 (1M context) * docs(agents): state why a routine cleanup prompts, and where scratch goes The guard hook already auto-allows a plain rm whose operands are under /tmp or inside a git checkout in $HOME, so deleting a temp dir or a stale .sqlx entry costs nothing. What prompts is the command shape: the hook's tokenizer defers on &&, ;, redirects, quotes and $VAR, so a chained cleanup falls through to the Bash(rm:*) ask rule. That was recorded only inside a paragraph about screenshot file paths in frontend/CLAUDE.md, where nobody looking for it would find it. Stated in Core Principles instead, alongside the rule that scratch belongs outside the tree — for the reason that actually applies, which is not committing junk rather than avoiding prompts. Co-Authored-By: Claude Opus 5 (1M context) * chore(security): deny agent edits to the permission hooks and project settings .claude/hooks/guard-rm-outside-tmp.sh and guard-main-branch.sh are the enforcement points for everything the permission rules are meant to catch, and nothing stopped an agent editing them. One sed -i disables the guard for every later command, silently, and the deny list in .claude/settings.json has the same exposure. Defence in depth rather than a boundary: an agent with arbitrary bash can still delete, and this may only close the Edit-tool path if Bash writes are not covered by Edit deny rules. It costs nothing and removes the cheapest way to turn the guards off. Changing them now means editing the files by hand, which is the intent. Co-Authored-By: Claude Opus 5 (1M context) * fix: address review round findings on head 3f47dc1 - backend/ and frontend/ guidance was Claude-only. Codex and Pi read AGENTS.md, not CLAUDE.md, so moving "Verifying Backend/Frontend Changes" and the $bindable ban out of the root AGENTS.md made them invisible to two of the three CLIs this repo supports. Renamed both to AGENTS.md with a one-line @AGENTS.md CLAUDE.md beside them, matching what the repo already does at the root and in ai_evals/, and retargeted the four references. - sqlx-cache.sh aborted with exit 2 and no output when .sqlx was empty: list_entries ran `ls -1 ./*.json`, and an unmatched glob under `set -euo pipefail` killed the script. An empty cache is precisely what a failed prepare leaves behind, so it broke in the one case it exists for. Replaced with a glob loop; reproduced the failure and verified the fix. - The oneshot prompt ("never leave the PR sitting in draft") contradicted the "Flip, or ask first" rule added in the same PR, which tells unattended runs to leave wide-blast-radius changes as clean drafts. The prompt now defers to the skill for the flip decision and keeps only "never stop at an unreviewed draft". - Bundled-resource references in the vendored skills were markdown links to `.agents/skills/...`, which resolve relative to the file, not the repo root. Replaced with inline paths stating they are repo-root relative. - The PR-ready calibration file was write-only: the skill said to record answers there but never to read it. It is now consulted before deciding. Co-Authored-By: Claude Opus 5 (1M context) * Revert "chore(security): deny agent edits to the permission hooks and project settings" This reverts commit 3f47dc169256e46fd64f0c167e38b065b5eba921. * fix: address round 2 nits - backend/AGENTS.md told agents to persist CARGO_FEATURES in runtime.env, but webmux regenerates that file from metadata and .env.local every time the worktree is opened, so the setting is lost on the next reopen. The persistent source is .env.local, which scripts/post-create.sh already writes. - UPSTREAM.md still described the vendoring delta as rewriting bundled-file *links* to repo-root paths. 555f063 replaced them with plain paths in prose, because a markdown target resolves relative to the file — a repo-root link is just as broken as a sibling-relative one through the symlink. Replaying the old wording on a refresh would reintroduce the bug UPSTREAM.md exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) * docs(skills): correct the UPSTREAM.md link-rewrite delta The delta note still described rewriting bundled-file *links* to repo-root paths. 555f063 replaced them with plain paths in prose, because a markdown target resolves relative to the file containing it — a repo-root link is as broken as a sibling-relative one read through the symlink. Replaying the old wording on a refresh would reintroduce exactly the bug UPSTREAM.md exists to prevent. The preceding commit's message claimed this fix; the edit had failed on a stale anchor and only the backend/AGENTS.md half landed. Co-Authored-By: Claude Opus 5 (1M context) * docs(dev): describe what a fresh worktree database actually contains Exercising a real worktree creation showed the previous wording ("none of your workspaces, scripts or flows") reads as an empty database. It is a bootstrap instance: the admins workspace, the admin@windmill.dev superadmin, the license key copied from the base database, and the migration seeds — observed as u/admin/hub_sync and the default app theme resource. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- .agents/skills/UPSTREAM.md | 63 +++++ .agents/skills/codebase-design/DEEPENING.md | 37 +++ .../skills/codebase-design/DESIGN-IT-TWICE.md | 44 ++++ .agents/skills/codebase-design/SKILL.md | 114 +++++++++ .../skills/domain-modeling/CONTEXT-FORMAT.md | 60 +++++ .agents/skills/domain-modeling/SKILL.md | 57 +++++ .agents/skills/grill-me/SKILL.md | 7 + .agents/skills/grilling/SKILL.md | 22 ++ .../HTML-REPORT.md | 122 ++++++++++ .../improve-codebase-architecture/SKILL.md | 68 ++++++ .agents/skills/pr/SKILL.md | 77 +++++- .agents/skills/refine/SKILL.md | 1 - .agents/skills/rust-backend/SKILL.md | 7 + .agents/skills/svelte-frontend/SKILL.md | 50 +++- .agents/skills/update-sqlx/SKILL.md | 23 +- .agents/skills/update-sqlx/sqlx-cache.sh | 88 +++++++ .claude/skills/codebase-design/SKILL.md | 1 + .claude/skills/domain-modeling/SKILL.md | 1 + .claude/skills/grill-me/SKILL.md | 1 + .claude/skills/grilling/SKILL.md | 1 + .../improve-codebase-architecture/SKILL.md | 1 + .gitignore | 4 + .webmux.yaml | 28 ++- AGENTS.md | 142 +++++------ backend/AGENTS.md | 223 ++++++++++++++++++ backend/CLAUDE.md | 145 +----------- docs/autonomous-mode.md | 83 ------- docs/feature-telemetry.md | 92 ++++++++ frontend/AGENTS.md | 85 +++++++ frontend/CLAUDE.md | 30 +-- scripts/worktree-common.sh | 17 +- 31 files changed, 1319 insertions(+), 375 deletions(-) create mode 100644 .agents/skills/UPSTREAM.md create mode 100644 .agents/skills/codebase-design/DEEPENING.md create mode 100644 .agents/skills/codebase-design/DESIGN-IT-TWICE.md create mode 100644 .agents/skills/codebase-design/SKILL.md create mode 100644 .agents/skills/domain-modeling/CONTEXT-FORMAT.md create mode 100644 .agents/skills/domain-modeling/SKILL.md create mode 100644 .agents/skills/grill-me/SKILL.md create mode 100644 .agents/skills/grilling/SKILL.md create mode 100644 .agents/skills/improve-codebase-architecture/HTML-REPORT.md create mode 100644 .agents/skills/improve-codebase-architecture/SKILL.md create mode 100755 .agents/skills/update-sqlx/sqlx-cache.sh create mode 120000 .claude/skills/codebase-design/SKILL.md create mode 120000 .claude/skills/domain-modeling/SKILL.md create mode 120000 .claude/skills/grill-me/SKILL.md create mode 120000 .claude/skills/grilling/SKILL.md create mode 120000 .claude/skills/improve-codebase-architecture/SKILL.md create mode 100644 backend/AGENTS.md delete mode 100644 docs/autonomous-mode.md create mode 100644 docs/feature-telemetry.md create mode 100644 frontend/AGENTS.md diff --git a/.agents/skills/UPSTREAM.md b/.agents/skills/UPSTREAM.md new file mode 100644 index 0000000000..83ba47964e --- /dev/null +++ b/.agents/skills/UPSTREAM.md @@ -0,0 +1,63 @@ +# Vendored skills + +These five skills are copied from an external repository, not written here: + +- `grill-me`, `grilling` +- `improve-codebase-architecture`, `codebase-design`, `domain-modeling` + +Source: https://github.com/mattpocock/skills +Pinned at commit `84fdeffd12f2ee307994d1eb6feb48173b6e0502`. + +They form one dependency closure — `grill-me` is a stub that runs `grilling`, and +`improve-codebase-architecture` draws its vocabulary from `codebase-design` and its +CONTEXT.md upkeep from `domain-modeling`. Removing any one breaks the others. + +Local changes on top of upstream, kept to the minimum so a refresh stays a diff: + +- Flattened the upstream `skills/engineering/` and `skills/productivity/` split, since this + repo's skills are flat. +- Replaced each SKILL.md's markdown links to its own bundled files with plain repo-root paths + in prose (`.agents/skills//FILE.md`). Upstream's sibling-relative links break when the + file is read through the `.claude/skills//SKILL.md` symlink, which mirrors only + SKILL.md — and a repo-root *link* is equally wrong, since a markdown target resolves relative + to the file containing it. Companion files keep their sibling-relative links; they are only + ever read at their real path, never through the symlink. +- Dropped the upstream `agents/openai.yaml` files — Codex packaging metadata for that repo's + own plugin distribution, unused here. +- **Removed every ADR path.** Upstream, `domain-modeling` offers to write Architecture Decision + Records into `docs/adr/` and `improve-codebase-architecture` reads and cites them. This repo has + not adopted ADRs, and a skill that offers to create them is how the practice arrives by side + effect rather than by decision. Deleted `domain-modeling/ADR-FORMAT.md`, its "Offer ADRs + sparingly" section, and the `docs/adr/` entries in its file-structure diagrams; dropped the ADR + clauses from `improve-codebase-architecture` (intro, explore step, "ADR conflicts", the + offer-an-ADR bullet in the grilling loop) and the ADR callout row in `HTML-REPORT.md`. Also cut + "record an architectural decision" from `domain-modeling`'s description, since that phrase is an + invocation trigger. What remains is CONTEXT.md and ubiquitous-language work only. + +To refresh, diff against the same paths at a newer commit and re-apply these four changes. The +ADR removal is the one that needs judgement: if the team later adopts ADRs, take upstream's +version of those sections back rather than rewriting them here. + +## License + +MIT License + +Copyright (c) 2026 Matt Pocock + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. diff --git a/.agents/skills/codebase-design/DEEPENING.md b/.agents/skills/codebase-design/DEEPENING.md new file mode 100644 index 0000000000..3938457b88 --- /dev/null +++ b/.agents/skills/codebase-design/DEEPENING.md @@ -0,0 +1,37 @@ +# Deepening + +How to deepen a cluster of shallow modules safely, given its dependencies. Assumes the vocabulary in [SKILL.md](SKILL.md) — **module**, **interface**, **seam**, **adapter**. + +## Dependency categories + +When assessing a candidate for deepening, classify its dependencies. The category determines how the deepened module is tested across its seam. + +### 1. In-process + +Pure computation, in-memory state, no I/O. Always deepenable — merge the modules and test through the new interface directly. No adapter needed. + +### 2. Local-substitutable + +Dependencies that have local test stand-ins (PGLite for Postgres, in-memory filesystem). Deepenable if the stand-in exists. The deepened module is tested with the stand-in running in the test suite. The seam is internal; no port at the module's external interface. + +### 3. Remote but owned (Ports & Adapters) + +Your own services across a network boundary (microservices, internal APIs). Define a **port** (interface) at the seam. The deep module owns the logic; the transport is injected as an **adapter**. Tests use an in-memory adapter. Production uses an HTTP/gRPC/queue adapter. + +Recommendation shape: *"Define a port at the seam, implement an HTTP adapter for production and an in-memory adapter for testing, so the logic sits in one deep module even though it's deployed across a network."* + +### 4. True external (Mock) + +Third-party services (Stripe, Twilio, etc.) you don't control. The deepened module takes the external dependency as an injected port; tests provide a mock adapter. + +## Seam discipline + +- **One adapter means a hypothetical seam. Two adapters means a real one.** Don't introduce a port unless at least two adapters are justified (typically production + test). A single-adapter seam is just indirection. +- **Internal seams vs external seams.** A deep module can have internal seams (private to its implementation, used by its own tests) as well as the external seam at its interface. Don't expose internal seams through the interface just because tests use them. + +## Testing strategy: replace, don't layer + +- Old unit tests on shallow modules become waste once tests at the deepened module's interface exist — delete them. +- Write new tests at the deepened module's interface. The **interface is the test surface**. +- Tests assert on observable outcomes through the interface, not internal state. +- Tests should survive internal refactors — they describe behaviour, not implementation. If a test has to change when the implementation changes, it's testing past the interface. diff --git a/.agents/skills/codebase-design/DESIGN-IT-TWICE.md b/.agents/skills/codebase-design/DESIGN-IT-TWICE.md new file mode 100644 index 0000000000..8419ad6fa9 --- /dev/null +++ b/.agents/skills/codebase-design/DESIGN-IT-TWICE.md @@ -0,0 +1,44 @@ +# Design It Twice + +When the user wants to explore alternative interfaces for a chosen deepening candidate, use this parallel sub-agent pattern. Based on "Design It Twice" (Ousterhout) — your first idea is unlikely to be the best. + +Uses the vocabulary in [SKILL.md](SKILL.md) — **module**, **interface**, **seam**, **adapter**, **leverage**. + +## Process + +### 1. Frame the problem space + +Before spawning sub-agents, write a user-facing explanation of the problem space for the chosen candidate: + +- The constraints any new interface would need to satisfy +- The dependencies it would rely on, and which category they fall into (see [DEEPENING.md](DEEPENING.md)) +- A rough illustrative code sketch to ground the constraints — not a proposal, just a way to make the constraints concrete + +Show this to the user, then immediately proceed to Step 2. The user reads and thinks while the sub-agents work in parallel. + +### 2. Spawn sub-agents + +Spawn 3+ sub-agents in parallel. Each must produce a **radically different** interface for the deepened module. + +Prompt each sub-agent with a separate technical brief (file paths, coupling details, dependency category from [DEEPENING.md](DEEPENING.md), what sits behind the seam). The brief is independent of the user-facing problem-space explanation in Step 1. Give each agent a different design constraint: + +- Agent 1: "Minimize the interface — aim for 1–3 entry points max. Maximise leverage per entry point." +- Agent 2: "Maximise flexibility — support many use cases and extension." +- Agent 3: "Optimise for the most common caller — make the default case trivial." +- Agent 4 (if applicable): "Design around ports & adapters for cross-seam dependencies." + +Include both [SKILL.md](SKILL.md) vocabulary and CONTEXT.md vocabulary in the brief so each sub-agent names things consistently with the architecture language and the project's domain language. + +Each sub-agent outputs: + +1. Interface (types, methods, params — plus invariants, ordering, error modes) +2. Usage example showing how callers use it +3. What the implementation hides behind the seam +4. Dependency strategy and adapters (see [DEEPENING.md](DEEPENING.md)) +5. Trade-offs — where leverage is high, where it's thin + +### 3. Present and compare + +Present designs sequentially so the user can absorb each one, then compare them in prose. Contrast by **depth** (leverage at the interface), **locality** (where change concentrates), and **seam placement**. + +After comparing, give your own recommendation: which design you think is strongest and why. If elements from different designs would combine well, propose a hybrid. Be opinionated — the user wants a strong read, not a menu. diff --git a/.agents/skills/codebase-design/SKILL.md b/.agents/skills/codebase-design/SKILL.md new file mode 100644 index 0000000000..b7cedf4732 --- /dev/null +++ b/.agents/skills/codebase-design/SKILL.md @@ -0,0 +1,114 @@ +--- +name: codebase-design +description: Shared vocabulary for designing deep modules. Use when the user wants to design or improve a module's interface, find deepening opportunities, decide where a seam goes, make code more testable or AI-navigable, or when another skill needs the deep-module vocabulary. +--- + +# Codebase Design + +Design **deep modules**: a lot of behaviour behind a small interface, placed at a clean seam, testable through that interface. Use this language and these principles wherever code is being designed or restructured. The aim is leverage for callers, locality for maintainers, and testability for everyone. + +## Glossary + +Use these terms exactly — don't substitute "component," "service," "API," or "boundary." Consistent language is the whole point. + +**Module** — anything with an interface and an implementation. Deliberately scale-agnostic: a function, class, package, or tier-spanning slice. _Avoid_: unit, component, service. + +**Interface** — everything a caller must know to use the module correctly: the type signature, but also invariants, ordering constraints, error modes, required configuration, and performance characteristics. _Avoid_: API, signature (too narrow — they refer only to the type-level surface). + +**Implementation** — what's inside a module, its body of code. Distinct from **Adapter**: a thing can be a small adapter with a large implementation (a Postgres repo) or a large adapter with a small implementation (an in-memory fake). Reach for "adapter" when the seam is the topic; "implementation" otherwise. + +**Depth** — leverage at the interface: the amount of behaviour a caller (or test) can exercise per unit of interface they have to learn. A module is **deep** when a large amount of behaviour sits behind a small interface, **shallow** when the interface is nearly as complex as the implementation. + +**Seam** _(Michael Feathers)_ — a place where you can alter behaviour without editing in that place; the *location* at which a module's interface lives. Where to put the seam is its own design decision, distinct from what goes behind it. _Avoid_: boundary (overloaded with DDD's bounded context). + +**Adapter** — a concrete thing that satisfies an interface at a seam. Describes *role* (what slot it fills), not substance (what's inside). + +**Leverage** — what callers get from depth: more capability per unit of interface they learn. One implementation pays back across N call sites and M tests. + +**Locality** — what maintainers get from depth: change, bugs, knowledge, and verification concentrate in one place rather than spreading across callers. Fix once, fixed everywhere. + +## Deep vs shallow + +**Deep module** = small interface + lots of implementation: + +``` +┌─────────────────────┐ +│ Small Interface │ ← Few methods, simple params +├─────────────────────┤ +│ │ +│ Deep Implementation│ ← Complex logic hidden +│ │ +└─────────────────────┘ +``` + +**Shallow module** = large interface + little implementation (avoid): + +``` +┌─────────────────────────────────┐ +│ Large Interface │ ← Many methods, complex params +├─────────────────────────────────┤ +│ Thin Implementation │ ← Just passes through +└─────────────────────────────────┘ +``` + +When designing an interface, ask: + +- Can I reduce the number of methods? +- Can I simplify the parameters? +- Can I hide more complexity inside? + +## Principles + +- **Depth is a property of the interface, not the implementation.** A deep module can be internally composed of small, mockable, swappable parts — they just aren't part of the interface. A module can have **internal seams** (private to its implementation, used by its own tests) as well as the **external seam** at its interface. +- **The deletion test.** Imagine deleting the module. If complexity vanishes, it was a pass-through. If complexity reappears across N callers, it was earning its keep. +- **The interface is the test surface.** Callers and tests cross the same seam. If you want to test *past* the interface, the module is probably the wrong shape. +- **One adapter means a hypothetical seam. Two adapters means a real one.** Don't introduce a seam unless something actually varies across it. + +## Designing for testability + +Good interfaces make testing natural: + +1. **Accept dependencies, don't create them.** + + ```typescript + // Testable + function processOrder(order, paymentGateway) {} + + // Hard to test + function processOrder(order) { + const gateway = new StripeGateway(); + } + ``` + +2. **Return results, don't produce side effects.** + + ```typescript + // Testable + function calculateDiscount(cart): Discount {} + + // Hard to test + function applyDiscount(cart): void { + cart.total -= discount; + } + ``` + +3. **Small surface area.** Fewer methods = fewer tests needed. Fewer params = simpler test setup. + +## Relationships + +- A **Module** has exactly one **Interface** (the surface it presents to callers and tests). +- **Depth** is a property of a **Module**, measured against its **Interface**. +- A **Seam** is where a **Module**'s **Interface** lives. +- An **Adapter** sits at a **Seam** and satisfies the **Interface**. +- **Depth** produces **Leverage** for callers and **Locality** for maintainers. + +## Rejected framings + +- **Depth as ratio of implementation-lines to interface-lines** (Ousterhout): rewards padding the implementation. We use depth-as-leverage instead. +- **"Interface" as the TypeScript `interface` keyword or a class's public methods**: too narrow — interface here includes every fact a caller must know. +- **"Boundary"**: overloaded with DDD's bounded context. Say **seam** or **interface**. + +## Going deeper + +- **Deepening a cluster given its dependencies** — see `.agents/skills/codebase-design/DEEPENING.md` (path from the repo root): dependency categories, seam discipline, and replace-don't-layer testing. +- **Exploring alternative interfaces** — see `.agents/skills/codebase-design/DESIGN-IT-TWICE.md` (path from the repo root): spin up parallel sub-agents to design the interface several radically different ways, then compare on depth, locality, and seam placement. diff --git a/.agents/skills/domain-modeling/CONTEXT-FORMAT.md b/.agents/skills/domain-modeling/CONTEXT-FORMAT.md new file mode 100644 index 0000000000..eaf2a18573 --- /dev/null +++ b/.agents/skills/domain-modeling/CONTEXT-FORMAT.md @@ -0,0 +1,60 @@ +# CONTEXT.md Format + +## Structure + +```md +# {Context Name} + +{One or two sentence description of what this context is and why it exists.} + +## Language + +**Order**: +{A one or two sentence description of the term} +_Avoid_: Purchase, transaction + +**Invoice**: +A request for payment sent to a customer after delivery. +_Avoid_: Bill, payment request + +**Customer**: +A person or organization that places orders. +_Avoid_: Client, buyer, account +``` + +## Rules + +- **Be opinionated.** When multiple words exist for the same concept, pick the best one and list the others under `_Avoid_`. +- **Keep definitions tight.** One or two sentences max. Define what it IS, not what it does. +- **Only include terms specific to this project's context.** General programming concepts (timeouts, error types, utility patterns) don't belong even if the project uses them extensively. Before adding a term, ask: is this a concept unique to this context, or a general programming concept? Only the former belongs. +- **Group terms under subheadings** when natural clusters emerge. If all terms belong to a single cohesive area, a flat list is fine. + +## Single vs multi-context repos + +**Single context (most repos):** One `CONTEXT.md` at the repo root. + +**Multiple contexts:** A `CONTEXT-MAP.md` at the repo root lists the contexts, where they live, and how they relate to each other: + +```md +# Context Map + +## Contexts + +- [Ordering](./src/ordering/CONTEXT.md) — receives and tracks customer orders +- [Billing](./src/billing/CONTEXT.md) — generates invoices and processes payments +- [Fulfillment](./src/fulfillment/CONTEXT.md) — manages warehouse picking and shipping + +## Relationships + +- **Ordering → Fulfillment**: Ordering emits `OrderPlaced` events; Fulfillment consumes them to start picking +- **Fulfillment → Billing**: Fulfillment emits `ShipmentDispatched` events; Billing consumes them to generate invoices +- **Ordering ↔ Billing**: Shared types for `CustomerId` and `Money` +``` + +The skill infers which structure applies: + +- If `CONTEXT-MAP.md` exists, read it to find contexts +- If only a root `CONTEXT.md` exists, single context +- If neither exists, create a root `CONTEXT.md` lazily when the first term is resolved + +When multiple contexts exist, infer which one the current topic relates to. If unclear, ask. diff --git a/.agents/skills/domain-modeling/SKILL.md b/.agents/skills/domain-modeling/SKILL.md new file mode 100644 index 0000000000..b0372a62b8 --- /dev/null +++ b/.agents/skills/domain-modeling/SKILL.md @@ -0,0 +1,57 @@ +--- +name: domain-modeling +description: Build and sharpen a project's domain model. Use when the user wants to pin down domain terminology or a ubiquitous language, or when another skill needs to maintain the domain model. +--- + +# Domain Modeling + +Actively build and sharpen the project's domain model as you design. This is the *active* discipline — challenging terms, inventing edge-case scenarios, and writing the glossary and decisions down the moment they crystallise. (Merely *reading* `CONTEXT.md` for vocabulary is not this skill — that's a one-line habit any skill can do. This skill is for when you're changing the model, not just consuming it.) + +## File structure + +Most repos have a single context: + +``` +/ +├── CONTEXT.md +└── src/ +``` + +If a `CONTEXT-MAP.md` exists at the root, the repo has multiple contexts. The map points to where each one lives: + +``` +/ +├── CONTEXT-MAP.md +└── src/ + ├── ordering/ + │ └── CONTEXT.md + └── billing/ + └── CONTEXT.md +``` + +Create files lazily — only when you have something to write. If no `CONTEXT.md` exists, create one when the first term is resolved. + +## During the session + +### Challenge against the glossary + +When the user uses a term that conflicts with the existing language in `CONTEXT.md`, call it out immediately. "Your glossary defines 'cancellation' as X, but you seem to mean Y — which is it?" + +### Sharpen fuzzy language + +When the user uses vague or overloaded terms, propose a precise canonical term. "You're saying 'account' — do you mean the Customer or the User? Those are different things." + +### Discuss concrete scenarios + +When domain relationships are being discussed, stress-test them with specific scenarios. Invent scenarios that probe edge cases and force the user to be precise about the boundaries between concepts. + +### Cross-reference with code + +When the user states how something works, check whether the code agrees. If you find a contradiction, surface it: "Your code cancels entire Orders, but you just said partial cancellation is possible — which is right?" + +### Update CONTEXT.md inline + +When a term is resolved, update `CONTEXT.md` right there. Don't batch these up — capture them as they happen. Use the format in `.agents/skills/domain-modeling/CONTEXT-FORMAT.md` (path from the repo root). + +`CONTEXT.md` should be totally devoid of implementation details. Do not treat `CONTEXT.md` as a spec, a scratch pad, or a repository for implementation decisions. It is a glossary and nothing else. + diff --git a/.agents/skills/grill-me/SKILL.md b/.agents/skills/grill-me/SKILL.md new file mode 100644 index 0000000000..9470cfcfe2 --- /dev/null +++ b/.agents/skills/grill-me/SKILL.md @@ -0,0 +1,7 @@ +--- +name: grill-me +description: A relentless interview to sharpen a plan or design. +disable-model-invocation: true +--- + +Run a `/grilling` session. diff --git a/.agents/skills/grilling/SKILL.md b/.agents/skills/grilling/SKILL.md new file mode 100644 index 0000000000..95bd01ee90 --- /dev/null +++ b/.agents/skills/grilling/SKILL.md @@ -0,0 +1,22 @@ +--- +name: grilling +description: Grill the user relentlessly about a plan, decision, or idea. Use when the user wants to stress-test their thinking, or uses any 'grill' trigger phrases. +--- + +Interview the user relentlessly until you reach a shared understanding. Map this as a **design tree**: every decision branches into the decisions that hang off it. + +Work the tree in **rounds**. The **frontier** is every decision whose prerequisites are already settled — the questions you can ask _now_ without guessing at answers you haven't heard yet. Ask the whole frontier in one round: number each question and give your recommended answer. Then wait for the user's answers before the next round. + +Each question should be formatted like so: + +``` +❓ **Q1** - ****: + +➡️ +``` + +Each round the user answers reshapes the tree — settled decisions push the frontier outward and unblock questions that depended on them. Recompute the frontier and ask the next round. A question whose answer depends on another question still open in this round belongs to a _later_ round, not this one. + +Finding _facts_ is your job, never the user's. When a frontier question needs a fact from the environment (filesystem, tools, etc.), dispatch a sub-agent to find it — don't ask the user for anything you could look up yourself. Don't block on it: a running exploration is an unsettled prerequisite, so only the questions downstream of it wait for the sub-agent to report — ask the rest of the frontier now. The _decisions_ are the user's — put each to them and wait. + +The session is done when the frontier is empty: every branch of the design tree visited, nothing left silently assumed. Do not act on it until the user confirms you have reached a shared understanding. diff --git a/.agents/skills/improve-codebase-architecture/HTML-REPORT.md b/.agents/skills/improve-codebase-architecture/HTML-REPORT.md new file mode 100644 index 0000000000..ecec59a00f --- /dev/null +++ b/.agents/skills/improve-codebase-architecture/HTML-REPORT.md @@ -0,0 +1,122 @@ +# HTML Report Format + +The architectural review is rendered as a single self-contained HTML file in the OS temp directory. Tailwind and Mermaid both come from CDNs. Mermaid handles graph-shaped diagrams reliably; hand-built divs and inline SVG handle the more editorial visuals (mass diagrams, cross-sections). Mix the two — don't lean on Mermaid for everything, it'll start to look generic. + +## Scaffold + +```html + + + + + Architecture review — {{repo name}} + + + + + +
    +
    ...
    +
    ...
    +
    ...
    +
    + + +``` + +## Header + +Repo name, date, and a compact legend: solid box = module, dashed line = seam, red arrow = leakage, thick dark box = deep module. No introduction paragraph — straight into the candidates. + +## Candidate card + +The diagrams carry the weight. Prose is sparse, plain, and uses the glossary terms (from the `/codebase-design` skill) without ceremony. + +Each candidate is one `
    `: + +- **Title** — short, names the deepening (e.g. "Collapse the Order intake pipeline"). +- **Badge row** — recommendation strength (`Strong` = emerald, `Worth exploring` = amber, `Speculative` = slate), plus a tag for the dependency category (`in-process`, `local-substitutable`, `ports & adapters`, `mock`). +- **Files** — monospaced list, `font-mono text-sm`. +- **Before / After diagram** — the centrepiece. Two columns, side by side. See patterns below. +- **Problem** — one sentence. What hurts. +- **Solution** — one sentence. What changes. +- **Wins** — bullets, ≤6 words each. e.g. "Tests hit one interface", "Pricing logic stops leaking", "Delete 4 shallow wrappers". + +No paragraphs of explanation. If the diagram needs a paragraph to be understood, redraw the diagram. + +## Diagram patterns + +Pick the pattern that fits the candidate. Mix them. Don't make every diagram look the same — variety is part of the point. + +### Mermaid graph (the workhorse for dependencies / call flow) + +Use a Mermaid `flowchart` or `graph` when the point is "X calls Y calls Z, and look at the mess." Wrap it in a Tailwind-styled card so it doesn't feel parachuted in. Style with classDef to colour leakage edges red and the deep module dark. Sequence diagrams work well for "before: 6 round-trips; after: 1." + +```html +
    + 
    +    flowchart LR
+      A[OrderHandler] --> B[OrderValidator]
+      B --> C[OrderRepo]
+      C -.leak.-> D[PricingClient]
+      classDef leak stroke:#dc2626,stroke-width:2px;
+      class C,D leak
+
    +
    +``` + +### Hand-built boxes-and-arrows (when Mermaid's layout fights you) + +Modules as `
    `s with borders and labels. Arrows as inline SVG `` or `` elements positioned absolutely over a relative container. Reach for this when you want the "after" diagram to feel like one thick-bordered deep module with greyed-out internals — Mermaid won't render that with the right weight. + +### Cross-section (good for layered shallowness) + +Stack horizontal bands (`h-12 border-l-4`) to show layers a call passes through. Before: 6 thin layers each doing nothing. After: 1 thick band labelled with the consolidated responsibility. + +### Mass diagram (good for "interface as wide as implementation") + +Two rectangles per module — one for interface surface area, one for implementation. Before: interface rectangle is nearly as tall as the implementation rectangle (shallow). After: interface rectangle is short, implementation rectangle is tall (deep). + +### Call-graph collapse + +Before: a tree of function calls rendered as nested boxes. After: the same tree collapsed into one box, with the now-internal calls shown faded inside it. + +## Style guidance + +- Lean editorial, not corporate-dashboard. Generous whitespace. Serif optional for headings (`font-serif` works well with stone/slate). +- Colour sparingly: one accent (emerald or indigo) plus red for leakage and amber for warnings. +- Keep diagrams ~320px tall so before/after sits comfortably side by side without scrolling. +- Use `text-xs uppercase tracking-wider` for module labels inside diagrams — they should read as schematic, not as UI. +- The only scripts are the Tailwind CDN and the Mermaid ESM import. The report is otherwise static — no app code, no interactivity beyond Mermaid's own rendering. + +## Top recommendation section + +One larger card. Candidate name, one sentence on why, anchor link to its card. That's it. + +## Tone + +Plain English, concise — but the architectural nouns and verbs come straight from the `/codebase-design` skill. Concision is not an excuse to drift. + +**Use exactly:** module, interface, implementation, depth, deep, shallow, seam, adapter, leverage, locality. + +**Never substitute:** component, service, unit (for module) · API, signature (for interface) · boundary (for seam) · layer, wrapper (for module, when you mean module). + +**Phrasings that fit the style:** + +- "Order intake module is shallow — interface nearly matches the implementation." +- "Pricing leaks across the seam." +- "Deepen: one interface, one place to test." +- "Two adapters justify the seam: HTTP in prod, in-memory in tests." + +**Wins bullets** name the gain in glossary terms: *"locality: bugs concentrate in one module"*, *"leverage: one interface, N call sites"*, *"interface shrinks; implementation absorbs the wrappers"*. Don't write *"easier to maintain"* or *"cleaner code"* — those terms aren't in the glossary and don't earn their place. + +No hedging, no throat-clearing, no "it's worth noting that…". If a sentence could be a bullet, make it a bullet. If a bullet could be cut, cut it. If a term isn't in the `/codebase-design` glossary, reach for one that is before inventing a new one. diff --git a/.agents/skills/improve-codebase-architecture/SKILL.md b/.agents/skills/improve-codebase-architecture/SKILL.md new file mode 100644 index 0000000000..488c850990 --- /dev/null +++ b/.agents/skills/improve-codebase-architecture/SKILL.md @@ -0,0 +1,68 @@ +--- +name: improve-codebase-architecture +description: Scan a codebase for deepening opportunities, present them as a visual HTML report, then grill through whichever one you pick. +disable-model-invocation: true +--- + +# Improve Codebase Architecture + +Surface architectural friction and propose **deepening opportunities** — refactors that turn shallow modules into deep ones. The aim is testability and AI-navigability. + +This command is _informed_ by the project's domain model and built on a shared design vocabulary: + +- Run the `/codebase-design` skill for the architecture vocabulary (**module**, **interface**, **depth**, **seam**, **adapter**, **leverage**, **locality**) and its principles (the deletion test, "the interface is the test surface", "one adapter = hypothetical seam, two = real"). Use these terms exactly in every suggestion — don't drift into "component," "service," "API," or "boundary." +- The domain language in `CONTEXT.md` gives names to good seams. + +## Process + +### 1. Explore + +**Scope before you scan — YAGNI.** Deepening a module pays off by making future changes to it easier, so put extra weight on the parts of the codebase that have recently changed. Decide *where* to look before you look: + +- If the user named a direction — a module, a subsystem, a pain point — take it, and skip the inference below. +- Otherwise, walk back a good stretch of the commit history (`git log --oneline`) to find the codebase's hot spots — the files and areas that keep coming up — and let those paths pull your attention first. If the changes are scattered with no clear hot spot, widen the net. + +Read the project's domain glossary (`CONTEXT.md`) first. + +Then spawn a sub-agent to walk the codebase. Don't follow rigid heuristics — explore organically and note where you experience friction: + +- Where does understanding one concept require bouncing between many small modules? +- Where are modules **shallow** — interface nearly as complex as the implementation? +- Where have pure functions been extracted just for testability, but the real bugs hide in how they're called (no **locality**)? +- Where do tightly-coupled modules leak across their seams? +- Which parts of the codebase are untested, or hard to test through their current interface? + +Apply the **deletion test** to anything you suspect is shallow: would deleting it concentrate complexity, or just move it? A "yes, concentrates" is the signal you want. + +### 2. Present candidates as an HTML report + +Write a self-contained HTML file to the OS temp directory so nothing lands in the repo. Resolve the temp dir from `$TMPDIR`, falling back to `/tmp` (or `%TEMP%` on Windows), and write to `/architecture-review-.html` so each run gets a fresh file. Open it for the user — `xdg-open ` on Linux, `open ` on macOS, `start ` on Windows — and tell them the absolute path. + +The report uses **Tailwind via CDN** for layout and styling, and **Mermaid via CDN** for diagrams where a graph/flow/sequence reliably communicates the structure. Mix Mermaid with hand-crafted CSS/SVG visuals — use Mermaid when relationships are graph-shaped (call graphs, dependencies, sequences), and hand-built divs/SVG when you want something more editorial (mass diagrams, cross-sections, collapse animations). Each candidate gets a **before/after visualisation**. Be visual. + +For each candidate, render a card with: + +- **Files** — which files/modules are involved +- **Problem** — why the current architecture is causing friction +- **Solution** — plain English description of what would change +- **Benefits** — explained in terms of locality and leverage, and how tests would improve +- **Before / After diagram** — side-by-side, custom-drawn, illustrating the shallowness and the deepening +- **Recommendation strength** — one of `Strong`, `Worth exploring`, `Speculative`, rendered as a badge + +End the report with a **Top recommendation** section: which candidate you'd tackle first and why. + +**Use CONTEXT.md vocabulary for the domain, and the `/codebase-design` vocabulary for the architecture.** If `CONTEXT.md` defines "Order," talk about "the Order intake module" — not "the FooBarHandler," and not "the Order service." + +See `.agents/skills/improve-codebase-architecture/HTML-REPORT.md` (path from the repo root) for the full HTML scaffold, diagram patterns, and styling guidance. + +Do NOT propose interfaces yet. After the file is written, ask the user: "Which of these would you like to explore?" + +### 3. Grilling loop + +Once the user picks a candidate, run the `/grilling` skill to walk the decision tree with them — constraints, dependencies, the shape of the deepened module, what sits behind the seam, what tests survive. + +Side effects happen inline as decisions crystallize — run the `/domain-modeling` skill to keep the domain model current as you go: + +- **Naming a deepened module after a concept not in `CONTEXT.md`?** Add the term to `CONTEXT.md`. Create the file lazily if it doesn't exist. +- **Sharpening a fuzzy term during the conversation?** Update `CONTEXT.md` right there. +- **Want to explore alternative interfaces for the deepened module?** Run the `/codebase-design` skill and use its design-it-twice parallel sub-agent pattern. diff --git a/.agents/skills/pr/SKILL.md b/.agents/skills/pr/SKILL.md index bd40b6c472..6a72949c96 100644 --- a/.agents/skills/pr/SKILL.md +++ b/.agents/skills/pr/SKILL.md @@ -62,7 +62,7 @@ If `git diff main...HEAD --name-only` matches `^frontend/`, the PR body **must** screenshots of the affected UI. Skip only when there is no visible UI effect (types, tests, build config) — and say so in the body. -1. Verify the change in the browser (AGENTS.md → "Verifying Frontend Changes"). +1. Verify the change in the browser (frontend/AGENTS.md → "Verifying Frontend Changes"). 2. Screenshot each affected page with `mcp__playwright__browser_take_screenshot` (save to a file). 3. Host each image and get its Markdown embed by pushing to the public `windmill-labs/agent-screenshots-internal` repo. **Pipe base64 through stdin** — @@ -130,6 +130,10 @@ and continue once they confirm it's done. ## Review rounds (draft → ready) A PR leaves draft **only after a clean CI review round**. Never run `gh pr ready` before that. +This is the rule in every mode, autonomous included. A clean round is necessary but not always +sufficient — see "Flip, or ask first" below. The one standing exception is an explicit request to +leave that PR in draft (usually so it can be tested first) — honour it for that PR, and don't +carry it over to the next one. 1. **Trigger a round and wait for it**: launch the waiter as a background Bash task (a round takes 10–30 min; you are woken when it exits — do not stop the session or poll in the foreground while it runs): @@ -162,6 +166,77 @@ A PR leaves draft **only after a clean CI review round**. Never run `gh pr ready If any P0/P1 finding is unaddressed or the head moved for reasons other than nit fixes, do **not** post the marker or flip — run another round instead. +### A round that never starts is usually a conflict + +The review workflows don't run on a PR that cannot merge, so a round that produces no verdict is +more often a conflict with `main` than a CI outage. Check before assuming anything is broken: + +```bash +gh pr view --json mergeable,mergeStateStatus +``` + +Resolve by **merging, not rebasing** — a rebase rewrites the head SHA that round verdicts and the +clean-round marker are keyed to, invalidating work you have already paid for: + +```bash +git fetch origin main +git merge origin/main +``` + +**If that merge changed `backend/ee-repo-ref.txt`, move the EE worktree to match.** The file pins +the EE commit CE builds against, so a merge that advances it leaves the EE checkout behind what CE +now expects, and `cargo check --features private` compiles a tree neither you nor CI intends: + +```bash +git -C merge "$(tr -d '[:space:]' < backend/ee-repo-ref.txt)" +``` + +Push both, then start a fresh round — the head moved, so the earlier verdicts no longer apply. + +### Flip, or ask first + +A clean round earns the flip; it does not always earn it *unattended*. Judge the blast radius from +the diff first — `git diff --name-only main...HEAD` answers most of these. + +**Ask before flipping** when the change: + +- touches `*_ee.rs` (it spans the EE repo through symlinks and has a companion PR) +- adds a migration under `backend/migrations/` +- changes `openapi.yaml`, `openflow.openapi.yaml`, or the generated client +- touches auth, permission, or token paths +- changes shared worker infrastructure — the job poller, `handle_child`, an executor +- trips `REVIEW.md`'s "Checklist for new public surfaces" + +**Flip without asking** when it is self-contained: a single-file fix, test-only, docs-only, one +call site, no new public surface. + +Unattended (webmux oneshot) there is nobody to ask, so the judgement holds and the action +degrades: flip the self-contained ones, and leave the rest at a clean draft with a line in the PR +description saying why — `left in draft: adds a migration, wants a human look before ready`. +Don't flip a wide-blast-radius change just because the round came back clean, and don't ask a +question nobody will read. + +`AGENTS.local.md` (gitignored, so it may not exist) carries a "PR ready calibration" section +recording how past ambiguous calls went. Read it before deciding; when a call is still genuinely +ambiguous, ask, then append the answer there so the next one is less ambiguous. + +### When rounds stop converging + +Three or more rounds without a clean verdict usually means the change's shape is wrong, not that +there is an endless supply of independent bugs. The tells: + +- findings keep landing in the same files round after round +- fixing one finding creates the next +- the findings are about coupling, duplication, or state threaded through many places, rather + than logic errors + +When that pattern holds, stop running rounds — each one costs a CI cycle and is not going to +converge. Say plainly that the remaining findings look structural rather than incidental, and +name the module or seam they cluster around. With a user present, suggest they run +`/improve-codebase-architecture` over that area: it is slash-only so you cannot invoke it +yourself, and reshaping the code is a scope change they should choose. Unattended, put the +diagnosis in the PR description and stop there rather than grinding out more rounds. + ## EE Companion PR (when `*_ee.rs` files were modified) The `*_ee.rs` files in the windmill repo are **symlinks** to `windmill-ee-private` — changes won't appear in `git diff` of the windmill repo. Instead, check the EE repo for uncommitted or unpushed changes. diff --git a/.agents/skills/refine/SKILL.md b/.agents/skills/refine/SKILL.md index aaf747cd29..51b29564c6 100644 --- a/.agents/skills/refine/SKILL.md +++ b/.agents/skills/refine/SKILL.md @@ -17,7 +17,6 @@ Reflect on the current session and update documentation with lessons learned. 2. **Read current docs**: Read the docs that were relevant to this session: - `docs/validation.md` - `docs/enterprise.md` - - `docs/autonomous-mode.md` - Any skills that were invoked 3. **Propose updates**: For each piece of friction, decide if it warrants a doc update: diff --git a/.agents/skills/rust-backend/SKILL.md b/.agents/skills/rust-backend/SKILL.md index f0c52002bc..2c6f077f0f 100644 --- a/.agents/skills/rust-backend/SKILL.md +++ b/.agents/skills/rust-backend/SKILL.md @@ -94,6 +94,13 @@ Use `tokio::sync::mpsc` (bounded) for channels. Avoid `std::thread::sleep` in as Always use rust-analyzer LSP for go-to-definition, find-references, and type info. Do not guess at module paths. +## Feature Telemetry + +`FEATURE_USAGE_KINDS` in `windmill-api-workspaces/src/workspaces.rs` is an allowlist: a +`(feature, kind)` pair missing from it is dropped by `valid_feature_usage_event` with a bare +`continue` — no error, and the route still returns 204. Adding a counter on the frontend without +registering it here records nothing. See `docs/feature-telemetry.md`. + ## Axum Handlers Destructure extractors directly in function signatures: diff --git a/.agents/skills/svelte-frontend/SKILL.md b/.agents/skills/svelte-frontend/SKILL.md index b0c4b39939..46bf970091 100644 --- a/.agents/skills/svelte-frontend/SKILL.md +++ b/.agents/skills/svelte-frontend/SKILL.md @@ -7,9 +7,47 @@ description: Svelte coding guidelines for the Windmill frontend. MUST use when w Apply these Windmill-specific patterns when writing Svelte code in `frontend/`. For general Svelte 5 syntax (runes, snippets, event handling), use the Svelte MCP server. +## Before writing any UI (MUST) + +Do both of these before the first line of markup — not after, and not only when something +looks unfamiliar. + +**1. Find the component that already exists.** `frontend/src/lib/components/common/index.ts` +is the design-system barrel — 28 lines, read it in full. It exports far more than the three +documented below: `Alert`, `Badge`, `Breadcrumb`, `Drawer`/`DrawerContent`, `Menu`/`MenuItem`, +`Tabs`/`Tab`/`TabContent`, `Skeleton`, `FileInput`, `RadioCard`, `Section`, `Kbd`, `ActionRow`, +`ClearableInput`, `CopyButton`, `SecondsInput`, `UndoRedo`, `Url`. + +The barrel is not the full picture either: `common/` has 34 subdirectories and only 23 exports, +so `modal/`, `popup/`, `stepper/`, `tooltip/`, `checkbox/`, `table/`, `contextmenu/`, +`confirmationModal/`, `calendarPicker/`, `fileUpload/`, `toggleButton-v2/` and more exist but +must be imported by path. Selects, text inputs and melt-based primitives sit next to `common/` +in `components/select/`, `components/text_input/`, `components/meltComponents/`. + +The tree holds 1,600+ components — grep `frontend/src/lib/components` for the thing you're about +to build; it almost certainly exists. Building a new one is the last resort, not the first move. + +**2. Read the guideline for what you're building.** `frontend/brand-guidelines.md` is the +authority on how it should look and read. Don't load all 34k chars — jump to the section: + +| Building | Section to read | +|---|---| +| Any new screen or component | `# Components` (Core Rules, Quick Reference) | +| Buttons, CTAs | `## Buttons` — hierarchy matters, only one Accent per view | +| Colors, surfaces, borders | `# Color system` (Quick Reference, Do's and Don'ts) | +| Text, labels, headings | `# Typography` — note `## Text Casing`, sentence case throughout | +| Spacing, grids, page structure | `# Spacing & Layout`; `# Layout` → `## Form` for forms | +| Shadows, overlays, depth | `# Elevation` | +| Icons | `# Iconography` | +| Wording of any UI copy | `# Voice & Communication`, `# Tone of Voice` | + +Get the line range with `grep -n '^#' frontend/brand-guidelines.md`, then read just that span. + ## Windmill UI Components (MUST use) -Always use Windmill's design-system components. Never use raw HTML elements. +Always use Windmill's design-system components. Never use raw HTML elements. The three below +are the ones you'll reach for most often — they are examples, not the catalog. For anything +else, go back to the barrel and grep. ### Buttons — `
    diff --git a/frontend/src/lib/components/SimpleEditor.svelte b/frontend/src/lib/components/SimpleEditor.svelte index 37616aa83a..2764b130e2 100644 --- a/frontend/src/lib/components/SimpleEditor.svelte +++ b/frontend/src/lib/components/SimpleEditor.svelte @@ -11,6 +11,7 @@ - + clearPageDrawerAnchor(VARIABLES_PATH)}> {#snippet actions()} + {#if edit && curWs} {/if} diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts index 1faa9c003b..806b963262 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts @@ -120,7 +120,8 @@ import { type ChatCommandItem, type SessionPromptContext, getSessionContextPromptSection, - type GlobalToolHelpers + type GlobalToolHelpers, + type GlobalActivePreviewContext } from './global/core' import { formatChatJobCompletion } from './datatableTools' import { isGlobalAiEnabled } from './global/gate' @@ -584,6 +585,10 @@ export class AIChatManager { // sessions modules — and re-read on every system-message rebuild; the send // path rebuilds after beforeSend, so a fork committed there is picked up. sessionContextResolver: (() => SessionPromptContext | undefined) | undefined = undefined + // The page the side panel shows, stamped on each user message. Same seam as above: + // a page tab is an iframe in its own realm, so the tab model is the only place the + // chat can learn it. Undefined for a live editor — ACTIVE EDITOR covers those. + activePreviewResolver: (() => GlobalActivePreviewContext | undefined) | undefined = undefined // Resolves the workspace this chat operates on. Session chats set it to their // own (possibly forked) workspace so the chat targets it WITHOUT switching the // global workspaceStore. Undefined for the global side-panel chat, which @@ -2329,7 +2334,10 @@ export class AIChatManager { return prepareGlobalUserMessage( pendingPrompt, this.contextManager.getSelectedContext(), - { workspace: this.operatingWorkspace } + { + workspace: this.operatingWorkspace, + activePreview: this.activePreviewResolver?.() + } ) } return undefined @@ -2936,6 +2944,7 @@ export class AIChatManager { case AIMode.GLOBAL: userMessage = prepareGlobalUserMessage(modelInstructions, oldSelectedContext, { workspace: this.operatingWorkspace, + activePreview: this.activePreviewResolver?.(), images: sentImages, files: files }) diff --git a/frontend/src/lib/components/copilot/chat/global/core.test.ts b/frontend/src/lib/components/copilot/chat/global/core.test.ts index e0619b3832..18851e8766 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.test.ts @@ -5052,6 +5052,18 @@ describe('session-only preview tools gating', () => { } }) + // Only a session chat can ever receive an ACTIVE PREVIEW section, so the rule + // explaining it is dead weight (~100 prompt tokens per request) anywhere else. + it('carries the ACTIVE PREVIEW rule only in a chat that has a side panel', () => { + const off = prepareGlobalSystemMessage(undefined, { previewTools: false }).content as string + const on = prepareGlobalSystemMessage(undefined, { previewTools: true }).content as string + expect(off).not.toContain('ACTIVE PREVIEW') + expect(on).toContain('ACTIVE PREVIEW') + // The ACTIVE EDITOR rule is unconditional — live editors exist in both. + expect(off).toContain('ACTIVE EDITOR') + expect(on).toContain('ACTIVE EDITOR') + }) + it('mentions open_preview / get_app_runtime_logs / list_app_runs in the system prompt only when preview tools are enabled', () => { const off = prepareGlobalSystemMessage(undefined, { previewTools: false }).content as string const on = prepareGlobalSystemMessage(undefined, { previewTools: true }).content as string @@ -5256,6 +5268,21 @@ describe('prepareGlobalUserMessage', () => { expect(message.content).not.toContain('content') }) + it('injects the previewed page and the row its drawer has open', () => { + const message = prepareGlobalUserMessage('Disable it', [], { + activePreview: { + label: 'Schedules', + location: '/schedules', + open: 'u/me/daily_report' + } + }) + + expect(message.content).toContain('## ACTIVE PREVIEW') + expect(message.content).toContain('page: Schedules') + expect(message.content).toContain('location: /schedules') + expect(message.content).toContain('open: u/me/daily_report') + }) + it('includes selected workspace item references without contents', () => { const message = prepareGlobalUserMessage('Update these items', [ { diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index e34e08de6e..23ca167395 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -254,9 +254,26 @@ export type GlobalActiveEditorContext = { isLiveDraft: true } +/** The page the session's side panel is showing, when it isn't one of the live + * editors ACTIVE EDITOR already covers. A page tab is an iframe in its own realm, + * so the chat can only learn about it from the tab model the session owns. */ +export type GlobalActivePreviewContext = { + /** Page name as the tab strip shows it, e.g. "Schedules". */ + label: string + /** Base-stripped page path plus the request params the page declares — values kept + * only for the ones addressing a workspace object, and percent-encoded. Never a raw + * location: a tab can host a legacy app whose hash is app state, and a filter value + * can be free text the user typed. Build it with `previewLocationContext`. */ + location: string + /** The row whose drawer is open on that page. The list pages drop the anchor when + * their drawer closes, so its absence means no row is open. */ + open?: string +} + export type GlobalUserMessageOptions = { workspace?: string activeEditor?: GlobalActiveEditorContext + activePreview?: GlobalActivePreviewContext /** Images attached to this message; delivered as image_url content parts. */ images?: AttachedImage[] /** Text files attached to this message; listed by reference below — the model @@ -1197,6 +1214,12 @@ const buildGlobalSystemPrompt = ( const pipelineAlphaNote = previewTools ? ' Data pipeline support in this chat is in ALPHA: the first time the user asks for a data pipeline in this session, briefly tell them it is an alpha feature before you start building.' : '' + // Gated on `previewTools` (constant per chat), never on whether a preview is open + // right now: the system prompt is the cached prefix, so a line appearing and + // disappearing between turns costs more cache than the tool call it saves. + const activePreviewRule = previewTools + ? '\n- If the user message includes an ACTIVE PREVIEW section, that is the page the side panel is showing — resolve "this page", "here" and "it" against it, and against `open` (the row the page is anchored at, whose drawer the user opened) when there is one. It already tells you what get_preview_status would, so do not call that tool to learn what is on screen; call it only to check the panel\'s *other* tabs.' + : '' const pipelineBullet = `- A "data pipeline" is NOT a flow: it is a DAG of independent scripts in one folder, wired by storage assets (DuckLake/data tables/S3) and triggers via top-of-file \`pipeline\` / \`on \` annotation comments written in each script's comment syntax (\`--\` for SQL, \`#\` for Python/Bash, \`//\` for TS — a \`//\` line in a SQL node is a syntax error). When the user asks for a data pipeline (or to ingest/transform/materialize data across steps), call get_instructions with subject "pipeline" and build annotated script drafts — do not build a flow.${pipelineAlphaNote}` return `You are Windmill's global workspace assistant. @@ -1214,7 +1237,7 @@ Path conventions: Rules: - Draft tools create or update drafts only; they do not deploy or mutate deployed workspace items. - Use list_workspace_items to find items and read_workspace_item before changing an existing item. For triggers, pass trigger_kind. -- If the user message includes an ACTIVE EDITOR section, treat it as the currently open item and use it for references like "this", "current", or "open editor". +- If the user message includes an ACTIVE EDITOR section, treat it as the currently open item and use it for references like "this", "current", or "open editor".${activePreviewRule} - Use deploy_workspace_item only after the user explicitly asks to deploy. It persists a draft to the workspace. - To undo something you created or changed in this chat, use discard_local_draft: everything you write is a draft until it is explicitly deployed, so "delete it" / "never mind" / "remove that" about your own work means discarding the draft (it also clears the matching open editor draft). Use delete_workspace_item only to remove an item that is already deployed in the workspace; it mutates the workspace and fails if nothing is deployed at that path. - Use diff to review changes — before deploying, or when the user asks what changed. It is read-only: without arguments it lists every draft in the workspace with its change status; with type+path it returns that item's unified diff (for multi-file apps, pass file to read one file's diff). In a fork, pass against="parent_workspace" to compare the deployed fork with its parent workspace instead. Pass search to grep changed lines across all diffs. @@ -7341,6 +7364,16 @@ export function prepareGlobalUserMessage( content += `isLiveDraft: true\n\n` } + if (options.activePreview) { + content += '## ACTIVE PREVIEW\n' + content += `page: ${options.activePreview.label}\n` + content += `location: ${options.activePreview.location}\n` + if (options.activePreview.open) { + content += `open: ${options.activePreview.open}\n` + } + content += '\n' + } + if (selectedWorkspaceItems.length > 0) { content += '## SELECTED CONTEXT\n' for (const context of selectedWorkspaceItems) { diff --git a/frontend/src/lib/components/copilot/chat/global/pageNavigation.ts b/frontend/src/lib/components/copilot/chat/global/pageNavigation.ts index 7219c509b6..6556799833 100644 --- a/frontend/src/lib/components/copilot/chat/global/pageNavigation.ts +++ b/frontend/src/lib/components/copilot/chat/global/pageNavigation.ts @@ -1,9 +1,8 @@ import { buildFilterUrl } from '$lib/navigation' -import { buildRunsFilterSearchbarSchema } from '$lib/components/runs/runsFilter' -import { buildSchedulesFilterSchema } from '$lib/components/schedules/schedulesFilter' import { COMPARE_PAGE, TRIGGER_PAGES, + pageRequestParams, type TriggerKind } from '$lib/components/sessions/previewRouter' import { @@ -11,16 +10,17 @@ import { serializeItemsMaskParam } from '$lib/components/sessions/modifiedItemsMask' -// In-app paths for the deep-linkable preview pages the AI chat can open. -export const RUNS_PATH = '/runs' -export const SCHEDULES_PATH = '/schedules' -export const VARIABLES_PATH = '/variables' -export const RESOURCES_PATH = '/resources' -export const ASSETS_PATH = '/assets' -export const AUDIT_LOGS_PATH = '/audit_logs' -export const WORKSPACE_SETTINGS_PATH = '/workspace_settings' -export const FOLDERS_PATH = '/folders' -export const GROUPS_PATH = '/groups' +import { + RUNS_PATH, + SCHEDULES_PATH, + VARIABLES_PATH, + RESOURCES_PATH, + ASSETS_PATH, + AUDIT_LOGS_PATH, + WORKSPACE_SETTINGS_PATH, + FOLDERS_PATH, + GROUPS_PATH +} from '$lib/components/sessions/previewPaths' // Selectable tabs on the Workspace settings page (the `?tab=` query param). Mirrors the // union in routes/(root)/(logged)/workspace_settings/+page.svelte. @@ -49,27 +49,17 @@ export const WORKSPACE_SETTINGS_TABS = [ 'shared_ui' ] as const -// Valid query-param keys are derived from the real filter schemas (option arrays are -// irrelevant to the key set), so a renamed filter key propagates here for free. The -// permission flags are on so the key set is complete: gating `all_workspaces` is the -// caller's job, and the Runs page ignores it for anyone whose own schema lacks the key. -const RUNS_FILTER_KEYS = Object.keys( - buildRunsFilterSearchbarSchema({ - paths: [], - usernames: [], - folders: [], - jobTriggerKinds: [], - isSuperAdminOrDevops: true, - isAdminsWorkspace: true - }) -) -const SCHEDULES_FILTER_KEYS = Object.keys( - buildSchedulesFilterSchema({ paths: [], scriptPaths: [] }) -) +// Every builder below allows exactly the params `previewRouter` records as +// request-settable for that page, so the URLs this emits and the preview's reading of +// them stay one set. Wherever the page declares a filter schema that set is its full +// key list, so a renamed or added filter propagates here for free — including the keys +// only some viewers see: gating `all_workspaces` is the caller's job, and the Runs page +// ignores it for anyone whose own schema lacks it. What the chat may actually pass is +// narrower and lives in the open_page tool schema, not here. /** Deep-link to the Runs page with the given filters (keys must match `runsFilter`). */ export function buildRunsUrl(filters: Record): string { - return buildFilterUrl(RUNS_PATH, filters, { validKeys: RUNS_FILTER_KEYS }) + return buildFilterUrl(RUNS_PATH, filters, { validKeys: pageRequestParams(RUNS_PATH) }) } /** @@ -84,15 +74,11 @@ export function buildSchedulesUrl({ filters?: Record }): string { return buildFilterUrl(SCHEDULES_PATH, filters ?? {}, { - validKeys: SCHEDULES_FILTER_KEYS, + validKeys: pageRequestParams(SCHEDULES_PATH), hash: open }) } -// The remaining pages expose a curated subset of each page's real query params (not the -// full filter schema), so the allow-list is the exact set of keys the builder emits — -// these names match the query params the pages read (variablesFilter/resourcesFilter/ -// assetsFilter and audit_logs/+page.svelte). /** When `open` is set, the variable at that exact path is opened in the edit * drawer via the `#` hash the page already handles. */ export function buildVariablesUrl({ @@ -103,7 +89,7 @@ export function buildVariablesUrl({ filters?: Record }): string { return buildFilterUrl(VARIABLES_PATH, filters ?? {}, { - validKeys: ['path', 'owner'], + validKeys: pageRequestParams(VARIABLES_PATH), hash: open }) } @@ -118,24 +104,26 @@ export function buildResourcesUrl({ filters?: Record }): string { return buildFilterUrl(RESOURCES_PATH, filters ?? {}, { - validKeys: ['path', 'resource_type', 'owner'], + validKeys: pageRequestParams(RESOURCES_PATH), hash: open ? `/resource/${open}` : undefined }) } export function buildAssetsUrl(filters: Record): string { - return buildFilterUrl(ASSETS_PATH, filters, { validKeys: ['path'] }) + return buildFilterUrl(ASSETS_PATH, filters, { validKeys: pageRequestParams(ASSETS_PATH) }) } export function buildAuditLogsUrl(filters: Record): string { return buildFilterUrl(AUDIT_LOGS_PATH, filters, { - validKeys: ['username', 'operation', 'resource'] + validKeys: pageRequestParams(AUDIT_LOGS_PATH) }) } /** Deep-link to the Workspace settings page, optionally on a specific `?tab=`. */ export function buildWorkspaceSettingsUrl({ tab }: { tab?: string }): string { - return buildFilterUrl(WORKSPACE_SETTINGS_PATH, tab ? { tab } : {}) + return buildFilterUrl(WORKSPACE_SETTINGS_PATH, tab ? { tab } : {}, { + validKeys: pageRequestParams(WORKSPACE_SETTINGS_PATH) + }) } /** Folders and Groups list pages have no query filters — just open them. */ @@ -173,7 +161,7 @@ export function buildCompareUrl({ mode, [COMPARE_ITEMS_PARAM]: items ? serializeItemsMaskParam(items) : undefined }, - { validKeys: ['workspace_id', 'mode', COMPARE_ITEMS_PARAM] } + { validKeys: pageRequestParams(COMPARE_PAGE.path) } ) } diff --git a/frontend/src/lib/components/pendingEditorFlush.test.ts b/frontend/src/lib/components/pendingEditorFlush.test.ts new file mode 100644 index 0000000000..b55732734e --- /dev/null +++ b/frontend/src/lib/components/pendingEditorFlush.test.ts @@ -0,0 +1,27 @@ +import { describe, it, expect } from 'vitest' +import { + anyEditorUnparseable, + setEditorUnparseable, + registerPendingEditor, + flushAllPendingEditorChanges +} from './pendingEditorFlush' + +describe('pendingEditorFlush', () => { + it('reports unparseable text until the editor clears it', () => { + const editor = {} + expect(anyEditorUnparseable()).toBe(false) + setEditorUnparseable(editor, true) + expect(anyEditorUnparseable()).toBe(true) + setEditorUnparseable(editor, false) + expect(anyEditorUnparseable()).toBe(false) + }) + + it('flushes registered editors, and stops once they unmount', () => { + let flushed = 0 + const deregister = registerPendingEditor({ flushPendingChanges: () => flushed++ }) + flushAllPendingEditorChanges() + deregister() + flushAllPendingEditorChanges() + expect(flushed).toBe(1) + }) +}) diff --git a/frontend/src/lib/components/pendingEditorFlush.ts b/frontend/src/lib/components/pendingEditorFlush.ts new file mode 100644 index 0000000000..20ddde67a5 --- /dev/null +++ b/frontend/src/lib/components/pendingEditorFlush.ts @@ -0,0 +1,36 @@ +// Every mounted `SimpleEditor`, so a caller can materialise what the user typed without +// knowing which editors a page contains — a drawer nests them through SchemaForm and +// ArgInput, so enumerating them from the container does not scale. Plain module rather +// than the editor component: importing that pulls Monaco's side-effect imports into every +// graph that reaches this, and the components around it defer Monaco deliberately. +const liveEditors = new Set<{ flushPendingChanges: () => void }>() + +/** Register a mounted editor; the returned function deregisters it. */ +export function registerPendingEditor(editor: { flushPendingChanges: () => void }): () => void { + liveEditors.add(editor) + return () => liveEditors.delete(editor) +} + +/** Drain every mounted editor's debounced buffer. For code that must act on what is on + * screen before leaving it — persisting a draft before routing to a session. */ +export function flushAllPendingEditorChanges(): void { + for (const editor of liveEditors) editor.flushPendingChanges() +} + +// Editors whose current text does not parse. Their value never reaches the bound field, so +// a caller persisting "what is on screen" would save the last value that did parse and +// leave without it. Registered by the editors that parse, not by the ones that only hold text. +const unparseable = new Set() + +/** Mark or clear this editor as holding text that does not parse. */ +export function setEditorUnparseable(key: object, invalid: boolean): void { + if (invalid) unparseable.add(key) + else unparseable.delete(key) +} + +/** Whether any editor on screen holds text that cannot be persisted as written. Registry- + * wide rather than per-item: the editors that parse are nested arbitrarily deep and none + * of them knows which draft it belongs to. */ +export function anyEditorUnparseable(): boolean { + return unparseable.size > 0 +} diff --git a/frontend/src/lib/components/sessions/OpenInSessionButton.svelte b/frontend/src/lib/components/sessions/OpenInSessionButton.svelte index 72d319ec9e..22f1167455 100644 --- a/frontend/src/lib/components/sessions/OpenInSessionButton.svelte +++ b/frontend/src/lib/components/sessions/OpenInSessionButton.svelte @@ -4,14 +4,26 @@ // What an editor hands over for "Open in AI session": the session target it // maps to, the workspace it lives in, and a persist hook run before routing // so the session preview opens the item exactly as currently edited. - export type OpenInSessionSource = { - target: SessionTarget + type OpenInSessionCommon = { workspaceId?: string beforeOpen?: () => void | Promise /** Where inside the item the preview should open (a flow's `selected` * step). Steers the editor only — tab identity is (kind, path). */ previewParams?: Record } + + // A destination is either an editable item or a page, never both and never + // neither — the union is what makes that a compile error rather than a button + // that silently does nothing. + export type OpenInSessionSource = OpenInSessionCommon & + ( + | { target: SessionTarget; page?: never } + /** Base-prefixed href of a workspace page the preview opens as a tab (Runs, + * a trigger list). Resolved on click, not at render: a page whose filters + * live in shallow-routed query params never reflects them in `page.url`, so + * only `window.location` read at that moment matches what the user sees. */ + | { page: () => string | undefined; target?: never } + ) {#if !allowDraft} {@render extra?.()} + {#if edit} + {#if !trigger?.draftConfig}
    import { untrack } from 'svelte' + import { + clearPageDrawerAnchor, + setPageDrawerAnchor + } from '$lib/components/sessions/pageDrawerSession' + import { TRIGGER_PAGES } from '$lib/components/sessions/previewPaths' import { Alert } from '$lib/components/common' import Drawer from '$lib/components/common/drawer/Drawer.svelte' import DrawerContent from '$lib/components/common/drawer/DrawerContent.svelte' @@ -159,6 +164,7 @@ drawerLoading = true try { drawer?.openDrawer() + setPageDrawerAnchor(TRIGGER_PAGES.amqp.path, ePath) initialPath = ePath itemKind = isFlow ? 'flow' : 'script' edit = true @@ -392,7 +398,11 @@ {/if} {#if useDrawer} - + clearPageDrawerAnchor(TRIGGER_PAGES.amqp.path)} + > import { Alert, Button } from '$lib/components/common' + import { + clearPageDrawerAnchor, + setPageDrawerAnchor + } from '$lib/components/sessions/pageDrawerSession' + import { TRIGGER_PAGES } from '$lib/components/sessions/previewPaths' import Drawer from '$lib/components/common/drawer/Drawer.svelte' import DrawerContent from '$lib/components/common/drawer/DrawerContent.svelte' import Path from '$lib/components/Path.svelte' @@ -131,6 +136,7 @@ drawerLoading = true try { drawer?.openDrawer() + setPageDrawerAnchor(TRIGGER_PAGES.azure.path, ePath) initialPath = ePath itemKind = isFlow ? 'flow' : 'script' edit = true @@ -206,11 +212,11 @@ const { draft: draftFromBackend, ...deployedTrigger } = (s ?? {}) as any loadTriggerConfig(deployedTrigger) return { - noDeployed: !!(s as any)?.no_deployed, - overlay: draftFromBackend - ? ({ ...deployedTrigger, ...draftFromBackend } as Record) - : undefined - } + noDeployed: !!(s as any)?.no_deployed, + overlay: draftFromBackend + ? ({ ...deployedTrigger, ...draftFromBackend } as Record) + : undefined + } } catch (error) { sendUserToast(`Could not load Azure trigger: ${error.body}`, true) return { overlay: undefined, noDeployed: false } @@ -348,7 +354,11 @@ {/if} {#if useDrawer} - + clearPageDrawerAnchor(TRIGGER_PAGES.azure.path)} + > import { Button } from '$lib/components/common' + import { + clearPageDrawerAnchor, + setPageDrawerAnchor + } from '$lib/components/sessions/pageDrawerSession' + import { TRIGGER_PAGES } from '$lib/components/sessions/previewPaths' import Drawer from '$lib/components/common/drawer/Drawer.svelte' import DrawerContent from '$lib/components/common/drawer/DrawerContent.svelte' import Path from '$lib/components/Path.svelte' @@ -128,6 +133,7 @@ }, 100) // if loading takes less than 100ms, we don't show the loader try { drawer?.openDrawer() + setPageDrawerAnchor(TRIGGER_PAGES.email.path, ePath) initialPath = ePath path = ePath itemKind = isFlow ? 'flow' : 'script' @@ -461,6 +467,7 @@ {#snippet saveButton()} {#if !drawerLoading} + clearPageDrawerAnchor(TRIGGER_PAGES.email.path)} + > import { Alert, Button } from '$lib/components/common' + import { + clearPageDrawerAnchor, + setPageDrawerAnchor + } from '$lib/components/sessions/pageDrawerSession' + import { TRIGGER_PAGES } from '$lib/components/sessions/previewPaths' import Drawer from '$lib/components/common/drawer/Drawer.svelte' import DrawerContent from '$lib/components/common/drawer/DrawerContent.svelte' import Path from '$lib/components/Path.svelte' @@ -136,6 +141,7 @@ drawerLoading = true try { drawer?.openDrawer() + setPageDrawerAnchor(TRIGGER_PAGES.gcp.path, ePath) initialPath = ePath itemKind = isFlow ? 'flow' : 'script' edit = true @@ -255,13 +261,7 @@ if (!cfg) { return } - const isSaved = await saveGcpTriggerFromCfg( - initialPath, - cfg, - edit, - wsId!, - usedTriggerKinds - ) + const isSaved = await saveGcpTriggerFromCfg(initialPath, cfg, edit, wsId!, usedTriggerKinds) if (isSaved) { draftSync.discard(previousPath, getGcpConfig()) onUpdate?.(cfg.path) @@ -368,7 +368,11 @@ {/if} {#if useDrawer} - + clearPageDrawerAnchor(TRIGGER_PAGES.gcp.path)} + > import { Button } from '$lib/components/common' + import { + clearPageDrawerAnchor, + setPageDrawerAnchor + } from '$lib/components/sessions/pageDrawerSession' + import { TRIGGER_PAGES } from '$lib/components/sessions/previewPaths' import Drawer from '$lib/components/common/drawer/Drawer.svelte' import DrawerContent from '$lib/components/common/drawer/DrawerContent.svelte' import Path from '$lib/components/Path.svelte' @@ -224,6 +229,7 @@ }, 100) // if loading takes less than 100ms, we don't show the loader try { drawer?.openDrawer() + setPageDrawerAnchor(TRIGGER_PAGES.http.path, ePath) initialPath = ePath path = ePath itemKind = isFlow ? 'flow' : 'script' @@ -362,8 +368,8 @@ return { noDeployed: !!(s as any)?.no_deployed, overlay: draftFromBackend - ? ({ ...deployedTrigger, ...draftFromBackend } as Record) - : undefined + ? ({ ...deployedTrigger, ...draftFromBackend } as Record) + : undefined } } @@ -985,6 +991,7 @@ {#snippet saveButton()} {#if !drawerLoading} + clearPageDrawerAnchor(TRIGGER_PAGES.http.path)} + > import { Alert, Button } from '$lib/components/common' + import { + clearPageDrawerAnchor, + setPageDrawerAnchor + } from '$lib/components/sessions/pageDrawerSession' + import { TRIGGER_PAGES } from '$lib/components/sessions/previewPaths' import ConfirmationModal from '$lib/components/common/confirmationModal/ConfirmationModal.svelte' import Drawer from '$lib/components/common/drawer/Drawer.svelte' import DrawerContent from '$lib/components/common/drawer/DrawerContent.svelte' @@ -159,6 +164,7 @@ drawerLoading = true try { drawer?.openDrawer() + setPageDrawerAnchor(TRIGGER_PAGES.kafka.path, ePath) initialPath = ePath itemKind = isFlow ? 'flow' : 'script' edit = true @@ -412,7 +418,11 @@ {/if} {#if useDrawer} - + clearPageDrawerAnchor(TRIGGER_PAGES.kafka.path)} + > import { untrack } from 'svelte' + import { + clearPageDrawerAnchor, + setPageDrawerAnchor + } from '$lib/components/sessions/pageDrawerSession' + import { TRIGGER_PAGES } from '$lib/components/sessions/previewPaths' import { Alert, Button } from '$lib/components/common' import Drawer from '$lib/components/common/drawer/Drawer.svelte' import DrawerContent from '$lib/components/common/drawer/DrawerContent.svelte' @@ -154,6 +159,7 @@ drawerLoading = true try { drawer?.openDrawer() + setPageDrawerAnchor(TRIGGER_PAGES.mqtt.path, ePath) initialPath = ePath itemKind = isFlow ? 'flow' : 'script' edit = true @@ -317,13 +323,7 @@ deploymentLoading = true const previousPath = initialPath const cfg = getSaveCfg() - const isSaved = await saveMqttTriggerFromCfg( - initialPath, - cfg, - edit, - wsId!, - usedTriggerKinds - ) + const isSaved = await saveMqttTriggerFromCfg(initialPath, cfg, edit, wsId!, usedTriggerKinds) if (isSaved) { draftSync.discard(previousPath, getSaveCfg()) onUpdate?.(cfg.path) @@ -392,7 +392,11 @@ {/if} {#if useDrawer} - + clearPageDrawerAnchor(TRIGGER_PAGES.mqtt.path)} + > import { Alert, Button } from '$lib/components/common' + import { + clearPageDrawerAnchor, + setPageDrawerAnchor + } from '$lib/components/sessions/pageDrawerSession' + import { TRIGGER_PAGES } from '$lib/components/sessions/previewPaths' import Drawer from '$lib/components/common/drawer/Drawer.svelte' import DrawerContent from '$lib/components/common/drawer/DrawerContent.svelte' import Path from '$lib/components/Path.svelte' @@ -142,6 +147,7 @@ drawerLoading = true try { drawer?.openDrawer() + setPageDrawerAnchor(TRIGGER_PAGES.nats.path, ePath) initialPath = ePath itemKind = isFlow ? 'flow' : 'script' edit = true @@ -296,13 +302,7 @@ deploymentLoading = true const previousPath = initialPath const cfg = natsConfig - const isSaved = await saveNatsTriggerFromCfg( - initialPath, - cfg, - edit, - wsId!, - usedTriggerKinds - ) + const isSaved = await saveNatsTriggerFromCfg(initialPath, cfg, edit, wsId!, usedTriggerKinds) if (isSaved) { draftSync.discard(previousPath, getSaveCfg()) onUpdate?.(cfg.path) @@ -389,7 +389,11 @@ {/if} {#if useDrawer} - + clearPageDrawerAnchor(TRIGGER_PAGES.nats.path)} + > import { Alert, Button, TabContent } from '$lib/components/common' + import { + clearPageDrawerAnchor, + setPageDrawerAnchor + } from '$lib/components/sessions/pageDrawerSession' + import { TRIGGER_PAGES } from '$lib/components/sessions/previewPaths' import Drawer from '$lib/components/common/drawer/Drawer.svelte' import DrawerContent from '$lib/components/common/drawer/DrawerContent.svelte' import Path from '$lib/components/Path.svelte' @@ -243,6 +248,7 @@ drawerLoading = true try { drawer?.openDrawer() + setPageDrawerAnchor(TRIGGER_PAGES.postgres.path, ePath) initialPath = ePath itemKind = isFlow ? 'flow' : 'script' edit = true @@ -567,7 +573,11 @@ {/if} {#if useDrawer} - + clearPageDrawerAnchor(TRIGGER_PAGES.postgres.path)} + > import { Alert, Badge, Button, ButtonType, Tab, Tabs } from '$lib/components/common' + import { + clearPageDrawerAnchor, + setPageDrawerAnchor + } from '$lib/components/sessions/pageDrawerSession' + import { SCHEDULES_PATH } from '$lib/components/sessions/previewPaths' import TriggerAdvancedBadges from '../TriggerAdvancedBadges.svelte' import Drawer from '$lib/components/common/drawer/Drawer.svelte' import DrawerContent from '$lib/components/common/drawer/DrawerContent.svelte' @@ -165,6 +170,7 @@ drawerLoading = true try { drawer?.openDrawer() + setPageDrawerAnchor(SCHEDULES_PATH, ePath) initialPath = ePath itemKind = isFlow ? 'flow' : 'script' path = defaultCfg?.path ?? ePath @@ -729,6 +735,7 @@ {#snippet saveButton()} {#if !drawerLoading} + clearPageDrawerAnchor(SCHEDULES_PATH)}> import { Alert, Button } from '$lib/components/common' + import { + clearPageDrawerAnchor, + setPageDrawerAnchor + } from '$lib/components/sessions/pageDrawerSession' + import { TRIGGER_PAGES } from '$lib/components/sessions/previewPaths' import Drawer from '$lib/components/common/drawer/Drawer.svelte' import DrawerContent from '$lib/components/common/drawer/DrawerContent.svelte' import Path from '$lib/components/Path.svelte' @@ -137,6 +142,7 @@ drawerLoading = true try { drawer?.openDrawer() + setPageDrawerAnchor(TRIGGER_PAGES.sqs.path, ePath) initialPath = ePath itemKind = isFlow ? 'flow' : 'script' edit = true @@ -306,13 +312,7 @@ deploymentLoading = true const previousPath = initialPath const cfg = getSaveCfg() - const isSaved = await saveSqsTriggerFromCfg( - initialPath, - cfg, - edit, - wsId!, - usedTriggerKinds - ) + const isSaved = await saveSqsTriggerFromCfg(initialPath, cfg, edit, wsId!, usedTriggerKinds) if (isSaved) { draftSync.discard(previousPath, getSaveCfg()) onUpdate?.(cfg.path) @@ -371,7 +371,11 @@ {/if} {#if useDrawer} - + clearPageDrawerAnchor(TRIGGER_PAGES.sqs.path)} + > import { Alert, Button } from '$lib/components/common' + import { + clearPageDrawerAnchor, + setPageDrawerAnchor + } from '$lib/components/sessions/pageDrawerSession' + import { TRIGGER_PAGES } from '$lib/components/sessions/previewPaths' import TextInput from '$lib/components/text_input/TextInput.svelte' import Drawer from '$lib/components/common/drawer/Drawer.svelte' import DrawerContent from '$lib/components/common/drawer/DrawerContent.svelte' @@ -180,6 +185,7 @@ drawerLoading = true try { drawer?.openDrawer() + setPageDrawerAnchor(TRIGGER_PAGES.websocket.path, ePath) initialPath = ePath itemKind = isFlow ? 'flow' : 'script' edit = true @@ -453,7 +459,11 @@ {/if} {#if useDrawer} - + clearPageDrawerAnchor(TRIGGER_PAGES.websocket.path)} + > Edit + {#if showEditButton} + + + {/if} {/if} {#if !showEditButton && !isCloudHosted() && editInForkAllowed($workspaceStore, $userWorkspaces)}