From e40dc7d9adff5099843b29efbe007c8bcb9c952c Mon Sep 17 00:00:00 2001 From: "fabro-sh-0530[bot]" <281434857+fabro-sh-0530[bot]@users.noreply.github.com> Date: Tue, 5 May 2026 15:33:31 -0400 Subject: [PATCH] Move GitHub token permissions to [run.integrations.github.permissions] (#215) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Token scopes describe what *a run* is authorized to do, not server identity. Today they live under `[server.integrations.github.permissions]`, which can't be overridden by `workflow.toml` / `project.toml` (server keys are stripped from per-workflow layers) — so projects and workflows can't tighten or relax permissions despite the docs already advertising a per-run config. This PR moves them under `[run.integrations.github.permissions]`, where the standard layer-merge (workflow > project > user > defaults) Just Works. Greenfield, no migration shim. ## What changed - **New layer/resolved types** in `fabro-config` and `fabro-types`: `RunIntegrationsLayer`, `RunIntegrationsGithubLayer`, and resolved counterparts. `permissions` becomes a flat `HashMap` post-resolve; empty = no token requested. - **Server schema**: `permissions` removed from `GithubIntegrationLayer` / `GithubIntegrationSettings`. `deny_unknown_fields` rejects the stale path. - **Bundled `workflow.toml` parsing** (`run_manifest.rs`): now goes through `SettingsLayer` via the new `parse_run_layer_from_settings_toml` helper, so stale `[server.integrations.github.permissions]` errors instead of being silently dropped by the old `toml::Table` lift-out. - **Consumers updated**: server preflight, run launch path, and the CLI worker (`runner.rs`) all read run-level permissions. CLI worker previously hardcoded `HashMap::new()` — runs launched via the local CLI path were getting no `GITHUB_TOKEN` regardless of TOML. - **Shared helpers** on `RunIntegrationsGithubSettings`: `is_token_requested()` and `resolve_permissions(lookup)` so server and CLI don't drift. - **OpenAPI + TS client** regenerated; new `RunIntegrationsSettings` / `RunIntegrationsGithubSettings` schemas added, `permissions` removed from `GithubIntegrationSettings`. - **Repo workflows + docs** rewritten to the new path. Docs gain a security-model note (boundary = installation grants; no Fabro-side cap). ## Key design decision: hand-rolled `Combine` for `RunIntegrationsGithubLayer` `ReplaceMap`'s "empty inherits from below" semantics (`maps.rs:76-80`) are wrong here — we want `permissions = {}` in a higher layer to act as an explicit clear. So the layer field is `Option>` with hand-rolled `Combine`: | Higher layer | Lower layer | Result | |---|---|---| | `None` | anything | lower (inherit) | | `Some(map)` | anything | `Some(map)` (full replace, including `Some({})` = clear) | Not derived: the blanket `Option` impl would recurse into the inner `HashMap` and reintroduce empty-fallback. Documented inline in `layers/run.rs`. `InterpString` is preserved through resolve and only flattened to `String` at the start-services boundary, matching the existing pattern. ### Plan Summary - New `[run.integrations.github.permissions]` layer + resolved types; remove from server side. - Hand-rolled `Combine` so empty-wins-as-clear; no change to `ReplaceMap` semantics for other consumers. - Strict `SettingsLayer` parse for bundled `workflow.toml` so stale schema errors loudly. - Both server and CLI worker paths read run-level permissions via shared helpers. - OpenAPI + TS client regenerated; parity test added. - Repo workflow TOMLs and `integrations/github.mdx` rewritten. ### Fabro Details
Ran 0 stages in 61m 23s for $53.41 | Stage | Duration | Cost | Retries | |---|---|---|---| | **Total** | **61m 23s** | **$53.41** | **0** |
Ran ImplementPlan.fabro (12 nodes and 15 edges) ```dot digraph ImplementPlan { graph [ goal="Implement and simplify", model_stylesheet=" * { model: claude-opus-4-7; } " ] rankdir=LR start [shape=Mdiamond, label="Start"] exit [shape=Msquare, label="Exit"] toolchain [label="Toolchain", shape=parallelogram, script="command -v cargo >/dev/null || { curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && sudo ln -sf $HOME/.cargo/bin/* /usr/local/bin/; }; cargo --version 2>&1", max_retries=0] preflight_compile [label="Preflight Compile", shape=parallelogram, script="cargo check -q --workspace 2>&1", max_retries=0] preflight_lint [label="Preflight Lint", shape=parallelogram, script="cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1", max_retries=0] fix_lints [label="Fix Lints", prompt="The preflight lint step failed. Read the build output from context and fix all clippy lint warnings.", max_visits=3] implement [label="Implement", prompt="Read the plan file referenced in the goal and implement every step. Make all the code changes described in the plan. Use red/green TDD."] simplify_opus [label="Simplify (Opus)", prompt="@prompts/simplify.md"] simplify_gpt [label="Simplify (GPT-55)", prompt="@prompts/simplify.md", model="gpt-55"] verify [label="Verify", shape=parallelogram, script="cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1 && cargo nextest run --cargo-quiet --workspace --status-level fail 2>&1 && cargo dev docs refresh 2>&1 && cargo dev docs check 2>&1", goal_gate=true, retry_target="fixup"] fixup [label="Fixup", prompt="The verify step failed. Read the build output from context and fix all clippy lint warnings, test failures, and generated docs errors.", max_visits=3] fmt [label="Format", shape=parallelogram, script="cargo +nightly-2026-04-14 fmt --all 2>&1", max_retries=0] start -> toolchain toolchain -> preflight_compile [condition="outcome=succeeded"] toolchain -> exit preflight_compile -> preflight_lint [condition="outcome=succeeded"] preflight_compile -> exit preflight_lint -> implement [condition="outcome=succeeded"] preflight_lint -> fix_lints fix_lints -> preflight_lint implement -> simplify_opus -> simplify_gpt -> verify verify -> fmt [condition="outcome=succeeded"] verify -> fixup fixup -> verify fmt -> exit } ```
⚒️ Generated with [Fabro](https://fabro.sh) --------- Co-authored-by: Fabro Co-authored-by: Bryan Helmkamp Co-authored-by: Claude Opus 4.7 (1M context) --- .fabro/workflows/gh-triage/workflow.toml | 6 +- .../workflows/implement-issue/workflow.toml | 6 +- apps/fabro-web/app/routes/workflow-detail.tsx | 1 + docs/public/api-reference/fabro-api.yaml | 24 ++- docs/public/integrations/github.mdx | 17 +- .../tests/run_integrations_round_trip.rs | 64 +++++++ .../fabro-cli/src/commands/run/runner.rs | 120 ++++++++++-- lib/crates/fabro-cli/tests/it/cmd/attach.rs | 5 + lib/crates/fabro-config/src/layers/mod.rs | 5 +- lib/crates/fabro-config/src/layers/run.rs | 39 ++++ lib/crates/fabro-config/src/layers/server.rs | 15 +- lib/crates/fabro-config/src/lib.rs | 13 +- lib/crates/fabro-config/src/parse.rs | 5 +- lib/crates/fabro-config/src/resolve/run.rs | 25 ++- lib/crates/fabro-config/src/resolve/server.rs | 13 +- lib/crates/fabro-config/src/run.rs | 21 ++ .../fabro-config/src/tests/resolve_run.rs | 180 ++++++++++++++++++ lib/crates/fabro-server/src/run_manifest.rs | 171 +++++++++++++++-- lib/crates/fabro-server/src/server.rs | 19 +- lib/crates/fabro-types/src/settings/mod.rs | 5 +- lib/crates/fabro-types/src/settings/run.rs | 95 +++++++++ lib/crates/fabro-types/src/settings/server.rs | 14 +- .../src/.openapi-generator/FILES | 2 + .../src/models/github-integration-settings.ts | 1 - .../fabro-api-client/src/models/index.ts | 2 + .../run-integrations-github-settings.ts | 22 +++ .../src/models/run-integrations-settings.ts | 25 +++ .../src/models/run-namespace.ts | 4 + 28 files changed, 808 insertions(+), 111 deletions(-) create mode 100644 lib/crates/fabro-api/tests/run_integrations_round_trip.rs create mode 100644 lib/packages/fabro-api-client/src/models/run-integrations-github-settings.ts create mode 100644 lib/packages/fabro-api-client/src/models/run-integrations-settings.ts diff --git a/.fabro/workflows/gh-triage/workflow.toml b/.fabro/workflows/gh-triage/workflow.toml index 8fafbcab1..3c2d95626 100644 --- a/.fabro/workflows/gh-triage/workflow.toml +++ b/.fabro/workflows/gh-triage/workflow.toml @@ -1,7 +1,5 @@ _version = 1 -[server.integrations.github] - -[server.integrations.github.permissions] -pull_requests = "read" +[run.integrations.github.permissions] issues = "read" +pull_requests = "write" diff --git a/.fabro/workflows/implement-issue/workflow.toml b/.fabro/workflows/implement-issue/workflow.toml index 2a0f45f16..df42f2eea 100644 --- a/.fabro/workflows/implement-issue/workflow.toml +++ b/.fabro/workflows/implement-issue/workflow.toml @@ -1,7 +1,5 @@ _version = 1 -[server.integrations.github] - -[server.integrations.github.permissions] +[run.integrations.github.permissions] +pull_requests = "read" issues = "read" -pull_requests = "write" diff --git a/apps/fabro-web/app/routes/workflow-detail.tsx b/apps/fabro-web/app/routes/workflow-detail.tsx index 8036c17e7..03d988279 100644 --- a/apps/fabro-web/app/routes/workflow-detail.tsx +++ b/apps/fabro-web/app/routes/workflow-detail.tsx @@ -91,6 +91,7 @@ function sampleSettings({ scm: { provider: null, owner: null, repository: null, github: null }, pull_request: null, artifacts: { include: [] }, + integrations: { github: { permissions: {} } }, }, }; } diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 0b499e7fa..eb1bd9bb7 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -7135,7 +7135,6 @@ components: - app_id - client_id - slug - - permissions - webhooks properties: enabled: @@ -7148,10 +7147,6 @@ components: type: ["string", "null"] slug: type: ["string", "null"] - permissions: - type: object - additionalProperties: - type: string webhooks: oneOf: - $ref: "#/components/schemas/IntegrationWebhooksSettings" @@ -7276,6 +7271,7 @@ components: - scm - pull_request - artifacts + - integrations properties: goal: oneOf: @@ -7323,6 +7319,24 @@ components: - type: "null" artifacts: $ref: "#/components/schemas/ArtifactsSettings" + integrations: + $ref: "#/components/schemas/RunIntegrationsSettings" + + RunIntegrationsSettings: + type: object + required: [github] + properties: + github: + $ref: "#/components/schemas/RunIntegrationsGithubSettings" + + RunIntegrationsGithubSettings: + type: object + required: [permissions] + properties: + permissions: + type: object + additionalProperties: + type: string RunGoal: oneOf: diff --git a/docs/public/integrations/github.mdx b/docs/public/integrations/github.mdx index cb9297732..9dab66ab5 100644 --- a/docs/public/integrations/github.mdx +++ b/docs/public/integrations/github.mdx @@ -32,7 +32,7 @@ The rest of this page describes the `app` strategy, which is required for browse | **Checkpoint pushing** | After each workflow stage, Fabro pushes the run branch and metadata branch back to origin from inside the sandbox | | **Auto-PR** | When `[run.pull_request] enabled = true` in the [run config](/execution/run-configuration#runpull_request), Fabro opens a PR from the agent's working branch after a successful run | | **Auto-merge** | When `[run.pull_request] auto_merge = true`, Fabro enables GitHub's auto-merge on created PRs so they merge automatically once required checks pass | -| **Sandbox GITHUB_TOKEN** | When `[server.integrations.github.permissions]` are declared in the server config, Fabro mints a scoped Installation Access Token and injects it as `GITHUB_TOKEN` in the sandbox | +| **Sandbox GITHUB_TOKEN** | When `[run.integrations.github.permissions]` are declared at any layer (workflow, project, or user settings), Fabro mints a scoped Installation Access Token and injects it as `GITHUB_TOKEN` in the sandbox | ## Setup @@ -208,16 +208,21 @@ For public repositories, the clone works without credentials. The token is still ### GITHUB_TOKEN injection -When a run config declares `[github] permissions`, Fabro mints a scoped Installation Access Token at startup and injects it into the sandbox as the `GITHUB_TOKEN` environment variable. Agents running inside the sandbox can use this token for GitHub API calls, cloning additional private repos, or pushing to branches. +When any settings layer declares `[run.integrations.github.permissions]`, Fabro mints a scoped Installation Access Token at startup and injects it into the sandbox as the `GITHUB_TOKEN` environment variable. Agents running inside the sandbox can use this token for GitHub API calls, cloning additional private repos, or pushing to branches. -```toml title="run.toml" -[github] -permissions = { contents = "write", pull_requests = "write" } +```toml title="workflow.toml" +[run.integrations.github.permissions] +contents = "write" +pull_requests = "write" ``` Only the listed permissions are requested — the token is scoped to the minimum access needed. If the GitHub App isn't configured or the repository lacks an installation, the run logs a warning and continues without the token. -This also works in `.fabro/project.toml` as a project-level default, so all workflows in the project automatically get a `GITHUB_TOKEN` without repeating the config in each run TOML. +The permissions table follows the standard layer-merge order (workflow > project > user > defaults). Set defaults at `[run.integrations.github.permissions]` in `~/.fabro/settings.toml` so every run inherits a baseline; tighten or override per-workflow as needed. A higher layer that defines `permissions = {}` clears the inherited map (no token requested). + +#### Security model + +The upper bound on what Fabro will mint is whatever permissions the GitHub App installation has been granted. Fabro does **not** impose a separate server-side cap on the run-level `permissions` map: any value the App has been granted can be requested by run config. Operators must not run untrusted workflow, project, or user TOML against a broadly-scoped GitHub App installation. Preflight prints the resolved permission set so reviewers can see what each run will request. ### Checkpoint pushing diff --git a/lib/crates/fabro-api/tests/run_integrations_round_trip.rs b/lib/crates/fabro-api/tests/run_integrations_round_trip.rs new file mode 100644 index 000000000..243268f1c --- /dev/null +++ b/lib/crates/fabro-api/tests/run_integrations_round_trip.rs @@ -0,0 +1,64 @@ +//! JSON parity test for `RunIntegrationsGithubSettings`. +//! +//! Asserts that the API-side generated `RunIntegrationsGithubSettings` and +//! the canonical Rust resolved type round-trip through the same JSON shape. +//! Covers both the populated and empty-permissions cases. + +use fabro_api::types::{ + RunIntegrationsGithubSettings as ApiRunIntegrationsGithubSettings, + RunIntegrationsSettings as ApiRunIntegrationsSettings, +}; +use fabro_types::settings::run::{RunIntegrationsGithubSettings, RunIntegrationsSettings}; +use serde_json::json; + +#[test] +fn run_integrations_github_settings_round_trips_with_permissions() { + let json_value = json!({ + "permissions": { + "issues": "read", + "contents": "write", + } + }); + + let api: ApiRunIntegrationsGithubSettings = + serde_json::from_value(json_value.clone()).expect("api type should parse"); + let canonical: RunIntegrationsGithubSettings = + serde_json::from_value(json_value.clone()).expect("canonical type should parse"); + + assert_eq!(serde_json::to_value(&api).unwrap(), json_value); + assert_eq!(serde_json::to_value(&canonical).unwrap(), json_value); +} + +#[test] +fn run_integrations_github_settings_round_trips_empty_permissions() { + // Empty map is the resolved form of "no token requested" — must + // serialize as an object, not omitted. + let json_value = json!({ "permissions": {} }); + + let api: ApiRunIntegrationsGithubSettings = + serde_json::from_value(json_value.clone()).expect("api type should parse empty"); + let canonical: RunIntegrationsGithubSettings = + serde_json::from_value(json_value.clone()).expect("canonical type should parse empty"); + + assert_eq!(serde_json::to_value(&api).unwrap(), json_value); + assert_eq!(serde_json::to_value(&canonical).unwrap(), json_value); +} + +#[test] +fn run_integrations_settings_round_trips() { + let json_value = json!({ + "github": { + "permissions": { + "issues": "read", + } + } + }); + + let api: ApiRunIntegrationsSettings = + serde_json::from_value(json_value.clone()).expect("api wrapper should parse"); + let canonical: RunIntegrationsSettings = + serde_json::from_value(json_value.clone()).expect("canonical wrapper should parse"); + + assert_eq!(serde_json::to_value(&api).unwrap(), json_value); + assert_eq!(serde_json::to_value(&canonical).unwrap(), json_value); +} diff --git a/lib/crates/fabro-cli/src/commands/run/runner.rs b/lib/crates/fabro-cli/src/commands/run/runner.rs index 36de6b516..d6d723933 100644 --- a/lib/crates/fabro-cli/src/commands/run/runner.rs +++ b/lib/crates/fabro-cli/src/commands/run/runner.rs @@ -4,7 +4,6 @@ std::io::BufReader; not on a Tokio path" )] -use std::collections::HashMap; use std::io::{BufRead as StdBufRead, BufReader as StdBufReader}; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -18,7 +17,7 @@ use fabro_interview::{ }; use fabro_store::{EventEnvelope, RunProjection, RunProjectionReducer}; use fabro_types::settings::InterpString; -use fabro_types::settings::run::RunMode; +use fabro_types::settings::run::{RunMode, RunNamespace}; use fabro_types::{ ArtifactUpload, EventBody, FailureReason, Principal, RunBlobId, RunEvent, RunId, WorkflowSettings, @@ -117,7 +116,12 @@ pub(crate) async fn execute( artifact_sink, run_control: Some(run_control), github_app, - github_permissions: HashMap::new(), + github_permissions: run_spec + .settings + .run + .integrations + .github + .resolve_permissions(process_env_var), vault, on_node: None, registry_override: None, @@ -517,30 +521,23 @@ fn maybe_build_github_credentials( ) -> Result> { let resolved_run = &settings.run; let resolved_server = ServerSettingsBuilder::load_default().ok(); - let required_github_credentials = (resolved_run.execution.mode != RunMode::DryRun - && clone_sandbox_requires_github_credentials(&resolved_run.sandbox.provider)) - || resolved_server - .as_ref() - .is_some_and(|settings| !settings.server.integrations.github.permissions.is_empty()); - let pull_request_enabled = - resolved_run.execution.mode != RunMode::DryRun && resolved_run.pull_request.is_some(); - let strategy = resolved_server - .as_ref() - .map(|settings| settings.server.integrations.github.strategy) + let server_ns = resolved_server.as_ref().map(|s| &s.server); + let strategy = server_ns + .map(|server| server.integrations.github.strategy) .unwrap_or_default(); - let app_id = resolved_server - .as_ref() - .and_then(|settings| settings.server.integrations.github.app_id.as_ref()) + let app_id = server_ns + .and_then(|server| server.integrations.github.app_id.as_ref()) .map(InterpString::as_source); - let app_slug = resolved_server - .as_ref() - .and_then(|settings| settings.server.integrations.github.slug.as_ref()) + let app_slug = server_ns + .and_then(|server| server.integrations.github.slug.as_ref()) .map(InterpString::as_source); - if required_github_credentials { + if requires_github_credentials(resolved_run) { return build_github_credentials(strategy, app_id.as_deref(), app_slug.as_deref(), vault); } + let pull_request_enabled = + resolved_run.execution.mode != RunMode::DryRun && resolved_run.pull_request.is_some(); if pull_request_enabled { return Ok(build_github_credentials( strategy, @@ -555,6 +552,26 @@ fn maybe_build_github_credentials( Ok(None) } +#[expect( + clippy::disallowed_methods, + reason = "CLI worker InterpString resolution facade for {{ env.* }} values." +)] +fn process_env_var(name: &str) -> Option { + std::env::var(name).ok() +} + +/// Hard-gate for the CLI worker path: a run-level token is requested, or +/// a clone-based sandbox in non-dry-run mode will need credentials to +/// pull the repository. Pull-request-driven credential acquisition is +/// handled separately by the caller as a soft fallback. +fn requires_github_credentials(run: &RunNamespace) -> bool { + if run.integrations.github.is_token_requested() { + return true; + } + run.execution.mode != RunMode::DryRun + && clone_sandbox_requires_github_credentials(&run.sandbox.provider) +} + fn clone_sandbox_requires_github_credentials(provider: &str) -> bool { matches!(provider, "docker" | "daytona") } @@ -941,4 +958,67 @@ mod tests { assert!(credential.contains("vault-key")); } + + mod requires_github_credentials_truth_table { + //! Truth-table coverage for the worker-side credential gate. + //! `InterpString` → `String` resolution is tested in `fabro-types` + //! next to `RunIntegrationsGithubSettings::resolve_permissions`. + + use std::collections::HashMap; + + use fabro_types::settings::InterpString; + use fabro_types::settings::run::{ + RunIntegrationsGithubSettings, RunIntegrationsSettings, RunMode, RunNamespace, + RunSandboxSettings, + }; + + use super::super::requires_github_credentials; + + fn run_with( + permissions: HashMap, + provider: &str, + mode: RunMode, + ) -> RunNamespace { + let mut run = RunNamespace::default(); + run.execution.mode = mode; + run.sandbox = RunSandboxSettings { + provider: provider.to_string(), + ..RunSandboxSettings::default() + }; + run.integrations = RunIntegrationsSettings { + github: RunIntegrationsGithubSettings { permissions }, + }; + run + } + + #[test] + fn requires_github_credentials_when_permissions_non_empty() { + let permissions = HashMap::from([("issues".to_string(), InterpString::parse("read"))]); + // Even with local sandbox + dry-run, non-empty permissions + // force credential acquisition. + let run = run_with(permissions, "local", RunMode::DryRun); + assert!(requires_github_credentials(&run)); + } + + #[test] + fn requires_github_credentials_for_clone_based_provider() { + let run = run_with(HashMap::new(), "docker", RunMode::Normal); + assert!(requires_github_credentials(&run)); + + let daytona = run_with(HashMap::new(), "daytona", RunMode::Normal); + assert!(requires_github_credentials(&daytona)); + } + + #[test] + fn does_not_require_github_credentials_for_local_clean_run() { + let run = run_with(HashMap::new(), "local", RunMode::Normal); + assert!(!requires_github_credentials(&run)); + } + + #[test] + fn does_not_require_github_credentials_for_clone_provider_in_dry_run() { + let run = run_with(HashMap::new(), "docker", RunMode::DryRun); + assert!(!requires_github_credentials(&run)); + } + } } diff --git a/lib/crates/fabro-cli/tests/it/cmd/attach.rs b/lib/crates/fabro-cli/tests/it/cmd/attach.rs index 7c6728f39..75d3d2950 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/attach.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/attach.rs @@ -615,6 +615,11 @@ fn attach_json_errors_without_prompting_for_human_input() { }, "hooks": [], "inputs": {}, + "integrations": { + "github": { + "permissions": {} + } + }, "interviews": { "discord": null, "provider": null, diff --git a/lib/crates/fabro-config/src/layers/mod.rs b/lib/crates/fabro-config/src/layers/mod.rs index 60af5e79e..22fb9b490 100644 --- a/lib/crates/fabro-config/src/layers/mod.rs +++ b/lib/crates/fabro-config/src/layers/mod.rs @@ -24,8 +24,9 @@ pub use run::{ GitAuthorLayer, HookAgentMarker, HookEntry, HookTlsMode, InterviewProviderLayer, InterviewsLayer, LocalSandboxLayer, McpEntryLayer, ModelRefOrSplice, NotificationProviderLayer, NotificationRouteLayer, PrepareStep, RunAgentLayer, RunArtifactsLayer, RunCheckpointLayer, - RunExecutionLayer, RunGitLayer, RunGoalLayer, RunLayer, RunModelLayer, RunPrepareLayer, - RunPullRequestLayer, RunSandboxLayer, RunScmLayer, ScmGitHubLayer, StringOrSplice, + RunExecutionLayer, RunGitLayer, RunGoalLayer, RunIntegrationsGithubLayer, RunIntegrationsLayer, + RunLayer, RunModelLayer, RunPrepareLayer, RunPullRequestLayer, RunSandboxLayer, RunScmLayer, + ScmGitHubLayer, StringOrSplice, }; pub use server::{ DiscordIntegrationLayer, GithubIntegrationLayer, IntegrationWebhooksLayer, diff --git a/lib/crates/fabro-config/src/layers/run.rs b/lib/crates/fabro-config/src/layers/run.rs index 8786315aa..b4aaabda1 100644 --- a/lib/crates/fabro-config/src/layers/run.rs +++ b/lib/crates/fabro-config/src/layers/run.rs @@ -9,6 +9,7 @@ use fabro_types::settings::run::{ use fabro_types::settings::{Duration, InterpString, ModelRef, Size}; use serde::{Deserialize, Serialize}; +use super::combine::Combine; use super::maps::{MergeMap, ReplaceMap, StickyMap}; use super::splice_array::SPLICE_MARKER; @@ -51,6 +52,44 @@ pub struct RunLayer { pub pull_request: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub artifacts: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub integrations: Option, +} + +/// `[run.integrations]` — run-level integration knobs. +#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize, fabro_macros::Combine)] +#[serde(deny_unknown_fields)] +pub struct RunIntegrationsLayer { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub github: Option, +} + +/// `[run.integrations.github]` — runtime GitHub token shape. +/// +/// `Combine` is hand-rolled (not derived) for two reasons: +/// 1. `HashMap: Combine` is not implemented in this +/// crate, so `#[derive(Combine)]` would not even compile. +/// 2. The `ReplaceMap` "empty inherits from below" semantics (`maps.rs:76-80`) +/// are the wrong fit: we want `Some({})` from a higher layer to be honored +/// as an explicit clear (no token requested) rather than fall through to a +/// lower layer's permissions. +/// +/// Note: this diverges from sibling map fields like `RunLayer::metadata` +/// (`ReplaceMap`), where empty-table-means-inherit. Document the +/// difference for workflow authors reading the schema by analogy. +#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct RunIntegrationsGithubLayer { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub permissions: Option>, +} + +impl Combine for RunIntegrationsGithubLayer { + fn combine(self, other: Self) -> Self { + Self { + permissions: self.permissions.or(other.permissions), + } + } } /// The source of a run's goal, either inline literal text or a reference to diff --git a/lib/crates/fabro-config/src/layers/server.rs b/lib/crates/fabro-config/src/layers/server.rs index 61560e313..f3a6f9a2c 100644 --- a/lib/crates/fabro-config/src/layers/server.rs +++ b/lib/crates/fabro-config/src/layers/server.rs @@ -8,7 +8,6 @@ use fabro_types::settings::{Duration, InterpString}; use serde::{Deserialize, Serialize}; use super::LogFilter; -use super::maps::StickyMap; #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize, fabro_macros::Combine)] #[serde(deny_unknown_fields)] @@ -213,19 +212,17 @@ pub struct ServerIntegrationsLayer { #[serde(deny_unknown_fields)] pub struct GithubIntegrationLayer { #[serde(default, skip_serializing_if = "Option::is_none")] - pub enabled: Option, + pub enabled: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub strategy: Option, + pub strategy: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub app_id: Option, + pub app_id: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub client_id: Option, + pub client_id: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub slug: Option, - #[serde(default, skip_serializing_if = "StickyMap::is_empty")] - pub permissions: StickyMap, + pub slug: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub webhooks: Option, + pub webhooks: Option, } /// `[server.integrations.slack]` — Slack workspace credentials and defaults. diff --git a/lib/crates/fabro-config/src/lib.rs b/lib/crates/fabro-config/src/lib.rs index 1394f08b9..d440c714e 100644 --- a/lib/crates/fabro-config/src/lib.rs +++ b/lib/crates/fabro-config/src/lib.rs @@ -44,12 +44,13 @@ pub use layers::{ LogFilter, McpEntryLayer, MergeMap, ModelRefOrSplice, NotificationProviderLayer, NotificationRouteLayer, ObjectStoreLocalLayer, ObjectStoreS3Layer, PrepareStep, ProjectLayer, ReplaceMap, RunAgentLayer, RunArtifactsLayer, RunCheckpointLayer, RunExecutionLayer, - RunGitLayer, RunGoalLayer, RunLayer, RunModelLayer, RunPrepareLayer, RunPullRequestLayer, - RunSandboxLayer, RunScmLayer, ScmGitHubLayer, ServerApiLayer, ServerArtifactsLayer, - ServerAuthGithubLayer, ServerAuthLayer, ServerIntegrationsLayer, ServerIpAllowlistLayer, - ServerIpAllowlistOverrideLayer, ServerLayer, ServerListenLayer, ServerLoggingLayer, - ServerSchedulerLayer, ServerSlateDbLayer, ServerStorageLayer, ServerWebLayer, - SlackIntegrationLayer, StickyMap, StringOrSplice, TeamsIntegrationLayer, WorkflowLayer, + RunGitLayer, RunGoalLayer, RunIntegrationsGithubLayer, RunIntegrationsLayer, RunLayer, + RunModelLayer, RunPrepareLayer, RunPullRequestLayer, RunSandboxLayer, RunScmLayer, + ScmGitHubLayer, ServerApiLayer, ServerArtifactsLayer, ServerAuthGithubLayer, ServerAuthLayer, + ServerIntegrationsLayer, ServerIpAllowlistLayer, ServerIpAllowlistOverrideLayer, ServerLayer, + ServerListenLayer, ServerLoggingLayer, ServerSchedulerLayer, ServerSlateDbLayer, + ServerStorageLayer, ServerWebLayer, SlackIntegrationLayer, StickyMap, StringOrSplice, + TeamsIntegrationLayer, WorkflowLayer, }; pub(crate) use layers::{Combine, SettingsLayer}; pub use logging::{resolve_log_destination, resolve_log_destination_with_env}; diff --git a/lib/crates/fabro-config/src/parse.rs b/lib/crates/fabro-config/src/parse.rs index 50d842b31..66f3f9fde 100644 --- a/lib/crates/fabro-config/src/parse.rs +++ b/lib/crates/fabro-config/src/parse.rs @@ -115,7 +115,10 @@ fn rename_hint(key: &str) -> Option { "max_concurrent_runs" => "rename to `[server.scheduler]` field", "fabro" => "rename to `[project]`; `fabro.root` becomes `project.directory`", "git" => "split into `[run.git]` (local git behavior) and `[server.integrations.github]`", - "github" => "rename to `[server.integrations.github]`", + "github" => { + "split into `[server.integrations.github]` (App identity/auth) and \ + `[run.integrations.github.permissions]` (sandbox token scopes)" + } "slack" => "move under `[server.integrations.slack]`", "log" => "rename to `[server.logging]` or `[cli.logging]` depending on owner", "prevent_idle_sleep" => "rename to `[cli.exec] prevent_idle_sleep`", diff --git a/lib/crates/fabro-config/src/resolve/run.rs b/lib/crates/fabro-config/src/resolve/run.rs index 8bb9b0703..c538d70ec 100644 --- a/lib/crates/fabro-config/src/resolve/run.rs +++ b/lib/crates/fabro-config/src/resolve/run.rs @@ -4,9 +4,9 @@ use fabro_types::settings::run::{ GitAuthorSettings, HookDefinition, HookType, InterviewProviderSettings, LocalSandboxSettings, McpServerSettings, McpTransport, MergeStrategy, NotificationProviderSettings, NotificationRouteSettings, PullRequestSettings, RunAgentSettings, RunCheckpointSettings, - RunExecutionSettings, RunGitSettings, RunGoal, RunInterviewsSettings, RunModelSettings, - RunNamespace, RunPrepareSettings, RunSandboxSettings, RunScmSettings, ScmGitHubSettings, - TlsMode, + RunExecutionSettings, RunGitSettings, RunGoal, RunIntegrationsGithubSettings, + RunIntegrationsSettings, RunInterviewsSettings, RunModelSettings, RunNamespace, + RunPrepareSettings, RunSandboxSettings, RunScmSettings, ScmGitHubSettings, TlsMode, }; use super::ResolveError; @@ -14,8 +14,9 @@ use crate::{ DaytonaDockerfileLayer, DaytonaSandboxLayer, HookAgentMarker, HookEntry, HookTlsMode, InterviewProviderLayer, InterviewsLayer, McpEntryLayer, ModelRefOrSplice, NotificationProviderLayer, NotificationRouteLayer, RunAgentLayer, RunArtifactsLayer, - RunCheckpointLayer, RunExecutionLayer, RunGitLayer, RunGoalLayer, RunLayer, RunModelLayer, - RunPrepareLayer, RunPullRequestLayer, RunSandboxLayer, RunScmLayer, StringOrSplice, + RunCheckpointLayer, RunExecutionLayer, RunGitLayer, RunGoalLayer, RunIntegrationsLayer, + RunLayer, RunModelLayer, RunPrepareLayer, RunPullRequestLayer, RunSandboxLayer, RunScmLayer, + StringOrSplice, }; pub fn resolve_run(layer: &RunLayer, errors: &mut Vec) -> RunNamespace { @@ -46,9 +47,23 @@ pub fn resolve_run(layer: &RunLayer, errors: &mut Vec) -> RunNames scm: resolve_scm(layer.scm.as_ref()), pull_request: resolve_pull_request(layer.pull_request.as_ref()), artifacts: resolve_artifacts(layer.artifacts.as_ref()), + integrations: resolve_integrations(layer.integrations.as_ref()), } } +fn resolve_integrations(layer: Option<&RunIntegrationsLayer>) -> RunIntegrationsSettings { + let github = layer + .and_then(|integrations| integrations.github.as_ref()) + .map(|github| RunIntegrationsGithubSettings { + // Collapse `Option>` -> `HashMap<...>`: both `None` + // and `Some({})` resolve to an empty map (no token requested). + // The presence distinction is only meaningful at merge time. + permissions: github.permissions.clone().unwrap_or_default(), + }) + .unwrap_or_default(); + RunIntegrationsSettings { github } +} + fn resolve_goal(goal: Option<&RunGoalLayer>) -> Option { match goal? { RunGoalLayer::Inline(value) => Some(RunGoal::Inline(value.clone())), diff --git a/lib/crates/fabro-config/src/resolve/server.rs b/lib/crates/fabro-config/src/resolve/server.rs index 6805fd6bd..902b1ff3c 100644 --- a/lib/crates/fabro-config/src/resolve/server.rs +++ b/lib/crates/fabro-config/src/resolve/server.rs @@ -457,13 +457,12 @@ fn resolve_integrations( github: layer .and_then(|integrations| integrations.github.as_ref()) .map(|github| GithubIntegrationSettings { - enabled: github.enabled.unwrap_or(true), - strategy: github.strategy.unwrap_or_default(), - app_id: github.app_id.clone(), - client_id: github.client_id.clone(), - slug: github.slug.clone(), - permissions: github.permissions.clone().into_inner(), - webhooks: github.webhooks.as_ref().map(|webhooks| { + enabled: github.enabled.unwrap_or(true), + strategy: github.strategy.unwrap_or_default(), + app_id: github.app_id.clone(), + client_id: github.client_id.clone(), + slug: github.slug.clone(), + webhooks: github.webhooks.as_ref().map(|webhooks| { resolve_github_webhooks(webhooks, "server.integrations.github.webhooks", errors) }), }) diff --git a/lib/crates/fabro-config/src/run.rs b/lib/crates/fabro-config/src/run.rs index 08e5f99cf..7fb5ec65f 100644 --- a/lib/crates/fabro-config/src/run.rs +++ b/lib/crates/fabro-config/src/run.rs @@ -25,6 +25,27 @@ pub(crate) fn load_run_config(path: &Path) -> Result { load_settings_path(path) } +/// Parse a settings TOML source string and extract its `[run]` layer. +/// +/// Goes through the strict `SettingsLayer` `FromStr` impl, so valid settings +/// domains (`_version`, `[workflow]`, `[run]`, `[server]`, etc.) parse cleanly, +/// while unknown top-level domains or unknown nested keys under a known domain +/// trip `deny_unknown_fields`. Bundled +/// `workflow.toml` configurations parse through this path so stale +/// schema (e.g. `[server.integrations.github.permissions]` after the +/// move to `[run.integrations.github.permissions]`) is rejected up front +/// rather than silently dropped by `RunLayer::try_from(toml::Value)`. +/// +/// Exists as a public helper because `SettingsLayer` itself is +/// `pub(crate)`, so external crates cannot replicate this two-line dance +/// inline. +pub fn parse_run_layer_from_settings_toml(source: &str) -> Result { + let layer = source + .parse::() + .map_err(|err| crate::Error::parse("Failed to parse run config TOML", err))?; + Ok(layer.run.unwrap_or_default()) +} + /// Resolve a graph path relative to a workflow.toml. #[must_use] pub fn resolve_graph_path(workflow_toml: &Path, graph_relative: &str) -> PathBuf { diff --git a/lib/crates/fabro-config/src/tests/resolve_run.rs b/lib/crates/fabro-config/src/tests/resolve_run.rs index 15ab2dce9..acb887813 100644 --- a/lib/crates/fabro-config/src/tests/resolve_run.rs +++ b/lib/crates/fabro-config/src/tests/resolve_run.rs @@ -80,3 +80,183 @@ name = "sonnet" ); assert_eq!(settings.model.name, Some(InterpString::parse("sonnet"))); } + +mod run_integrations_github_permissions { + //! Layer + resolver tests for `[run.integrations.github.permissions]`. + //! + //! `[run.integrations.github]` uses a hand-rolled `Combine` impl so a + //! higher layer that sets `permissions = {}` clears the inherited map + //! ("empty wins as clear"), and an absent block inherits from below. + + use std::collections::HashMap; + + use fabro_types::settings::InterpString; + + use crate::layers::Combine; + use crate::{SettingsLayer, WorkflowSettingsBuilder}; + + fn parse_settings(source: &str) -> SettingsLayer { + source + .parse::() + .expect("fixture should parse via SettingsLayer") + } + + fn one_perm(key: &str, value: &str) -> HashMap { + HashMap::from([(key.to_string(), InterpString::parse(value))]) + } + + #[test] + fn workflow_layer_parses_run_level_permissions() { + let layer = parse_settings( + r#" +_version = 1 + +[run.integrations.github.permissions] +issues = "read" +"#, + ); + let github = layer + .run + .as_ref() + .and_then(|run| run.integrations.as_ref()) + .and_then(|integrations| integrations.github.as_ref()) + .expect("permissions block should be parsed into RunIntegrationsGithubLayer"); + let permissions = github + .permissions + .as_ref() + .expect("permissions table should be present"); + assert_eq!(permissions.len(), 1); + assert_eq!( + permissions.get("issues"), + Some(&InterpString::parse("read")) + ); + } + + #[test] + fn workflow_replaces_user_permissions_wholesale() { + let workflow = parse_settings( + r#" +_version = 1 + +[run.integrations.github.permissions] +issues = "write" +"#, + ); + let user = parse_settings( + r#" +_version = 1 + +[run.integrations.github.permissions] +contents = "read" +"#, + ); + let merged = workflow.combine(user); + + let resolved = WorkflowSettingsBuilder::from_layer(&merged) + .expect("merged settings should resolve") + .run; + + assert_eq!( + resolved.integrations.github.permissions, + one_perm("issues", "write",) + ); + } + + #[test] + fn absent_higher_layer_inherits_lower_permissions() { + let workflow = parse_settings("_version = 1\n"); + let user = parse_settings( + r#" +_version = 1 + +[run.integrations.github.permissions] +contents = "read" +"#, + ); + let merged = workflow.combine(user); + + let resolved = WorkflowSettingsBuilder::from_layer(&merged) + .expect("merged settings should resolve") + .run; + + assert_eq!( + resolved.integrations.github.permissions, + one_perm("contents", "read",) + ); + } + + #[test] + fn empty_higher_layer_clears_inherited_permissions() { + // Workflow declares `permissions = {}` -> Some(empty map). The + // hand-rolled `Combine` keeps Some over fallback, so the resolved + // map is empty (no token requested) — empty-wins-as-clear. + let workflow = parse_settings( + r" +_version = 1 + +[run.integrations.github] +permissions = {} +", + ); + let user = parse_settings( + r#" +_version = 1 + +[run.integrations.github.permissions] +contents = "read" +"#, + ); + let merged = workflow.combine(user); + + let resolved = WorkflowSettingsBuilder::from_layer(&merged) + .expect("merged settings should resolve") + .run; + + assert!( + resolved.integrations.github.permissions.is_empty(), + "empty higher layer should clear inherited permissions, got {:?}", + resolved.integrations.github.permissions + ); + } + + #[test] + fn server_integrations_github_permissions_is_now_unknown_field() { + let err = r#" +_version = 1 + +[server.integrations.github.permissions] +issues = "read" +"# + .parse::() + .expect_err("stale [server.integrations.github.permissions] must error"); + let message = err.to_string(); + assert!( + message.contains("permissions") || message.contains("unknown field"), + "expected unknown-field error mentioning permissions, got: {message}" + ); + } + + #[test] + fn resolver_preserves_interp_string_in_permissions() { + let resolved = WorkflowSettingsBuilder::from_toml( + r#" +_version = 1 + +[run.integrations.github.permissions] +issues = "{{ env.GH_PERM_LEVEL }}" +"#, + ) + .expect("env-token permissions should resolve") + .run; + + let issues = resolved + .integrations + .github + .permissions + .get("issues") + .expect("issues permission should be present"); + // Resolver does NOT eagerly resolve env tokens; the `InterpString` + // form is preserved for late binding by the consumer. + assert_eq!(issues.as_source(), "{{ env.GH_PERM_LEVEL }}"); + } +} diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index 713dcc50c..cd1b766f3 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -7,6 +7,7 @@ use std::time::Duration; use anyhow::{Context as _, Result, anyhow, bail}; use fabro_api::types; use fabro_auth::auth_issue_message; +use fabro_config::run::parse_run_layer_from_settings_toml; use fabro_config::{ CliLayer, CliOutputLayer, DaytonaDockerfileLayer, DockerSandboxLayer, LocalSandboxLayer, ReplaceMap, RunExecutionLayer, RunLayer, RunModelLayer, RunSandboxLayer, @@ -24,7 +25,6 @@ use fabro_sandbox::daytona::DaytonaConfig; use fabro_sandbox::redact::redact_auth_url; use fabro_sandbox::{DockerSandboxOptions, Sandbox, SandboxProvider, SandboxSpec}; use fabro_static::EnvVars; -use fabro_types::settings::ServerNamespace; use fabro_types::settings::cli::OutputVerbosity; use fabro_types::settings::interp::InterpString; use fabro_types::settings::run::{ @@ -292,16 +292,14 @@ fn root_workflow_run_layer(workflow: &BundledWorkflow) -> Result { return Ok(RunLayer::default()); }; - let mut document: toml::Table = config - .source - .parse() + // Parse via `SettingsLayer` so unknown nested keys (like a stale + // `[server.integrations.github.permissions]` after the move to + // `[run.integrations.github.permissions]`) trip + // `deny_unknown_fields`. Other valid top-level domains + // (`_version`, `[workflow]`, `[server.*]`) parse cleanly and the + // builder ignores everything outside `[run]` for this code path. + let mut run = parse_run_layer_from_settings_toml(&config.source) .context("Failed to parse run config TOML")?; - let mut run = document - .remove("run") - .map(toml::Value::try_into::) - .transpose() - .context("Failed to parse run config TOML")? - .unwrap_or_default(); resolve_manifest_dockerfile(&mut run, &config.path, &workflow.files)?; Ok(run) } @@ -494,7 +492,7 @@ async fn build_preflight_report( sandbox_provider }; let needs_github_credentials = - sandbox_provider.is_clone_based() || !github_integration.permissions.is_empty(); + sandbox_provider.is_clone_based() || resolved_run.integrations.github.is_token_requested(); let github_app = if needs_github_credentials { state .github_credentials(github_integration) @@ -529,7 +527,7 @@ async fn build_preflight_report( &configured_providers, ) .await; - run_github_token_check(&mut checks, prepared, &server_settings.server, github_app).await; + run_github_token_check(&mut checks, prepared, &resolved_run, github_app).await; let checks_ok = sandbox_ok && repository_access_ok && llm_ok; @@ -1184,22 +1182,19 @@ fn runtime_docker_config(settings: &DockerSettings) -> DockerSandboxOptions { async fn run_github_token_check( checks: &mut Vec, prepared: &PreparedManifest, - settings: &ServerNamespace, + resolved_run: &RunNamespace, github_app: Option, ) { - if settings.integrations.github.permissions.is_empty() { + if !resolved_run.integrations.github.is_token_requested() { return; } // Resolve InterpString permission values eagerly for token minting and // for display in the preflight report. - let github_permissions: HashMap = settings + let github_permissions = resolved_run .integrations .github - .permissions - .iter() - .map(|(k, v)| (k.clone(), v.as_source())) - .collect(); + .resolve_permissions(process_env_var); let perm_details = github_permissions .iter() @@ -1228,7 +1223,7 @@ async fn run_github_token_check( name: "GitHub Token".into(), status: CheckStatus::Warning, summary: "skipped".into(), - details: vec![], + details: perm_details, remediation: Some("No GitHub credentials or origin URL available".to_string()), }), } @@ -1818,6 +1813,57 @@ app_id = "fixture-app-id" assert_eq!(response.checks.sections[0].checks.len(), 2); } + #[tokio::test] + async fn preflight_runs_github_token_check_when_run_level_permissions_declared() { + // When a workflow declares `[run.integrations.github.permissions]`, + // `run_github_token_check` is invoked and surfaces a "GitHub Token" + // entry in the preflight report. With no configured GitHub App + // credentials in the test fixture, the check status is + // Warning/skipped — the important assertion is that the entry + // *exists*, proving the gate now reads from run-level config. + let state = crate::test_support::test_app_state(); + let mut manifest = minimal_manifest(); + manifest.workflows.get_mut("workflow.fabro").unwrap().config = + Some(types::ManifestWorkflowConfig { + path: "workflow.toml".to_string(), + source: r#"_version = 1 + +[run.sandbox] +provider = "local" + +[run.integrations.github.permissions] +issues = "read" +"# + .to_string(), + }); + + let prepared = prepare_manifest( + &manifest_run_defaults(Some(&default_settings_fixture())), + &manifest, + ) + .unwrap(); + let validated = validate_prepared_manifest(&prepared).unwrap(); + assert!(!validated.has_errors()); + + let (response, _ok) = run_preflight(state.as_ref(), &prepared, &validated) + .await + .unwrap(); + + assert!( + response.checks.sections[0] + .checks + .iter() + .any(|check| check.name == "GitHub Token"), + "expected GitHub Token check to run when run-level permissions are set; \ + checks were {:?}", + response.checks.sections[0] + .checks + .iter() + .map(|c| c.name.as_str()) + .collect::>() + ); + } + #[tokio::test] async fn preflight_allows_pull_request_enabled_without_github_credentials() { let state = crate::test_support::test_app_state(); @@ -1980,4 +2026,89 @@ digraph Demo { ); assert!(response_mock.calls_async().await >= 1); } + + mod root_workflow_run_layer_tests { + //! `root_workflow_run_layer` parses bundled workflow.toml through + //! the strict `SettingsLayer` schema, so unknown fields anywhere in + //! the document trip `deny_unknown_fields`. + + use fabro_workflow::ManifestPath; + use fabro_workflow::workflow_bundle::{BundledWorkflow, ParsedWorkflowConfig}; + + use super::super::root_workflow_run_layer; + + fn workflow_with_config(source: &str) -> BundledWorkflow { + BundledWorkflow { + path: ManifestPath::from_wire("workflow.fabro").expect("path should be valid"), + source: "digraph G {}".to_string(), + config: Some(ParsedWorkflowConfig { + path: ManifestPath::from_wire("workflow.toml") + .expect("config path should be valid"), + source: source.to_string(), + }), + files: std::collections::HashMap::new(), + } + } + + #[test] + fn parses_run_integrations_github_permissions() { + let workflow = workflow_with_config( + r#"_version = 1 + +[run.integrations.github.permissions] +issues = "read" +"#, + ); + + let run = root_workflow_run_layer(&workflow).expect("workflow.toml should parse"); + let github = run + .integrations + .as_ref() + .and_then(|integrations| integrations.github.as_ref()) + .expect("integrations.github should be present"); + let permissions = github + .permissions + .as_ref() + .expect("permissions should be present"); + assert_eq!(permissions.len(), 1); + assert!(permissions.contains_key("issues")); + } + + #[test] + fn rejects_stale_server_integrations_github_permissions() { + let workflow = workflow_with_config( + r#"_version = 1 + +[server.integrations.github.permissions] +issues = "read" +"#, + ); + + let err = root_workflow_run_layer(&workflow) + .expect_err("stale [server.integrations.github.permissions] should be rejected"); + let message = format!("{err:#}"); + assert!( + message.contains("permissions") || message.contains("unknown field"), + "expected unknown-field error, got: {message}" + ); + } + + #[test] + fn accepts_workflow_block_and_version() { + let workflow = workflow_with_config( + r#"_version = 1 + +[workflow] +name = "demo" + +[run.integrations.github.permissions] +contents = "read" +"#, + ); + + let run = + root_workflow_run_layer(&workflow).expect("workflow + run blocks should parse"); + assert!(run.integrations.is_some()); + } + } } diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 4df49b12a..ffd42443f 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -2718,7 +2718,7 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { .is_some_and(|origin| !origin.trim().is_empty()); let pull_request_can_use_github_credentials = settings.execution.mode != RunMode::DryRun && settings.pull_request.is_some(); - if !github_settings.permissions.is_empty() { + if settings.integrations.github.is_token_requested() { state.github_credentials(github_settings) } else if clone_can_use_github_credentials || pull_request_can_use_github_credentials { match state.github_credentials(github_settings) { @@ -2754,16 +2754,13 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { return; } }; - let github_permissions = github_settings - .permissions - .iter() - .map(|(name, value)| { - let resolved = value - .resolve(process_env_var) - .map_or_else(|_| value.as_source(), |resolved| resolved.value); - (name.clone(), resolved) - }) - .collect(); + let github_permissions = persisted + .run_spec() + .settings + .run + .integrations + .github + .resolve_permissions(process_env_var); let services = operations::StartServices { run_id, cancel_token: cancel_token.clone(), diff --git a/lib/crates/fabro-types/src/settings/mod.rs b/lib/crates/fabro-types/src/settings/mod.rs index 003e70c82..54b8c6aa1 100644 --- a/lib/crates/fabro-types/src/settings/mod.rs +++ b/lib/crates/fabro-types/src/settings/mod.rs @@ -40,8 +40,9 @@ pub use run::{ GitAuthorSettings, HookDefinition, HookType, InterviewProviderSettings, McpServerSettings, McpTransport, NotificationProviderSettings, NotificationRouteSettings, PullRequestSettings, RunAgentSettings, RunCheckpointSettings, RunExecutionSettings, RunGitSettings, RunGoal, - RunInterviewsSettings, RunModelSettings, RunNamespace, RunPrepareSettings, RunSandboxSettings, - RunScmSettings, ScmGitHubSettings, TlsMode, + RunIntegrationsGithubSettings, RunIntegrationsSettings, RunInterviewsSettings, + RunModelSettings, RunNamespace, RunPrepareSettings, RunSandboxSettings, RunScmSettings, + ScmGitHubSettings, TlsMode, }; pub use server::{ DiscordIntegrationSettings, GithubIntegrationSettings, IntegrationWebhooksSettings, diff --git a/lib/crates/fabro-types/src/settings/run.rs b/lib/crates/fabro-types/src/settings/run.rs index 7619a92fb..ba57c28d4 100644 --- a/lib/crates/fabro-types/src/settings/run.rs +++ b/lib/crates/fabro-types/src/settings/run.rs @@ -35,6 +35,101 @@ pub struct RunNamespace { pub scm: RunScmSettings, pub pull_request: Option, pub artifacts: ArtifactsSettings, + pub integrations: RunIntegrationsSettings, +} + +/// `[run.integrations]` — run-level integration knobs. +#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] +pub struct RunIntegrationsSettings { + pub github: RunIntegrationsGithubSettings, +} + +/// `[run.integrations.github]` — runtime GitHub token shape. +/// +/// `permissions` is empty when no token should be requested. The +/// presence-vs-clear distinction is only meaningful at the layer-merge +/// stage; the resolved form collapses both `None` and `Some({})` into an +/// empty map. +#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] +pub struct RunIntegrationsGithubSettings { + pub permissions: HashMap, +} + +impl RunIntegrationsGithubSettings { + /// Whether the run config asks Fabro to mint a `GITHUB_TOKEN` for the + /// sandbox. Empty `permissions` means "no token requested" — the only + /// resolved-form sentinel for this state. + pub fn is_token_requested(&self) -> bool { + !self.permissions.is_empty() + } + + /// Resolve every `permissions` value's `{{ env.* }}` tokens via + /// `lookup`, falling back to `InterpString::as_source()` when + /// resolution fails so callers see a recognizable diagnostic instead + /// of a silently dropped key. The `lookup` seam keeps tests free of + /// process-env coupling; production callers pass a thin wrapper over + /// `std::env::var`. + pub fn resolve_permissions(&self, mut lookup: F) -> HashMap + where + F: FnMut(&str) -> Option, + { + self.permissions + .iter() + .map(|(name, value)| { + let resolved = value + .resolve(&mut lookup) + .map_or_else(|_| value.as_source(), |resolved| resolved.value); + (name.clone(), resolved) + }) + .collect() + } +} + +#[cfg(test)] +mod run_integrations_github_tests { + use super::{HashMap, InterpString, RunIntegrationsGithubSettings}; + + fn settings(permissions: &[(&str, &str)]) -> RunIntegrationsGithubSettings { + RunIntegrationsGithubSettings { + permissions: permissions + .iter() + .map(|(k, v)| ((*k).to_string(), InterpString::parse(v))) + .collect(), + } + } + + #[test] + fn is_token_requested_reflects_permissions_presence() { + assert!(!settings(&[]).is_token_requested()); + assert!(settings(&[("issues", "read")]).is_token_requested()); + } + + #[test] + fn resolve_permissions_substitutes_env_tokens_via_lookup() { + let s = settings(&[("issues", "{{ env.GH_PERM_LEVEL }}"), ("contents", "read")]); + let resolved = s.resolve_permissions(|name| match name { + "GH_PERM_LEVEL" => Some("write".to_string()), + _ => None, + }); + assert_eq!(resolved.get("issues"), Some(&"write".to_string())); + assert_eq!(resolved.get("contents"), Some(&"read".to_string())); + } + + #[test] + fn resolve_permissions_falls_back_to_source_when_lookup_fails() { + let s = settings(&[("issues", "{{ env.GH_PERM_MISSING }}")]); + let resolved = s.resolve_permissions(|_| None); + assert_eq!( + resolved.get("issues"), + Some(&"{{ env.GH_PERM_MISSING }}".to_string()) + ); + } + + #[test] + fn resolve_permissions_is_empty_for_empty_settings() { + let s: HashMap = settings(&[]).resolve_permissions(|_| None); + assert!(s.is_empty()); + } } /// The resolved source of a run goal. diff --git a/lib/crates/fabro-types/src/settings/server.rs b/lib/crates/fabro-types/src/settings/server.rs index 2aec85d8e..16a5718f9 100644 --- a/lib/crates/fabro-types/src/settings/server.rs +++ b/lib/crates/fabro-types/src/settings/server.rs @@ -5,7 +5,6 @@ //! scheduler, logging, integrations). Same-host and split-host deployments //! use the same schema. -use std::collections::HashMap; use std::net::SocketAddr; use std::time::Duration as StdDuration; @@ -267,13 +266,12 @@ pub struct ServerIntegrationsSettings { #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct GithubIntegrationSettings { - pub enabled: bool, - pub strategy: GithubIntegrationStrategy, - pub app_id: Option, - pub client_id: Option, - pub slug: Option, - pub permissions: HashMap, - pub webhooks: Option, + pub enabled: bool, + pub strategy: GithubIntegrationStrategy, + pub app_id: Option, + pub client_id: Option, + pub slug: Option, + pub webhooks: Option, } #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] diff --git a/lib/packages/fabro-api-client/src/.openapi-generator/FILES b/lib/packages/fabro-api-client/src/.openapi-generator/FILES index 2dffdba54..53cf5a73f 100644 --- a/lib/packages/fabro-api-client/src/.openapi-generator/FILES +++ b/lib/packages/fabro-api-client/src/.openapi-generator/FILES @@ -231,6 +231,8 @@ models/run-goal-file.ts models/run-goal-inline.ts models/run-goal.ts models/run-interviews-settings.ts +models/run-integrations-github-settings.ts +models/run-integrations-settings.ts models/run-list-item.ts models/run-manifest.ts models/run-mode.ts diff --git a/lib/packages/fabro-api-client/src/models/github-integration-settings.ts b/lib/packages/fabro-api-client/src/models/github-integration-settings.ts index 487a5b511..cc8cf1e5a 100644 --- a/lib/packages/fabro-api-client/src/models/github-integration-settings.ts +++ b/lib/packages/fabro-api-client/src/models/github-integration-settings.ts @@ -26,7 +26,6 @@ export interface GithubIntegrationSettings { 'app_id': string | null; 'client_id': string | null; 'slug': string | null; - 'permissions': { [key: string]: string; }; 'webhooks': IntegrationWebhooksSettings | null; } diff --git a/lib/packages/fabro-api-client/src/models/index.ts b/lib/packages/fabro-api-client/src/models/index.ts index fcb9e05ef..c41afe7a6 100644 --- a/lib/packages/fabro-api-client/src/models/index.ts +++ b/lib/packages/fabro-api-client/src/models/index.ts @@ -210,6 +210,8 @@ export * from './run-goal'; export * from './run-goal-file'; export * from './run-goal-inline'; export * from './run-interviews-settings'; +export * from './run-integrations-github-settings'; +export * from './run-integrations-settings'; export * from './run-list-item'; export * from './run-manifest'; export * from './run-mode'; diff --git a/lib/packages/fabro-api-client/src/models/run-integrations-github-settings.ts b/lib/packages/fabro-api-client/src/models/run-integrations-github-settings.ts new file mode 100644 index 000000000..740e2967b --- /dev/null +++ b/lib/packages/fabro-api-client/src/models/run-integrations-github-settings.ts @@ -0,0 +1,22 @@ +/* tslint:disable */ +/* eslint-disable */ +/** + * Fabro Run API + * HTTP API for managing Fabro workflow run executions. + * + * The version of the OpenAPI document: 0.1.0 + * + * + * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). + * https://openapi-generator.tech + * Do not edit the class manually. + */ + + + +export interface RunIntegrationsGithubSettings { + 'permissions': { [key: string]: string; }; +} + + + diff --git a/lib/packages/fabro-api-client/src/models/run-integrations-settings.ts b/lib/packages/fabro-api-client/src/models/run-integrations-settings.ts new file mode 100644 index 000000000..de44e953f --- /dev/null +++ b/lib/packages/fabro-api-client/src/models/run-integrations-settings.ts @@ -0,0 +1,25 @@ +/* tslint:disable */ +/* eslint-disable */ +/** + * Fabro Run API + * HTTP API for managing Fabro workflow run executions. + * + * The version of the OpenAPI document: 0.1.0 + * + * + * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). + * https://openapi-generator.tech + * Do not edit the class manually. + */ + + +// May contain unused imports in some cases +// @ts-ignore +import type { RunIntegrationsGithubSettings } from './run-integrations-github-settings'; + +export interface RunIntegrationsSettings { + 'github': RunIntegrationsGithubSettings; +} + + + diff --git a/lib/packages/fabro-api-client/src/models/run-namespace.ts b/lib/packages/fabro-api-client/src/models/run-namespace.ts index cb8ec4e8d..ef2cf3d1d 100644 --- a/lib/packages/fabro-api-client/src/models/run-namespace.ts +++ b/lib/packages/fabro-api-client/src/models/run-namespace.ts @@ -42,6 +42,9 @@ import type { RunGitSettings } from './run-git-settings'; import type { RunGoal } from './run-goal'; // May contain unused imports in some cases // @ts-ignore +import type { RunIntegrationsSettings } from './run-integrations-settings'; +// May contain unused imports in some cases +// @ts-ignore import type { RunInterviewsSettings } from './run-interviews-settings'; // May contain unused imports in some cases // @ts-ignore @@ -77,5 +80,6 @@ export interface RunNamespace { 'scm': RunScmSettings; 'pull_request': PullRequestSettings | null; 'artifacts': ArtifactsSettings; + 'integrations': RunIntegrationsSettings; }