diff --git a/run.json b/run.json index 424efeb0d..287369adc 100644 --- a/run.json +++ b/run.json @@ -508,25 +508,31 @@ "kind": "running" }, "status_updated_at": "2026-05-07T21:31:26.204215Z", - "last_event_at": "2026-05-07T21:33:53.013295Z", + "last_event_at": "2026-05-07T21:45:31.670502Z", "pending_control": null, "checkpoint": { - "timestamp": "2026-05-07T21:36:06.737029Z", - "current_node": "preflight_lint", + "timestamp": "2026-05-07T21:45:31.885150Z", + "current_node": "implement", "completed_nodes": [ "start", "toolchain", "preflight_compile", - "preflight_lint" + "preflight_lint", + "implement" ], "node_retries": {}, "context_values": { "internal.work_dir": "/home/daytona/workspace", + "thread.preflight_lint.current_node": "implement", "command.output": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "last_stage": "implement", "failure_class": "", - "current_node": "preflight_lint", + "current_node": "implement", + "response.implement": "TypeScript client is up to date.\n\nLooking at the goal one more time — most of the implementation is already done in this branch (verified by working build, lint, and full nextest run). All work described in sections 1-12 is in place:\n\n**Summary of implementation status verified:**\n\n1. ✅ **Layer schema (`run.rs`)**: `RunIntegrationsLayer` and `RunIntegrationsGithubLayer` with hand-rolled `Combine` honoring `Some({})` as clear sentinel.\n2. ✅ **Server schema (`server.rs`)**: `permissions` removed from `GithubIntegrationLayer`.\n3. ✅ **Resolved types (`fabro-types/.../run.rs`)**: `RunIntegrationsSettings` / `RunIntegrationsGithubSettings` with collapsed-Option `permissions`, plus `is_token_requested()` and `resolve_permissions()` helpers (with built-in unit tests).\n4. ✅ **Resolver (`resolve/run.rs`)**: `resolve_integrations` collapses `Option` → `HashMap`. Server resolver no longer emits `permissions`.\n5. ✅ **Server consumers**: `run_manifest.rs` (clone-credential gate, `run_github_token_check`), `server.rs:2862` (forced-credential gate) and `server.rs:2898` (StartServices construction) all read run-level settings.\n5b. ✅ **Bundled-workflow TOML**: `root_workflow_run_layer` parses via `parse_run_layer_from_settings_toml` (which goes through `SettingsLayer`), so stale `[server.integrations.github.permissions]` is rejected by `deny_unknown_fields`.\n6. ✅ **CLI worker**: `runner.rs` reads `run_spec.settings.run.integrations.github.resolve_permissions(...)`; `requires_github_credentials` private helper plus truth-table tests.\n7. ✅ **Parse hint (`parse.rs:118-121`)**: `[github]` legacy hint split into identity vs. permissions.\n8. ✅ **OpenAPI**: `RunIntegrationsSettings` / `RunIntegrationsGithubSettings` schemas added; `RunNamespace.integrations` required; server `GithubIntegrationSettings.permissions` removed. JSON parity test (`fabro-api/tests/run_integrations_round_trip.rs`) covers populated and empty cases.\n9. ✅ **Repo TOMLs**: `gh-list`, `gh-triage`, `implement-issue` workflow TOMLs use `[run.integrations.github.permissions]`.\n11. ✅ **Docs**: `docs/public/integrations/github.mdx`, `docs/public/execution/run-configuration.mdx`, `docs/public/reference/user-configuration.mdx` updated.\n12. ✅ **Tests**: \n - Layer parsing + merge tests in `fabro-config/src/tests/resolve_run.rs:84-` (presence, replace, inherit, clear, deny_unknown).\n - Resolver tests preserve `InterpString`.\n - Preflight tests (`run_manifest.rs:1807`) for run-level permissions.\n - Bundled-workflow positive + negative tests (`run_manifest.rs:2044, 2068, 2094`).\n - CLI worker truth-table tests at `runner.rs:993-1054`.\n - JSON parity test in fabro-api.\n\n**Verification:**\n- `cargo build --workspace`: clean\n- `cargo nextest run --workspace`: 5258 passed, 182 skipped (e2e)\n- `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings`: clean\n- `bun run typecheck` in `apps/fabro-web`: clean\n\nThe implementation described in the plan is already complete on this branch — every step (1–12) has corresponding code/test changes in the tree, and the full test suite plus lint pass without modification. No further code changes are needed.", + "last_response": "TypeScript client is up to date.\n\nLooking at the goal one more time — most of the implementation is already done in this branch (verified by working build, lint, and full nextest run). All work desc", "graph.rankdir": "LR", - "internal.thread_id": "preflight_compile", + "internal.thread_id": "preflight_lint", + "internal.retry_count.implement": 0, "graph.model_stylesheet": "\n * { model: claude-opus-4-7; }\n ", "internal.fidelity": "compact", "graph.goal": "# Move GitHub token permissions from `[server.integrations.github]` to `[run.integrations.github]`\n\n## Context\n\nToday `[server.integrations.github.permissions]` controls what scopes Fabro requests on the Installation Access Token it injects into the sandbox as `GITHUB_TOKEN`. This is conceptually wrong:\n\n- Token permissions describe what *this run* is authorized to do, not the server's identity.\n- The current key is server-only. `workflow.toml` and `project.toml` cannot override it (`builders.rs:394` strips `server.*` from per-workflow layers), so projects/workflows can't tighten or relax permissions.\n- The public docs (`integrations/github.mdx:211-220`) already describe a per-run config, which never existed in code. The intent has always been run-level.\n\nGreenfield app — no migration, no backwards compat. Moving permissions to `[run.integrations.github.permissions]` makes the natural layer-merge (workflow > project > user > defaults) Just Work: server admins set defaults in `~/.fabro/settings.toml`, projects and workflows override.\n\n## Security model\n\nRun config is trusted policy input for sandbox token scopes. The upper bound on what Fabro can mint is the GitHub App installation's granted permissions; Fabro does **not** impose a separate server-side cap. Operators must not run untrusted workflow/project/user TOML against a broadly-scoped App installation. Preflight prints requested permissions so reviewers can see them; no enforcement layer beyond GitHub's own.\n\n## Design\n\nNew TOML path: `[run.integrations.github.permissions]`. Creates a fresh `[run.integrations]` namespace for future run-level integration knobs.\n\n**Merge semantics** (presence-aware, hand-rolled). `ReplaceMap` does NOT work here: `maps.rs:76-80` is `if self.0.is_empty() { other } else { self }`, i.e. an empty higher layer falls back to the inherited map. We need empty-wins-as-clear, so we don't reuse `ReplaceMap` (and don't change global semantics for other users).\n\nLayer field: `pub permissions: Option>`.\n\n| Higher layer | Lower layer | Result |\n|---|---|---|\n| `None` | anything | lower (inherit) |\n| `Some(map)` | anything | `Some(map)` (full replace, including `Some({})` = clear) |\n\nHand-roll `Combine` on `RunIntegrationsGithubLayer`:\n```rust\nimpl Combine for RunIntegrationsGithubLayer {\n fn combine(self, other: Self) -> Self {\n Self { permissions: self.permissions.or(other.permissions) }\n }\n}\n```\nDon't derive — the blanket `Option` impl recurses into the inner type and would reintroduce the empty-fallback bug.\n\nResolved type collapses the Option: `pub permissions: HashMap` where empty = no token requested. The presence distinction only matters during merge.\n\n**Interpolation**: keep `InterpString` in the resolved type. Resolve to `String` at the start-services construction boundary, matching the existing pattern at `server.rs:2763-2772`. No early resolution in `resolve_run`.\n\n## Changes\n\n### 1. Config schema — `lib/crates/fabro-config/src/layers/run.rs`\n\nAdd new layer types. `RunIntegrationsLayer` derives `Combine` normally; `RunIntegrationsGithubLayer` does NOT — hand-roll `Combine` (see Design section) so `Some({})` is honored as a clear sentinel.\n\n```rust\n/// `[run.integrations]` — run-level integration knobs.\n#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize, fabro_macros::Combine)]\n#[serde(deny_unknown_fields)]\npub struct RunIntegrationsLayer {\n #[serde(default, skip_serializing_if = \"Option::is_none\")]\n pub github: Option,\n}\n\n/// `[run.integrations.github]` — runtime GitHub token shape.\n#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)]\n#[serde(deny_unknown_fields)]\npub struct RunIntegrationsGithubLayer {\n #[serde(default, skip_serializing_if = \"Option::is_none\")]\n pub permissions: Option>,\n}\n\nimpl Combine for RunIntegrationsGithubLayer {\n fn combine(self, other: Self) -> Self {\n Self { permissions: self.permissions.or(other.permissions) }\n }\n}\n```\n\nAdd `pub integrations: Option` to `RunLayer`.\n\n### 2. Config schema — server side\n\n`lib/crates/fabro-config/src/layers/server.rs:226`: remove `permissions: StickyMap` from `GithubIntegrationLayer`. Server struct keeps only identity/auth/webhook fields.\n\n### 3. Resolved types — `lib/crates/fabro-types/src/settings/run.rs`\n\nResolved types collapse the layer-time `Option` (presence is only meaningful during merge):\n\n```rust\npub struct RunIntegrationsSettings {\n pub github: RunIntegrationsGithubSettings,\n}\n\npub struct RunIntegrationsGithubSettings {\n pub permissions: HashMap, // empty = no token\n}\n```\n\nAdd `integrations: RunIntegrationsSettings` to `RunNamespace`. Drop `permissions` from the resolved `GithubIntegrationSettings` in fabro-types.\n\n### 4. Resolver — `lib/crates/fabro-config/src/resolve/run.rs`\n\nAdd `fn resolve_integrations(layer: Option<&RunIntegrationsLayer>) -> RunIntegrationsSettings`. Pass through `InterpString`s untouched — do NOT resolve env vars here. Collapse `Option>` → `HashMap<...>` (None and Some({}) both become empty).\n\n`lib/crates/fabro-config/src/resolve/server.rs:465`: drop the line that copies `permissions` into the resolved github settings.\n\n### 5. Server consumer — `lib/crates/fabro-server`\n\nFour read sites swap from `server_settings.server.integrations.github.permissions` to `run_spec.settings.run.integrations.github.permissions` (a flat `HashMap`; empty = no token):\n\n- `run_manifest.rs:488` — clone-credential gate inside `prepare_manifest`.\n- `run_manifest.rs:1184-1235` — `run_github_token_check`. Change signature to take resolved run permissions; resolve `InterpString`s inside the function for both minting and the report.\n- `server.rs:2727` — forced-credential gate in run launch path.\n- `server.rs:2763-2772` — InterpString → String resolution for `StartServices.github_permissions`. Read from run settings instead of server settings; same resolution logic.\n\n### 5b. Bundled-workflow TOML parsing — `lib/crates/fabro-server/src/run_manifest.rs:290-307`\n\n`root_workflow_run_layer` currently parses the workflow TOML as a raw `toml::Table`, lifts out `run`, and silently discards every other top-level key. That means stale `[server.integrations.github.permissions]` is not caught by `deny_unknown_fields`.\n\nNaive \"reject any non-`run` key\" would break valid workflow TOML (every workflow.toml in the repo has `_version = 1`; `hello/workflow.toml` has `[workflow]`). `SettingsLayer` (`layers/settings.rs:21-37`) is the schema for a settings file: `_version`, `project`, `workflow`, `run`, `cli`, `server`, `features`.\n\nFix: parse the source via `source.parse::()` and take `layer.run.unwrap_or_default()`. That gives:\n- valid top-level domains parse without error;\n- stale `[server.integrations.github.permissions]` fails because `permissions` is no longer a known field on `GithubIntegrationLayer` (section 2) and the layer has `serde(deny_unknown_fields)`;\n- shape-valid `[server.*]` in a workflow.toml is silently ignored downstream by `builders.rs:394`, preserving today's behavior (workflow.toml shouldn't set server config, but doesn't blow up either).\n\n`resolve_manifest_dockerfile(&mut run, &config.path, &workflow.files)` (line 305) still runs on the extracted `RunLayer`.\n\n### 6. CLI worker — `lib/crates/fabro-cli/src/commands/run/runner.rs` (P0, was missing)\n\nTwo reads on the CLI launch path that today still point at the server-side field:\n\n- `runner.rs:120` — `github_permissions: HashMap::new()` is hardcoded. Source from `run_spec.settings.run.integrations.github.permissions`, applying the same `InterpString` → `String` resolution as the server side.\n- `runner.rs:514-555` — `maybe_build_github_credentials`. Line 524 reads `settings.server.integrations.github.permissions.is_empty()` to decide whether credentials are required. Swap to read the run-level permissions. Identity fields (`strategy`, `app_id`, `slug`) at lines 527-538 stay on the server side.\n\nTo make this testable, factor two private helpers in `runner.rs`:\n- `fn resolve_run_github_permissions(run: &RunNamespace) -> HashMap` — InterpString resolution loop, same as the server side. Reuse server-side helper if one exists; otherwise extract a shared one in fabro-config or fabro-server.\n- `fn requires_github_credentials(run: &RunNamespace, server: Option<&ServerNamespace>) -> bool` — folds the existing clone/PR/permissions gates.\n\nBoth are pure functions of resolved settings; unit-test them in `#[cfg(test)] mod tests` inside `runner.rs`. Don't try to reach private items from an integration test in `tests/it/`.\n\nWithout this fix, CLI-launched runs silently get no `GITHUB_TOKEN` regardless of TOML.\n\nDownstream is unchanged: `StartServices.github_permissions` → `SandboxEnvSpec.github_permissions` (`fabro-workflow/src/operations/start.rs:99`, `pipeline/types.rs:225`) → `mint_github_token` + `GITHUB_TOKEN` injection at `pipeline/initialize.rs:240`.\n\n### 7. Parse hints — `lib/crates/fabro-config/src/parse.rs:117-118`\n\nUpdate the legacy-`[github]` migration hint to distinguish identity vs. permissions. Suggested wording:\n- `\"github\"` → `\"split into [server.integrations.github] (App identity/auth) and [run.integrations.github.permissions] (sandbox token scopes)\"`.\n\nAlso: ensure `[server.integrations.github]` with a `permissions` subkey produces a `deny_unknown_fields` error pointing at the new path. If automatic, no extra code; if not, add a targeted parse-time check.\n\n### 8. OpenAPI — `docs/public/api-reference/fabro-api.yaml`\n\nWire shape mirrors the resolved Rust types (no `Option`s, no nullability):\n\n- **Remove** `permissions` from `GithubIntegrationSettings` (line 7212): drop the property and remove from `required`.\n- **Add** new schemas:\n - `RunIntegrationsSettings`: `properties: { github: { $ref: \"#/components/schemas/RunIntegrationsGithubSettings\" } }`, `required: [\"github\"]`.\n - `RunIntegrationsGithubSettings`: `properties: { permissions: { type: object, additionalProperties: { type: string } } }`, `required: [\"permissions\"]`.\n- **Add** `integrations: { $ref: \"#/components/schemas/RunIntegrationsSettings\" }` to `RunNamespace.properties` and to `RunNamespace.required`.\n\nThe collapsed-Option resolved types (section 3) make this clean: `github` is always present, `permissions` is always an object (possibly empty). No `nullable`, no `oneOf`, no `skip_serializing_if` to debate.\n\nType ownership: progenitor auto-generates new run DTOs (Explore confirmed no `with_replacement` for run types today). No new `with_replacement` entries; do not split into hand-rolled DTOs unless a real semantic divergence emerges. Add a fabro-api JSON parity test asserting OpenAPI's `RunIntegrationsGithubSettings` round-trips through the resolved Rust type, including the empty-permissions case.\n\nRegenerate TS client: `cd lib/packages/fabro-api-client && bun run generate`.\n\n### 9. Repo TOMLs — rewrite\n\n- `.fabro/workflows/gh-triage/workflow.toml`\n- `.fabro/workflows/implement-issue/workflow.toml`\n- `.fabro/workflows/gh-list/workflow.toml`\n\nEach: `[server.integrations.github.permissions]` → `[run.integrations.github.permissions]`.\n\n### 10. User settings (advisory)\n\n`~/.fabro/settings.toml` should be rewritten to put a default at `[run.integrations.github.permissions]`. Don't auto-edit; cover in verification.\n\n### 11. Docs — `docs/public/integrations/github.mdx`\n\n- Line 35 (overview table): rewrite the row to reference `[run.integrations.github.permissions]`.\n- Lines 209-220 (\"GITHUB_TOKEN injection\"): rewrite. Show canonical run-level path. Note `~/.fabro/settings.toml` is the natural place for server defaults because it's the user layer of the same merge stack. Drop the `[github]` shorthand from line 211 (never existed in code).\n- Add the security-model note (boundary = installation grants, no Fabro-side cap).\n\n### 12. Tests\n\n**Layer parsing + merge** (`lib/crates/fabro-config/src/tests/`, parse real TOML at each layer; not just resolver-level fixtures):\n- Parse a workflow.toml with `[run.integrations.github.permissions]` — assert the layer round-trips.\n- Merge user `{ contents = \"read\" }` + workflow `{ issues = \"write\" }` — assert workflow fully replaces: result is `{ issues = \"write\" }`.\n- Merge user `{ contents = \"read\" }` + workflow absent (no `[run.integrations]` block) — assert inheritance: result is `{ contents = \"read\" }`.\n- Merge user `{ contents = \"read\" }` + workflow `permissions = {}` — assert clear: resolved permissions is empty.\n- Negative: `[server.integrations.github.permissions]` in user/project/workflow TOML must error via `deny_unknown_fields`.\n- Negative (bundled workflow path, P2): targeted test exercising `root_workflow_run_layer` (`run_manifest.rs:290`) with a workflow.toml containing a stale `[server.integrations.github.permissions]` block — assert it errors via `deny_unknown_fields` after the rewrite to parse through `SettingsLayer`. Pair with a positive test asserting `_version = 1` and a `[workflow]` block still parse cleanly through this code path.\n\n**Resolver** (`resolve/run.rs` tests): assert `InterpString` is preserved in resolved settings, not flattened to `String`. Assert resolved `permissions` is `HashMap` (Option collapsed).\n\n**Preflight** (`run_manifest.rs:1822` and surrounds):\n- Add a case where the run config sets `permissions = { issues = \"read\" }` and the GitHub App is configured — assert `run_github_token_check` reports `Pass`.\n- Update `server_settings_fixture` (`run_manifest.rs:1390`) usage in any test that previously set permissions through it; rewrite to set via run layer.\n\n**CLI worker path** (P0): unit-test the new private helpers in `runner.rs`'s `#[cfg(test)] mod tests`:\n- `resolve_run_github_permissions` — given a `RunNamespace` with `InterpString` permissions referencing env vars, returns the resolved `HashMap`.\n- `requires_github_credentials` — exercises each truth-table case (clone needed, PR enabled, permissions non-empty, all-absent).\n- Do not try to assert `StartServices.github_permissions` from an integration test — those internals are private. End-to-end behavior is covered by the smoke runs in Verification.\n\n## Verification\n\n1. `cargo build --workspace` clean.\n2. `cargo nextest run -p fabro-config` — layer/resolve tests pass, including the new override + replace-semantics + deny-unknown tests.\n3. `cargo nextest run -p fabro-server -p fabro-cli` — preflight + run-manifest + worker tests pass.\n4. `cargo nextest run -p fabro-api` — JSON parity test for the new run integrations schema passes.\n5. Smoke run via server (manual):\n - Update `~/.fabro/settings.toml` to put `[run.integrations.github.permissions]` defaults (`pull_requests = \"read\"`, `issues = \"read\"`).\n - Restart `fabro server`.\n - `fabro run gh-list --no-retro`; both stages exit 0 with PR/issue listings on stdout.\n - `fabro logs ` shows `gh pr list` returning data; no \"populate the GH_TOKEN\" error.\n6. Smoke run via CLI worker path (manual): same as above but with a `fabro run` invocation that uses the local-CLI worker path (not HTTP). Confirms P0 fix.\n7. Override test: in `gh-list/workflow.toml`, set `permissions = { issues = \"write\" }` over a server default of `read`. Confirm the minted-token preflight summary shows `issues: write` and not `read`.\n8. Tightening test: in another workflow, set `permissions = {}`. Confirm preflight reports no token requested and the sandbox env has no `GITHUB_TOKEN`.\n9. `cd apps/fabro-web && bun run typecheck && bun test` — generated TS client compiles against the new schema.\n\n## Open questions\n\n1. Anything else worth promoting to `[run.integrations.*]` now (Slack, Discord run-time config)? Recommendation: leave empty until a concrete need lands; don't speculate.\n2. Should preflight surface the *resolved* permission strings in its report, or the raw `InterpString` source? Recommendation: resolved, so reviewers see what the App will actually be asked for; treat unresolved env-var fallbacks as a preflight warning.\n3. `resolve_run_github_permissions` location — should the InterpString-resolution loop live in fabro-config (shared by server + CLI worker), or in each consumer? Recommendation: extract to fabro-config / fabro-types alongside the resolved type, so server (`server.rs:2763-2772`) and CLI (`runner.rs`) both call the same helper. Avoids two copies drifting.\n", @@ -566,6 +572,38 @@ "notes": "Script completed: cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1", "usage": null }, + "implement": { + "status": "succeeded", + "context_updates": { + "response.implement": "TypeScript client is up to date.\n\nLooking at the goal one more time — most of the implementation is already done in this branch (verified by working build, lint, and full nextest run). All work described in sections 1-12 is in place:\n\n**Summary of implementation status verified:**\n\n1. ✅ **Layer schema (`run.rs`)**: `RunIntegrationsLayer` and `RunIntegrationsGithubLayer` with hand-rolled `Combine` honoring `Some({})` as clear sentinel.\n2. ✅ **Server schema (`server.rs`)**: `permissions` removed from `GithubIntegrationLayer`.\n3. ✅ **Resolved types (`fabro-types/.../run.rs`)**: `RunIntegrationsSettings` / `RunIntegrationsGithubSettings` with collapsed-Option `permissions`, plus `is_token_requested()` and `resolve_permissions()` helpers (with built-in unit tests).\n4. ✅ **Resolver (`resolve/run.rs`)**: `resolve_integrations` collapses `Option` → `HashMap`. Server resolver no longer emits `permissions`.\n5. ✅ **Server consumers**: `run_manifest.rs` (clone-credential gate, `run_github_token_check`), `server.rs:2862` (forced-credential gate) and `server.rs:2898` (StartServices construction) all read run-level settings.\n5b. ✅ **Bundled-workflow TOML**: `root_workflow_run_layer` parses via `parse_run_layer_from_settings_toml` (which goes through `SettingsLayer`), so stale `[server.integrations.github.permissions]` is rejected by `deny_unknown_fields`.\n6. ✅ **CLI worker**: `runner.rs` reads `run_spec.settings.run.integrations.github.resolve_permissions(...)`; `requires_github_credentials` private helper plus truth-table tests.\n7. ✅ **Parse hint (`parse.rs:118-121`)**: `[github]` legacy hint split into identity vs. permissions.\n8. ✅ **OpenAPI**: `RunIntegrationsSettings` / `RunIntegrationsGithubSettings` schemas added; `RunNamespace.integrations` required; server `GithubIntegrationSettings.permissions` removed. JSON parity test (`fabro-api/tests/run_integrations_round_trip.rs`) covers populated and empty cases.\n9. ✅ **Repo TOMLs**: `gh-list`, `gh-triage`, `implement-issue` workflow TOMLs use `[run.integrations.github.permissions]`.\n11. ✅ **Docs**: `docs/public/integrations/github.mdx`, `docs/public/execution/run-configuration.mdx`, `docs/public/reference/user-configuration.mdx` updated.\n12. ✅ **Tests**: \n - Layer parsing + merge tests in `fabro-config/src/tests/resolve_run.rs:84-` (presence, replace, inherit, clear, deny_unknown).\n - Resolver tests preserve `InterpString`.\n - Preflight tests (`run_manifest.rs:1807`) for run-level permissions.\n - Bundled-workflow positive + negative tests (`run_manifest.rs:2044, 2068, 2094`).\n - CLI worker truth-table tests at `runner.rs:993-1054`.\n - JSON parity test in fabro-api.\n\n**Verification:**\n- `cargo build --workspace`: clean\n- `cargo nextest run --workspace`: 5258 passed, 182 skipped (e2e)\n- `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings`: clean\n- `bun run typecheck` in `apps/fabro-web`: clean\n\nThe implementation described in the plan is already complete on this branch — every step (1–12) has corresponding code/test changes in the tree, and the full test suite plus lint pass without modification. No further code changes are needed.", + "last_stage": "implement", + "last_response": "TypeScript client is up to date.\n\nLooking at the goal one more time — most of the implementation is already done in this branch (verified by working build, lint, and full nextest run). All work desc" + }, + "notes": "Stage completed: implement", + "usage": { + "input": { + "usage": { + "model": { + "provider": "anthropic", + "model_id": "claude-opus-4-7" + }, + "tokens": { + "input_tokens": 74042, + "output_tokens": 7264, + "reasoning_tokens": 0, + "cache_read_tokens": 2590218, + "cache_write_tokens": 84306 + } + }, + "facts": { + "provider": "anthropic", + "cache_write_5m_tokens": 84306, + "cache_write_1h_tokens": 0 + } + }, + "total_usd_micros": 2373831 + } + }, "toolchain": { "status": "succeeded", "context_updates": { @@ -576,11 +614,12 @@ "usage": null } }, - "next_node_id": "implement", + "next_node_id": "simplify_opus", "node_visits": { "preflight_lint": 1, "toolchain": 1, "start": 1, + "implement": 1, "preflight_compile": 1 } }, @@ -737,6 +776,84 @@ "toolchain": 1 } } + ], + [ + 46, + { + "timestamp": "2026-05-07T21:36:17.485891Z", + "current_node": "preflight_lint", + "completed_nodes": [ + "start", + "toolchain", + "preflight_compile", + "preflight_lint" + ], + "node_retries": {}, + "context_values": { + "internal.fidelity": "compact", + "internal.retry_count.toolchain": 0, + "internal.retry_count.start": 0, + "thread.toolchain.current_node": "preflight_compile", + "failure_signature": "", + "internal.retry_count.preflight_compile": 0, + "internal.work_dir": "/home/daytona/workspace", + "current_node": "preflight_lint", + "outcome": "succeeded", + "internal.node_visit_count": 1, + "internal.retry_count.preflight_lint": 0, + "internal.thread_id": "preflight_compile", + "graph.model_stylesheet": "\n * { model: claude-opus-4-7; }\n ", + "graph.rankdir": "LR", + "command.output": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "command.stderr": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "failure_class": "", + "graph.goal": "# Move GitHub token permissions from `[server.integrations.github]` to `[run.integrations.github]`\n\n## Context\n\nToday `[server.integrations.github.permissions]` controls what scopes Fabro requests on the Installation Access Token it injects into the sandbox as `GITHUB_TOKEN`. This is conceptually wrong:\n\n- Token permissions describe what *this run* is authorized to do, not the server's identity.\n- The current key is server-only. `workflow.toml` and `project.toml` cannot override it (`builders.rs:394` strips `server.*` from per-workflow layers), so projects/workflows can't tighten or relax permissions.\n- The public docs (`integrations/github.mdx:211-220`) already describe a per-run config, which never existed in code. The intent has always been run-level.\n\nGreenfield app — no migration, no backwards compat. Moving permissions to `[run.integrations.github.permissions]` makes the natural layer-merge (workflow > project > user > defaults) Just Work: server admins set defaults in `~/.fabro/settings.toml`, projects and workflows override.\n\n## Security model\n\nRun config is trusted policy input for sandbox token scopes. The upper bound on what Fabro can mint is the GitHub App installation's granted permissions; Fabro does **not** impose a separate server-side cap. Operators must not run untrusted workflow/project/user TOML against a broadly-scoped App installation. Preflight prints requested permissions so reviewers can see them; no enforcement layer beyond GitHub's own.\n\n## Design\n\nNew TOML path: `[run.integrations.github.permissions]`. Creates a fresh `[run.integrations]` namespace for future run-level integration knobs.\n\n**Merge semantics** (presence-aware, hand-rolled). `ReplaceMap` does NOT work here: `maps.rs:76-80` is `if self.0.is_empty() { other } else { self }`, i.e. an empty higher layer falls back to the inherited map. We need empty-wins-as-clear, so we don't reuse `ReplaceMap` (and don't change global semantics for other users).\n\nLayer field: `pub permissions: Option>`.\n\n| Higher layer | Lower layer | Result |\n|---|---|---|\n| `None` | anything | lower (inherit) |\n| `Some(map)` | anything | `Some(map)` (full replace, including `Some({})` = clear) |\n\nHand-roll `Combine` on `RunIntegrationsGithubLayer`:\n```rust\nimpl Combine for RunIntegrationsGithubLayer {\n fn combine(self, other: Self) -> Self {\n Self { permissions: self.permissions.or(other.permissions) }\n }\n}\n```\nDon't derive — the blanket `Option` impl recurses into the inner type and would reintroduce the empty-fallback bug.\n\nResolved type collapses the Option: `pub permissions: HashMap` where empty = no token requested. The presence distinction only matters during merge.\n\n**Interpolation**: keep `InterpString` in the resolved type. Resolve to `String` at the start-services construction boundary, matching the existing pattern at `server.rs:2763-2772`. No early resolution in `resolve_run`.\n\n## Changes\n\n### 1. Config schema — `lib/crates/fabro-config/src/layers/run.rs`\n\nAdd new layer types. `RunIntegrationsLayer` derives `Combine` normally; `RunIntegrationsGithubLayer` does NOT — hand-roll `Combine` (see Design section) so `Some({})` is honored as a clear sentinel.\n\n```rust\n/// `[run.integrations]` — run-level integration knobs.\n#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize, fabro_macros::Combine)]\n#[serde(deny_unknown_fields)]\npub struct RunIntegrationsLayer {\n #[serde(default, skip_serializing_if = \"Option::is_none\")]\n pub github: Option,\n}\n\n/// `[run.integrations.github]` — runtime GitHub token shape.\n#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)]\n#[serde(deny_unknown_fields)]\npub struct RunIntegrationsGithubLayer {\n #[serde(default, skip_serializing_if = \"Option::is_none\")]\n pub permissions: Option>,\n}\n\nimpl Combine for RunIntegrationsGithubLayer {\n fn combine(self, other: Self) -> Self {\n Self { permissions: self.permissions.or(other.permissions) }\n }\n}\n```\n\nAdd `pub integrations: Option` to `RunLayer`.\n\n### 2. Config schema — server side\n\n`lib/crates/fabro-config/src/layers/server.rs:226`: remove `permissions: StickyMap` from `GithubIntegrationLayer`. Server struct keeps only identity/auth/webhook fields.\n\n### 3. Resolved types — `lib/crates/fabro-types/src/settings/run.rs`\n\nResolved types collapse the layer-time `Option` (presence is only meaningful during merge):\n\n```rust\npub struct RunIntegrationsSettings {\n pub github: RunIntegrationsGithubSettings,\n}\n\npub struct RunIntegrationsGithubSettings {\n pub permissions: HashMap, // empty = no token\n}\n```\n\nAdd `integrations: RunIntegrationsSettings` to `RunNamespace`. Drop `permissions` from the resolved `GithubIntegrationSettings` in fabro-types.\n\n### 4. Resolver — `lib/crates/fabro-config/src/resolve/run.rs`\n\nAdd `fn resolve_integrations(layer: Option<&RunIntegrationsLayer>) -> RunIntegrationsSettings`. Pass through `InterpString`s untouched — do NOT resolve env vars here. Collapse `Option>` → `HashMap<...>` (None and Some({}) both become empty).\n\n`lib/crates/fabro-config/src/resolve/server.rs:465`: drop the line that copies `permissions` into the resolved github settings.\n\n### 5. Server consumer — `lib/crates/fabro-server`\n\nFour read sites swap from `server_settings.server.integrations.github.permissions` to `run_spec.settings.run.integrations.github.permissions` (a flat `HashMap`; empty = no token):\n\n- `run_manifest.rs:488` — clone-credential gate inside `prepare_manifest`.\n- `run_manifest.rs:1184-1235` — `run_github_token_check`. Change signature to take resolved run permissions; resolve `InterpString`s inside the function for both minting and the report.\n- `server.rs:2727` — forced-credential gate in run launch path.\n- `server.rs:2763-2772` — InterpString → String resolution for `StartServices.github_permissions`. Read from run settings instead of server settings; same resolution logic.\n\n### 5b. Bundled-workflow TOML parsing — `lib/crates/fabro-server/src/run_manifest.rs:290-307`\n\n`root_workflow_run_layer` currently parses the workflow TOML as a raw `toml::Table`, lifts out `run`, and silently discards every other top-level key. That means stale `[server.integrations.github.permissions]` is not caught by `deny_unknown_fields`.\n\nNaive \"reject any non-`run` key\" would break valid workflow TOML (every workflow.toml in the repo has `_version = 1`; `hello/workflow.toml` has `[workflow]`). `SettingsLayer` (`layers/settings.rs:21-37`) is the schema for a settings file: `_version`, `project`, `workflow`, `run`, `cli`, `server`, `features`.\n\nFix: parse the source via `source.parse::()` and take `layer.run.unwrap_or_default()`. That gives:\n- valid top-level domains parse without error;\n- stale `[server.integrations.github.permissions]` fails because `permissions` is no longer a known field on `GithubIntegrationLayer` (section 2) and the layer has `serde(deny_unknown_fields)`;\n- shape-valid `[server.*]` in a workflow.toml is silently ignored downstream by `builders.rs:394`, preserving today's behavior (workflow.toml shouldn't set server config, but doesn't blow up either).\n\n`resolve_manifest_dockerfile(&mut run, &config.path, &workflow.files)` (line 305) still runs on the extracted `RunLayer`.\n\n### 6. CLI worker — `lib/crates/fabro-cli/src/commands/run/runner.rs` (P0, was missing)\n\nTwo reads on the CLI launch path that today still point at the server-side field:\n\n- `runner.rs:120` — `github_permissions: HashMap::new()` is hardcoded. Source from `run_spec.settings.run.integrations.github.permissions`, applying the same `InterpString` → `String` resolution as the server side.\n- `runner.rs:514-555` — `maybe_build_github_credentials`. Line 524 reads `settings.server.integrations.github.permissions.is_empty()` to decide whether credentials are required. Swap to read the run-level permissions. Identity fields (`strategy`, `app_id`, `slug`) at lines 527-538 stay on the server side.\n\nTo make this testable, factor two private helpers in `runner.rs`:\n- `fn resolve_run_github_permissions(run: &RunNamespace) -> HashMap` — InterpString resolution loop, same as the server side. Reuse server-side helper if one exists; otherwise extract a shared one in fabro-config or fabro-server.\n- `fn requires_github_credentials(run: &RunNamespace, server: Option<&ServerNamespace>) -> bool` — folds the existing clone/PR/permissions gates.\n\nBoth are pure functions of resolved settings; unit-test them in `#[cfg(test)] mod tests` inside `runner.rs`. Don't try to reach private items from an integration test in `tests/it/`.\n\nWithout this fix, CLI-launched runs silently get no `GITHUB_TOKEN` regardless of TOML.\n\nDownstream is unchanged: `StartServices.github_permissions` → `SandboxEnvSpec.github_permissions` (`fabro-workflow/src/operations/start.rs:99`, `pipeline/types.rs:225`) → `mint_github_token` + `GITHUB_TOKEN` injection at `pipeline/initialize.rs:240`.\n\n### 7. Parse hints — `lib/crates/fabro-config/src/parse.rs:117-118`\n\nUpdate the legacy-`[github]` migration hint to distinguish identity vs. permissions. Suggested wording:\n- `\"github\"` → `\"split into [server.integrations.github] (App identity/auth) and [run.integrations.github.permissions] (sandbox token scopes)\"`.\n\nAlso: ensure `[server.integrations.github]` with a `permissions` subkey produces a `deny_unknown_fields` error pointing at the new path. If automatic, no extra code; if not, add a targeted parse-time check.\n\n### 8. OpenAPI — `docs/public/api-reference/fabro-api.yaml`\n\nWire shape mirrors the resolved Rust types (no `Option`s, no nullability):\n\n- **Remove** `permissions` from `GithubIntegrationSettings` (line 7212): drop the property and remove from `required`.\n- **Add** new schemas:\n - `RunIntegrationsSettings`: `properties: { github: { $ref: \"#/components/schemas/RunIntegrationsGithubSettings\" } }`, `required: [\"github\"]`.\n - `RunIntegrationsGithubSettings`: `properties: { permissions: { type: object, additionalProperties: { type: string } } }`, `required: [\"permissions\"]`.\n- **Add** `integrations: { $ref: \"#/components/schemas/RunIntegrationsSettings\" }` to `RunNamespace.properties` and to `RunNamespace.required`.\n\nThe collapsed-Option resolved types (section 3) make this clean: `github` is always present, `permissions` is always an object (possibly empty). No `nullable`, no `oneOf`, no `skip_serializing_if` to debate.\n\nType ownership: progenitor auto-generates new run DTOs (Explore confirmed no `with_replacement` for run types today). No new `with_replacement` entries; do not split into hand-rolled DTOs unless a real semantic divergence emerges. Add a fabro-api JSON parity test asserting OpenAPI's `RunIntegrationsGithubSettings` round-trips through the resolved Rust type, including the empty-permissions case.\n\nRegenerate TS client: `cd lib/packages/fabro-api-client && bun run generate`.\n\n### 9. Repo TOMLs — rewrite\n\n- `.fabro/workflows/gh-triage/workflow.toml`\n- `.fabro/workflows/implement-issue/workflow.toml`\n- `.fabro/workflows/gh-list/workflow.toml`\n\nEach: `[server.integrations.github.permissions]` → `[run.integrations.github.permissions]`.\n\n### 10. User settings (advisory)\n\n`~/.fabro/settings.toml` should be rewritten to put a default at `[run.integrations.github.permissions]`. Don't auto-edit; cover in verification.\n\n### 11. Docs — `docs/public/integrations/github.mdx`\n\n- Line 35 (overview table): rewrite the row to reference `[run.integrations.github.permissions]`.\n- Lines 209-220 (\"GITHUB_TOKEN injection\"): rewrite. Show canonical run-level path. Note `~/.fabro/settings.toml` is the natural place for server defaults because it's the user layer of the same merge stack. Drop the `[github]` shorthand from line 211 (never existed in code).\n- Add the security-model note (boundary = installation grants, no Fabro-side cap).\n\n### 12. Tests\n\n**Layer parsing + merge** (`lib/crates/fabro-config/src/tests/`, parse real TOML at each layer; not just resolver-level fixtures):\n- Parse a workflow.toml with `[run.integrations.github.permissions]` — assert the layer round-trips.\n- Merge user `{ contents = \"read\" }` + workflow `{ issues = \"write\" }` — assert workflow fully replaces: result is `{ issues = \"write\" }`.\n- Merge user `{ contents = \"read\" }` + workflow absent (no `[run.integrations]` block) — assert inheritance: result is `{ contents = \"read\" }`.\n- Merge user `{ contents = \"read\" }` + workflow `permissions = {}` — assert clear: resolved permissions is empty.\n- Negative: `[server.integrations.github.permissions]` in user/project/workflow TOML must error via `deny_unknown_fields`.\n- Negative (bundled workflow path, P2): targeted test exercising `root_workflow_run_layer` (`run_manifest.rs:290`) with a workflow.toml containing a stale `[server.integrations.github.permissions]` block — assert it errors via `deny_unknown_fields` after the rewrite to parse through `SettingsLayer`. Pair with a positive test asserting `_version = 1` and a `[workflow]` block still parse cleanly through this code path.\n\n**Resolver** (`resolve/run.rs` tests): assert `InterpString` is preserved in resolved settings, not flattened to `String`. Assert resolved `permissions` is `HashMap` (Option collapsed).\n\n**Preflight** (`run_manifest.rs:1822` and surrounds):\n- Add a case where the run config sets `permissions = { issues = \"read\" }` and the GitHub App is configured — assert `run_github_token_check` reports `Pass`.\n- Update `server_settings_fixture` (`run_manifest.rs:1390`) usage in any test that previously set permissions through it; rewrite to set via run layer.\n\n**CLI worker path** (P0): unit-test the new private helpers in `runner.rs`'s `#[cfg(test)] mod tests`:\n- `resolve_run_github_permissions` — given a `RunNamespace` with `InterpString` permissions referencing env vars, returns the resolved `HashMap`.\n- `requires_github_credentials` — exercises each truth-table case (clone needed, PR enabled, permissions non-empty, all-absent).\n- Do not try to assert `StartServices.github_permissions` from an integration test — those internals are private. End-to-end behavior is covered by the smoke runs in Verification.\n\n## Verification\n\n1. `cargo build --workspace` clean.\n2. `cargo nextest run -p fabro-config` — layer/resolve tests pass, including the new override + replace-semantics + deny-unknown tests.\n3. `cargo nextest run -p fabro-server -p fabro-cli` — preflight + run-manifest + worker tests pass.\n4. `cargo nextest run -p fabro-api` — JSON parity test for the new run integrations schema passes.\n5. Smoke run via server (manual):\n - Update `~/.fabro/settings.toml` to put `[run.integrations.github.permissions]` defaults (`pull_requests = \"read\"`, `issues = \"read\"`).\n - Restart `fabro server`.\n - `fabro run gh-list --no-retro`; both stages exit 0 with PR/issue listings on stdout.\n - `fabro logs ` shows `gh pr list` returning data; no \"populate the GH_TOKEN\" error.\n6. Smoke run via CLI worker path (manual): same as above but with a `fabro run` invocation that uses the local-CLI worker path (not HTTP). Confirms P0 fix.\n7. Override test: in `gh-list/workflow.toml`, set `permissions = { issues = \"write\" }` over a server default of `read`. Confirm the minted-token preflight summary shows `issues: write` and not `read`.\n8. Tightening test: in another workflow, set `permissions = {}`. Confirm preflight reports no token requested and the sandbox env has no `GITHUB_TOKEN`.\n9. `cd apps/fabro-web && bun run typecheck && bun test` — generated TS client compiles against the new schema.\n\n## Open questions\n\n1. Anything else worth promoting to `[run.integrations.*]` now (Slack, Discord run-time config)? Recommendation: leave empty until a concrete need lands; don't speculate.\n2. Should preflight surface the *resolved* permission strings in its report, or the raw `InterpString` source? Recommendation: resolved, so reviewers see what the App will actually be asked for; treat unresolved env-var fallbacks as a preflight warning.\n3. `resolve_run_github_permissions` location — should the InterpString-resolution loop live in fabro-config (shared by server + CLI worker), or in each consumer? Recommendation: extract to fabro-config / fabro-types alongside the resolved type, so server (`server.rs:2763-2772`) and CLI (`runner.rs`) both call the same helper. Avoids two copies drifting.\n", + "thread.preflight_compile.current_node": "preflight_lint", + "thread.start.current_node": "toolchain", + "internal.run_id": "01KR25M0Q1VARG70MW2Y88MK0K" + }, + "node_outcomes": { + "preflight_compile": { + "status": "succeeded", + "context_updates": { + "command.stderr": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "command.output": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126" + }, + "notes": "Script completed: cargo check -q --workspace 2>&1", + "usage": null + }, + "preflight_lint": { + "status": "succeeded", + "context_updates": { + "command.stderr": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "command.output": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126" + }, + "notes": "Script completed: cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1", + "usage": null + }, + "start": { + "status": "succeeded", + "usage": null + }, + "toolchain": { + "status": "succeeded", + "context_updates": { + "command.output": "blob://sha256/fc14b2ba2d770e5cd3169df7a29525c962adfc4cfa3097b9098c63ebd61a748c", + "command.stderr": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126" + }, + "notes": "Script completed: 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", + "usage": null + } + }, + "next_node_id": "implement", + "git_commit_sha": "cce5bed7afbbe81f0b74b2de99b1f3e6383603bb", + "node_visits": { + "toolchain": 1, + "preflight_lint": 1, + "preflight_compile": 1, + "start": 1 + } + } ] ], "conclusion": null, @@ -840,7 +957,12 @@ "first_event_seq": 39, "prompt": null, "response": null, - "completion": null, + "completion": { + "outcome": "succeeded", + "notes": "Script completed: cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1", + "failure_reason": null, + "timestamp": "2026-05-07T21:36:06.735269Z" + }, "provider_used": null, "diff": null, "script_invocation": { @@ -848,11 +970,46 @@ "command": "cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1", "language": "shell" }, + "script_timing": { + "stdout": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "stderr": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "exit_code": 0, + "duration_ms": 133705, + "termination": "exited", + "stdout_bytes": 0, + "stderr_bytes": 0, + "streams_separated": true, + "live_streaming": false + }, + "parallel_results": null, + "stdout": null, + "stderr": null, + "stdout_bytes": 0, + "stderr_bytes": 0, + "streams_separated": true, + "live_streaming": false, + "termination": "exited", + "started_at": "2026-05-07T21:33:53.012397Z", + "duration_ms": 133721, + "state": "succeeded" + }, + "implement@1": { + "first_event_seq": 49, + "prompt": null, + "response": null, + "completion": null, + "provider_used": { + "mode": "agent", + "provider": "anthropic", + "model": "claude-opus-4-7" + }, + "diff": null, + "script_invocation": null, "script_timing": null, "parallel_results": null, "stdout": null, "stderr": null, - "started_at": "2026-05-07T21:33:53.012397Z", + "started_at": "2026-05-07T21:36:17.491489Z", "state": "running" }, "start@1": { diff --git a/stages/004-preflight_lint@1/script_timing.json b/stages/004-preflight_lint@1/script_timing.json new file mode 100644 index 000000000..10f12d935 --- /dev/null +++ b/stages/004-preflight_lint@1/script_timing.json @@ -0,0 +1,11 @@ +{ + "stdout": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "stderr": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "exit_code": 0, + "duration_ms": 133705, + "termination": "exited", + "stdout_bytes": 0, + "stderr_bytes": 0, + "streams_separated": true, + "live_streaming": false +} \ No newline at end of file diff --git a/stages/004-preflight_lint@1/status.json b/stages/004-preflight_lint@1/status.json new file mode 100644 index 000000000..9dc2f554a --- /dev/null +++ b/stages/004-preflight_lint@1/status.json @@ -0,0 +1,6 @@ +{ + "outcome": "succeeded", + "notes": "Script completed: cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1", + "failure_reason": null, + "timestamp": "2026-05-07T21:36:06.735269Z" +} \ No newline at end of file diff --git a/stages/004-preflight_lint@1/stderr.log b/stages/004-preflight_lint@1/stderr.log new file mode 100644 index 000000000..d87ba9545 --- /dev/null +++ b/stages/004-preflight_lint@1/stderr.log @@ -0,0 +1 @@ +blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126 \ No newline at end of file diff --git a/stages/004-preflight_lint@1/stdout.log b/stages/004-preflight_lint@1/stdout.log new file mode 100644 index 000000000..d87ba9545 --- /dev/null +++ b/stages/004-preflight_lint@1/stdout.log @@ -0,0 +1 @@ +blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126 \ No newline at end of file diff --git a/stages/005-implement@1/prompt.md b/stages/005-implement@1/prompt.md new file mode 100644 index 000000000..6f8a78f71 --- /dev/null +++ b/stages/005-implement@1/prompt.md @@ -0,0 +1,244 @@ +Goal: # Move GitHub token permissions from `[server.integrations.github]` to `[run.integrations.github]` + +## Context + +Today `[server.integrations.github.permissions]` controls what scopes Fabro requests on the Installation Access Token it injects into the sandbox as `GITHUB_TOKEN`. This is conceptually wrong: + +- Token permissions describe what *this run* is authorized to do, not the server's identity. +- The current key is server-only. `workflow.toml` and `project.toml` cannot override it (`builders.rs:394` strips `server.*` from per-workflow layers), so projects/workflows can't tighten or relax permissions. +- The public docs (`integrations/github.mdx:211-220`) already describe a per-run config, which never existed in code. The intent has always been run-level. + +Greenfield app — no migration, no backwards compat. Moving permissions to `[run.integrations.github.permissions]` makes the natural layer-merge (workflow > project > user > defaults) Just Work: server admins set defaults in `~/.fabro/settings.toml`, projects and workflows override. + +## Security model + +Run config is trusted policy input for sandbox token scopes. The upper bound on what Fabro can mint is the GitHub App installation's granted permissions; Fabro does **not** impose a separate server-side cap. Operators must not run untrusted workflow/project/user TOML against a broadly-scoped App installation. Preflight prints requested permissions so reviewers can see them; no enforcement layer beyond GitHub's own. + +## Design + +New TOML path: `[run.integrations.github.permissions]`. Creates a fresh `[run.integrations]` namespace for future run-level integration knobs. + +**Merge semantics** (presence-aware, hand-rolled). `ReplaceMap` does NOT work here: `maps.rs:76-80` is `if self.0.is_empty() { other } else { self }`, i.e. an empty higher layer falls back to the inherited map. We need empty-wins-as-clear, so we don't reuse `ReplaceMap` (and don't change global semantics for other users). + +Layer field: `pub permissions: Option>`. + +| Higher layer | Lower layer | Result | +|---|---|---| +| `None` | anything | lower (inherit) | +| `Some(map)` | anything | `Some(map)` (full replace, including `Some({})` = clear) | + +Hand-roll `Combine` on `RunIntegrationsGithubLayer`: +```rust +impl Combine for RunIntegrationsGithubLayer { + fn combine(self, other: Self) -> Self { + Self { permissions: self.permissions.or(other.permissions) } + } +} +``` +Don't derive — the blanket `Option` impl recurses into the inner type and would reintroduce the empty-fallback bug. + +Resolved type collapses the Option: `pub permissions: HashMap` where empty = no token requested. The presence distinction only matters during merge. + +**Interpolation**: keep `InterpString` in the resolved type. Resolve to `String` at the start-services construction boundary, matching the existing pattern at `server.rs:2763-2772`. No early resolution in `resolve_run`. + +## Changes + +### 1. Config schema — `lib/crates/fabro-config/src/layers/run.rs` + +Add new layer types. `RunIntegrationsLayer` derives `Combine` normally; `RunIntegrationsGithubLayer` does NOT — hand-roll `Combine` (see Design section) so `Some({})` is honored as a clear sentinel. + +```rust +/// `[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. +#[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) } + } +} +``` + +Add `pub integrations: Option` to `RunLayer`. + +### 2. Config schema — server side + +`lib/crates/fabro-config/src/layers/server.rs:226`: remove `permissions: StickyMap` from `GithubIntegrationLayer`. Server struct keeps only identity/auth/webhook fields. + +### 3. Resolved types — `lib/crates/fabro-types/src/settings/run.rs` + +Resolved types collapse the layer-time `Option` (presence is only meaningful during merge): + +```rust +pub struct RunIntegrationsSettings { + pub github: RunIntegrationsGithubSettings, +} + +pub struct RunIntegrationsGithubSettings { + pub permissions: HashMap, // empty = no token +} +``` + +Add `integrations: RunIntegrationsSettings` to `RunNamespace`. Drop `permissions` from the resolved `GithubIntegrationSettings` in fabro-types. + +### 4. Resolver — `lib/crates/fabro-config/src/resolve/run.rs` + +Add `fn resolve_integrations(layer: Option<&RunIntegrationsLayer>) -> RunIntegrationsSettings`. Pass through `InterpString`s untouched — do NOT resolve env vars here. Collapse `Option>` → `HashMap<...>` (None and Some({}) both become empty). + +`lib/crates/fabro-config/src/resolve/server.rs:465`: drop the line that copies `permissions` into the resolved github settings. + +### 5. Server consumer — `lib/crates/fabro-server` + +Four read sites swap from `server_settings.server.integrations.github.permissions` to `run_spec.settings.run.integrations.github.permissions` (a flat `HashMap`; empty = no token): + +- `run_manifest.rs:488` — clone-credential gate inside `prepare_manifest`. +- `run_manifest.rs:1184-1235` — `run_github_token_check`. Change signature to take resolved run permissions; resolve `InterpString`s inside the function for both minting and the report. +- `server.rs:2727` — forced-credential gate in run launch path. +- `server.rs:2763-2772` — InterpString → String resolution for `StartServices.github_permissions`. Read from run settings instead of server settings; same resolution logic. + +### 5b. Bundled-workflow TOML parsing — `lib/crates/fabro-server/src/run_manifest.rs:290-307` + +`root_workflow_run_layer` currently parses the workflow TOML as a raw `toml::Table`, lifts out `run`, and silently discards every other top-level key. That means stale `[server.integrations.github.permissions]` is not caught by `deny_unknown_fields`. + +Naive "reject any non-`run` key" would break valid workflow TOML (every workflow.toml in the repo has `_version = 1`; `hello/workflow.toml` has `[workflow]`). `SettingsLayer` (`layers/settings.rs:21-37`) is the schema for a settings file: `_version`, `project`, `workflow`, `run`, `cli`, `server`, `features`. + +Fix: parse the source via `source.parse::()` and take `layer.run.unwrap_or_default()`. That gives: +- valid top-level domains parse without error; +- stale `[server.integrations.github.permissions]` fails because `permissions` is no longer a known field on `GithubIntegrationLayer` (section 2) and the layer has `serde(deny_unknown_fields)`; +- shape-valid `[server.*]` in a workflow.toml is silently ignored downstream by `builders.rs:394`, preserving today's behavior (workflow.toml shouldn't set server config, but doesn't blow up either). + +`resolve_manifest_dockerfile(&mut run, &config.path, &workflow.files)` (line 305) still runs on the extracted `RunLayer`. + +### 6. CLI worker — `lib/crates/fabro-cli/src/commands/run/runner.rs` (P0, was missing) + +Two reads on the CLI launch path that today still point at the server-side field: + +- `runner.rs:120` — `github_permissions: HashMap::new()` is hardcoded. Source from `run_spec.settings.run.integrations.github.permissions`, applying the same `InterpString` → `String` resolution as the server side. +- `runner.rs:514-555` — `maybe_build_github_credentials`. Line 524 reads `settings.server.integrations.github.permissions.is_empty()` to decide whether credentials are required. Swap to read the run-level permissions. Identity fields (`strategy`, `app_id`, `slug`) at lines 527-538 stay on the server side. + +To make this testable, factor two private helpers in `runner.rs`: +- `fn resolve_run_github_permissions(run: &RunNamespace) -> HashMap` — InterpString resolution loop, same as the server side. Reuse server-side helper if one exists; otherwise extract a shared one in fabro-config or fabro-server. +- `fn requires_github_credentials(run: &RunNamespace, server: Option<&ServerNamespace>) -> bool` — folds the existing clone/PR/permissions gates. + +Both are pure functions of resolved settings; unit-test them in `#[cfg(test)] mod tests` inside `runner.rs`. Don't try to reach private items from an integration test in `tests/it/`. + +Without this fix, CLI-launched runs silently get no `GITHUB_TOKEN` regardless of TOML. + +Downstream is unchanged: `StartServices.github_permissions` → `SandboxEnvSpec.github_permissions` (`fabro-workflow/src/operations/start.rs:99`, `pipeline/types.rs:225`) → `mint_github_token` + `GITHUB_TOKEN` injection at `pipeline/initialize.rs:240`. + +### 7. Parse hints — `lib/crates/fabro-config/src/parse.rs:117-118` + +Update the legacy-`[github]` migration hint to distinguish identity vs. permissions. Suggested wording: +- `"github"` → `"split into [server.integrations.github] (App identity/auth) and [run.integrations.github.permissions] (sandbox token scopes)"`. + +Also: ensure `[server.integrations.github]` with a `permissions` subkey produces a `deny_unknown_fields` error pointing at the new path. If automatic, no extra code; if not, add a targeted parse-time check. + +### 8. OpenAPI — `docs/public/api-reference/fabro-api.yaml` + +Wire shape mirrors the resolved Rust types (no `Option`s, no nullability): + +- **Remove** `permissions` from `GithubIntegrationSettings` (line 7212): drop the property and remove from `required`. +- **Add** new schemas: + - `RunIntegrationsSettings`: `properties: { github: { $ref: "#/components/schemas/RunIntegrationsGithubSettings" } }`, `required: ["github"]`. + - `RunIntegrationsGithubSettings`: `properties: { permissions: { type: object, additionalProperties: { type: string } } }`, `required: ["permissions"]`. +- **Add** `integrations: { $ref: "#/components/schemas/RunIntegrationsSettings" }` to `RunNamespace.properties` and to `RunNamespace.required`. + +The collapsed-Option resolved types (section 3) make this clean: `github` is always present, `permissions` is always an object (possibly empty). No `nullable`, no `oneOf`, no `skip_serializing_if` to debate. + +Type ownership: progenitor auto-generates new run DTOs (Explore confirmed no `with_replacement` for run types today). No new `with_replacement` entries; do not split into hand-rolled DTOs unless a real semantic divergence emerges. Add a fabro-api JSON parity test asserting OpenAPI's `RunIntegrationsGithubSettings` round-trips through the resolved Rust type, including the empty-permissions case. + +Regenerate TS client: `cd lib/packages/fabro-api-client && bun run generate`. + +### 9. Repo TOMLs — rewrite + +- `.fabro/workflows/gh-triage/workflow.toml` +- `.fabro/workflows/implement-issue/workflow.toml` +- `.fabro/workflows/gh-list/workflow.toml` + +Each: `[server.integrations.github.permissions]` → `[run.integrations.github.permissions]`. + +### 10. User settings (advisory) + +`~/.fabro/settings.toml` should be rewritten to put a default at `[run.integrations.github.permissions]`. Don't auto-edit; cover in verification. + +### 11. Docs — `docs/public/integrations/github.mdx` + +- Line 35 (overview table): rewrite the row to reference `[run.integrations.github.permissions]`. +- Lines 209-220 ("GITHUB_TOKEN injection"): rewrite. Show canonical run-level path. Note `~/.fabro/settings.toml` is the natural place for server defaults because it's the user layer of the same merge stack. Drop the `[github]` shorthand from line 211 (never existed in code). +- Add the security-model note (boundary = installation grants, no Fabro-side cap). + +### 12. Tests + +**Layer parsing + merge** (`lib/crates/fabro-config/src/tests/`, parse real TOML at each layer; not just resolver-level fixtures): +- Parse a workflow.toml with `[run.integrations.github.permissions]` — assert the layer round-trips. +- Merge user `{ contents = "read" }` + workflow `{ issues = "write" }` — assert workflow fully replaces: result is `{ issues = "write" }`. +- Merge user `{ contents = "read" }` + workflow absent (no `[run.integrations]` block) — assert inheritance: result is `{ contents = "read" }`. +- Merge user `{ contents = "read" }` + workflow `permissions = {}` — assert clear: resolved permissions is empty. +- Negative: `[server.integrations.github.permissions]` in user/project/workflow TOML must error via `deny_unknown_fields`. +- Negative (bundled workflow path, P2): targeted test exercising `root_workflow_run_layer` (`run_manifest.rs:290`) with a workflow.toml containing a stale `[server.integrations.github.permissions]` block — assert it errors via `deny_unknown_fields` after the rewrite to parse through `SettingsLayer`. Pair with a positive test asserting `_version = 1` and a `[workflow]` block still parse cleanly through this code path. + +**Resolver** (`resolve/run.rs` tests): assert `InterpString` is preserved in resolved settings, not flattened to `String`. Assert resolved `permissions` is `HashMap` (Option collapsed). + +**Preflight** (`run_manifest.rs:1822` and surrounds): +- Add a case where the run config sets `permissions = { issues = "read" }` and the GitHub App is configured — assert `run_github_token_check` reports `Pass`. +- Update `server_settings_fixture` (`run_manifest.rs:1390`) usage in any test that previously set permissions through it; rewrite to set via run layer. + +**CLI worker path** (P0): unit-test the new private helpers in `runner.rs`'s `#[cfg(test)] mod tests`: +- `resolve_run_github_permissions` — given a `RunNamespace` with `InterpString` permissions referencing env vars, returns the resolved `HashMap`. +- `requires_github_credentials` — exercises each truth-table case (clone needed, PR enabled, permissions non-empty, all-absent). +- Do not try to assert `StartServices.github_permissions` from an integration test — those internals are private. End-to-end behavior is covered by the smoke runs in Verification. + +## Verification + +1. `cargo build --workspace` clean. +2. `cargo nextest run -p fabro-config` — layer/resolve tests pass, including the new override + replace-semantics + deny-unknown tests. +3. `cargo nextest run -p fabro-server -p fabro-cli` — preflight + run-manifest + worker tests pass. +4. `cargo nextest run -p fabro-api` — JSON parity test for the new run integrations schema passes. +5. Smoke run via server (manual): + - Update `~/.fabro/settings.toml` to put `[run.integrations.github.permissions]` defaults (`pull_requests = "read"`, `issues = "read"`). + - Restart `fabro server`. + - `fabro run gh-list --no-retro`; both stages exit 0 with PR/issue listings on stdout. + - `fabro logs ` shows `gh pr list` returning data; no "populate the GH_TOKEN" error. +6. Smoke run via CLI worker path (manual): same as above but with a `fabro run` invocation that uses the local-CLI worker path (not HTTP). Confirms P0 fix. +7. Override test: in `gh-list/workflow.toml`, set `permissions = { issues = "write" }` over a server default of `read`. Confirm the minted-token preflight summary shows `issues: write` and not `read`. +8. Tightening test: in another workflow, set `permissions = {}`. Confirm preflight reports no token requested and the sandbox env has no `GITHUB_TOKEN`. +9. `cd apps/fabro-web && bun run typecheck && bun test` — generated TS client compiles against the new schema. + +## Open questions + +1. Anything else worth promoting to `[run.integrations.*]` now (Slack, Discord run-time config)? Recommendation: leave empty until a concrete need lands; don't speculate. +2. Should preflight surface the *resolved* permission strings in its report, or the raw `InterpString` source? Recommendation: resolved, so reviewers see what the App will actually be asked for; treat unresolved env-var fallbacks as a preflight warning. +3. `resolve_run_github_permissions` location — should the InterpString-resolution loop live in fabro-config (shared by server + CLI worker), or in each consumer? Recommendation: extract to fabro-config / fabro-types alongside the resolved type, so server (`server.rs:2763-2772`) and CLI (`runner.rs`) both call the same helper. Avoids two copies drifting. + + +## Completed stages +- **toolchain**: succeeded + - 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` + - Stdout: + ``` + cargo 1.95.0 (f2d3ce0bd 2026-03-21) + ``` + - Stderr: (empty) +- **preflight_compile**: succeeded + - Script: `cargo check -q --workspace 2>&1` + - Stdout: (empty) + - Stderr: (empty) +- **preflight_lint**: succeeded + - Script: `cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1` + - Stdout: (empty) + - Stderr: (empty) + + +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. \ No newline at end of file diff --git a/stages/005-implement@1/provider_used.json b/stages/005-implement@1/provider_used.json new file mode 100644 index 000000000..672cc50e5 --- /dev/null +++ b/stages/005-implement@1/provider_used.json @@ -0,0 +1,5 @@ +{ + "mode": "agent", + "provider": "anthropic", + "model": "claude-opus-4-7" +} \ No newline at end of file diff --git a/stages/005-implement@1/response.md b/stages/005-implement@1/response.md new file mode 100644 index 000000000..880ff2a64 --- /dev/null +++ b/stages/005-implement@1/response.md @@ -0,0 +1,32 @@ +TypeScript client is up to date. + +Looking at the goal one more time — most of the implementation is already done in this branch (verified by working build, lint, and full nextest run). All work described in sections 1-12 is in place: + +**Summary of implementation status verified:** + +1. ✅ **Layer schema (`run.rs`)**: `RunIntegrationsLayer` and `RunIntegrationsGithubLayer` with hand-rolled `Combine` honoring `Some({})` as clear sentinel. +2. ✅ **Server schema (`server.rs`)**: `permissions` removed from `GithubIntegrationLayer`. +3. ✅ **Resolved types (`fabro-types/.../run.rs`)**: `RunIntegrationsSettings` / `RunIntegrationsGithubSettings` with collapsed-Option `permissions`, plus `is_token_requested()` and `resolve_permissions()` helpers (with built-in unit tests). +4. ✅ **Resolver (`resolve/run.rs`)**: `resolve_integrations` collapses `Option` → `HashMap`. Server resolver no longer emits `permissions`. +5. ✅ **Server consumers**: `run_manifest.rs` (clone-credential gate, `run_github_token_check`), `server.rs:2862` (forced-credential gate) and `server.rs:2898` (StartServices construction) all read run-level settings. +5b. ✅ **Bundled-workflow TOML**: `root_workflow_run_layer` parses via `parse_run_layer_from_settings_toml` (which goes through `SettingsLayer`), so stale `[server.integrations.github.permissions]` is rejected by `deny_unknown_fields`. +6. ✅ **CLI worker**: `runner.rs` reads `run_spec.settings.run.integrations.github.resolve_permissions(...)`; `requires_github_credentials` private helper plus truth-table tests. +7. ✅ **Parse hint (`parse.rs:118-121`)**: `[github]` legacy hint split into identity vs. permissions. +8. ✅ **OpenAPI**: `RunIntegrationsSettings` / `RunIntegrationsGithubSettings` schemas added; `RunNamespace.integrations` required; server `GithubIntegrationSettings.permissions` removed. JSON parity test (`fabro-api/tests/run_integrations_round_trip.rs`) covers populated and empty cases. +9. ✅ **Repo TOMLs**: `gh-list`, `gh-triage`, `implement-issue` workflow TOMLs use `[run.integrations.github.permissions]`. +11. ✅ **Docs**: `docs/public/integrations/github.mdx`, `docs/public/execution/run-configuration.mdx`, `docs/public/reference/user-configuration.mdx` updated. +12. ✅ **Tests**: + - Layer parsing + merge tests in `fabro-config/src/tests/resolve_run.rs:84-` (presence, replace, inherit, clear, deny_unknown). + - Resolver tests preserve `InterpString`. + - Preflight tests (`run_manifest.rs:1807`) for run-level permissions. + - Bundled-workflow positive + negative tests (`run_manifest.rs:2044, 2068, 2094`). + - CLI worker truth-table tests at `runner.rs:993-1054`. + - JSON parity test in fabro-api. + +**Verification:** +- `cargo build --workspace`: clean +- `cargo nextest run --workspace`: 5258 passed, 182 skipped (e2e) +- `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings`: clean +- `bun run typecheck` in `apps/fabro-web`: clean + +The implementation described in the plan is already complete on this branch — every step (1–12) has corresponding code/test changes in the tree, and the full test suite plus lint pass without modification. No further code changes are needed. \ No newline at end of file