mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-10 03:30:59 +00:00
parent
23200ac76a
commit
9a9795285a
6 changed files with 1185 additions and 27 deletions
575
run.json
575
run.json
File diff suppressed because one or more lines are too long
269
stages/006-simplify_opus@1/diff.patch
Normal file
269
stages/006-simplify_opus@1/diff.patch
Normal file
|
|
@ -0,0 +1,269 @@
|
|||
diff --git a/Cargo.lock b/Cargo.lock
|
||||
index 7e8f48e4c..d73ff2ac9 100644
|
||||
--- a/Cargo.lock
|
||||
+++ b/Cargo.lock
|
||||
@@ -2508,6 +2508,7 @@ dependencies = [
|
||||
"clap",
|
||||
"dirs",
|
||||
"fabro-model",
|
||||
+ "fabro-types",
|
||||
"fabro-util",
|
||||
"hex",
|
||||
"ipnet",
|
||||
diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs
|
||||
index 1760d76cf..71fc43b35 100644
|
||||
--- a/lib/crates/fabro-server/src/run_manifest.rs
|
||||
+++ b/lib/crates/fabro-server/src/run_manifest.rs
|
||||
@@ -26,13 +26,11 @@ use fabro_static::EnvVars;
|
||||
use fabro_types::settings::cli::OutputVerbosity;
|
||||
use fabro_types::settings::interp::InterpString;
|
||||
use fabro_types::settings::run::{EnvironmentProvider, RunGoal, RunNamespace};
|
||||
-use fabro_types::{
|
||||
- ManifestPath, RunId, RunProvenance, SandboxProviderKind, ServerSettings, WorkflowSettings,
|
||||
-};
|
||||
+use fabro_types::{ManifestPath, RunId, SandboxProviderKind, ServerSettings, WorkflowSettings};
|
||||
use fabro_util::check_report::{CheckDetail, CheckReport, CheckResult, CheckSection, CheckStatus};
|
||||
use fabro_validate::Severity;
|
||||
use fabro_workflow::Error as WorkflowError;
|
||||
-use fabro_workflow::operations::{CreateRunInput, ValidateInput, WorkflowInput, validate};
|
||||
+use fabro_workflow::operations::{ValidateInput, WorkflowInput, validate};
|
||||
use fabro_workflow::pipeline::Validated;
|
||||
use fabro_workflow::run_materialization::materialize_run;
|
||||
use fabro_workflow::workflow_bundle::{BundledWorkflow, ParsedWorkflowConfig, WorkflowBundle};
|
||||
@@ -200,31 +198,6 @@ pub(crate) fn validate_prepared_manifest(
|
||||
})
|
||||
}
|
||||
|
||||
-pub(crate) fn create_run_input(
|
||||
- prepared: PreparedManifest,
|
||||
- configured_providers: Vec<ProviderId>,
|
||||
- provenance: RunProvenance,
|
||||
- web_url: Option<String>,
|
||||
-) -> CreateRunInput {
|
||||
- CreateRunInput {
|
||||
- workflow: WorkflowInput::Bundled(prepared.workflow_input),
|
||||
- settings: prepared.settings,
|
||||
- cwd: prepared.cwd,
|
||||
- workflow_slug: None,
|
||||
- workflow_path: Some(prepared.target_path),
|
||||
- workflow_bundle: Some(prepared.workflow_bundle),
|
||||
- submitted_manifest_bytes: None,
|
||||
- run_id: prepared.run_id,
|
||||
- title: prepared.title,
|
||||
- git: prepared.git,
|
||||
- fork_source_ref: None,
|
||||
- parent_id: prepared.parent_id,
|
||||
- provenance,
|
||||
- configured_providers,
|
||||
- web_url,
|
||||
- }
|
||||
-}
|
||||
-
|
||||
pub(crate) async fn run_preflight(
|
||||
state: &AppState,
|
||||
prepared: &PreparedManifest,
|
||||
diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs
|
||||
index 25316c66a..27fd06072 100644
|
||||
--- a/lib/crates/fabro-server/src/server.rs
|
||||
+++ b/lib/crates/fabro-server/src/server.rs
|
||||
@@ -1825,7 +1825,8 @@ async fn http_log_middleware(mut req: axum_extract::Request, next: Next) -> Resp
|
||||
team_id = team_id.as_str(),
|
||||
user_id = user_id.as_str(),
|
||||
),
|
||||
- None | Some(Principal::Agent { .. } | Principal::System { .. }) => {
|
||||
+ None => emit_http_log!($level),
|
||||
+ Some(Principal::Agent { .. } | Principal::System { .. }) => {
|
||||
emit_http_log!($level)
|
||||
}
|
||||
}
|
||||
diff --git a/lib/crates/fabro-server/src/server/handler/runs.rs b/lib/crates/fabro-server/src/server/handler/runs.rs
|
||||
index e581c8048..5c6997d98 100644
|
||||
--- a/lib/crates/fabro-server/src/server/handler/runs.rs
|
||||
+++ b/lib/crates/fabro-server/src/server/handler/runs.rs
|
||||
@@ -642,14 +642,23 @@ async fn create_run(
|
||||
.map(LlmClientResult::provider_ids)
|
||||
.unwrap_or_default();
|
||||
let provenance = run_provenance(&headers, &actor);
|
||||
- let mut create_input = run_manifest::create_run_input(
|
||||
- prepared.clone(),
|
||||
- ready_provider_ids.clone(),
|
||||
+ let create_input = operations::CreateRunInput {
|
||||
+ workflow: operations::WorkflowInput::Bundled(prepared.workflow_input.clone()),
|
||||
+ settings: prepared.settings.clone(),
|
||||
+ cwd: prepared.cwd.clone(),
|
||||
+ workflow_slug: None,
|
||||
+ workflow_path: Some(prepared.target_path.clone()),
|
||||
+ workflow_bundle: Some(prepared.workflow_bundle.clone()),
|
||||
+ submitted_manifest_bytes: Some(body.to_vec()),
|
||||
+ run_id: Some(run_id),
|
||||
+ title: prepared.title.clone(),
|
||||
+ git: prepared.git.clone(),
|
||||
+ fork_source_ref: None,
|
||||
+ parent_id: prepared.parent_id,
|
||||
provenance,
|
||||
- web_url.clone(),
|
||||
- );
|
||||
- create_input.run_id = Some(run_id);
|
||||
- create_input.submitted_manifest_bytes = Some(body.to_vec());
|
||||
+ configured_providers: ready_provider_ids.clone(),
|
||||
+ web_url: web_url.clone(),
|
||||
+ };
|
||||
|
||||
let storage_root = match resolve_interp_string(&state.server_settings().server.storage.root) {
|
||||
Ok(path) => PathBuf::from(path),
|
||||
diff --git a/lib/crates/fabro-types/Cargo.toml b/lib/crates/fabro-types/Cargo.toml
|
||||
index c0a6dfc1b..86cf14331 100644
|
||||
--- a/lib/crates/fabro-types/Cargo.toml
|
||||
+++ b/lib/crates/fabro-types/Cargo.toml
|
||||
@@ -34,4 +34,5 @@ ulid.workspace = true
|
||||
url.workspace = true
|
||||
|
||||
[dev-dependencies]
|
||||
+fabro-types = { path = ".", features = ["test-support"] }
|
||||
tempfile = "3"
|
||||
diff --git a/lib/crates/fabro-types/src/test_support.rs b/lib/crates/fabro-types/src/test_support.rs
|
||||
index 9db7eff91..2f2a2e147 100644
|
||||
--- a/lib/crates/fabro-types/src/test_support.rs
|
||||
+++ b/lib/crates/fabro-types/src/test_support.rs
|
||||
@@ -1,4 +1,6 @@
|
||||
-use crate::{AuthMethod, IdpIdentity, Principal, RunProvenance, RunServerProvenance};
|
||||
+use crate::{
|
||||
+ AuthMethod, IdpIdentity, Principal, RunProvenance, RunServerProvenance, SystemActorKind,
|
||||
+};
|
||||
|
||||
#[must_use]
|
||||
pub fn test_principal() -> Principal {
|
||||
@@ -19,3 +21,16 @@ pub fn test_run_provenance() -> RunProvenance {
|
||||
subject: test_principal(),
|
||||
}
|
||||
}
|
||||
+
|
||||
+/// Provenance attributed to the engine itself, with no server/client metadata.
|
||||
+/// Used in tests that exercise system-initiated runs and serde round-trips.
|
||||
+#[must_use]
|
||||
+pub fn engine_run_provenance() -> RunProvenance {
|
||||
+ RunProvenance {
|
||||
+ server: None,
|
||||
+ client: None,
|
||||
+ subject: Principal::System {
|
||||
+ system_kind: SystemActorKind::Engine,
|
||||
+ },
|
||||
+ }
|
||||
+}
|
||||
diff --git a/lib/crates/fabro-types/tests/run_event_serde.rs b/lib/crates/fabro-types/tests/run_event_serde.rs
|
||||
index 533d21f15..be824fa8c 100644
|
||||
--- a/lib/crates/fabro-types/tests/run_event_serde.rs
|
||||
+++ b/lib/crates/fabro-types/tests/run_event_serde.rs
|
||||
@@ -6,18 +6,9 @@ use fabro_types::run_event::run::{RunCreatedProps, RunParentLinkedProps, RunPare
|
||||
use fabro_types::run_event::{RunSessionTurnFailedCode, RunSessionTurnFailedProps};
|
||||
use fabro_types::settings::InterpString;
|
||||
use fabro_types::settings::run::RunGoal;
|
||||
+use fabro_types::test_support::engine_run_provenance;
|
||||
use fabro_types::{EventBody, TurnId, WorkflowSettings, fixtures};
|
||||
|
||||
-fn test_run_provenance() -> fabro_types::RunProvenance {
|
||||
- fabro_types::RunProvenance {
|
||||
- server: None,
|
||||
- client: None,
|
||||
- subject: fabro_types::Principal::System {
|
||||
- system_kind: fabro_types::SystemActorKind::Engine,
|
||||
- },
|
||||
- }
|
||||
-}
|
||||
-
|
||||
fn templated_settings() -> WorkflowSettings {
|
||||
let mut settings = WorkflowSettings::default();
|
||||
settings.run.goal = Some(RunGoal::Inline(InterpString::parse("Ship {{ env.TASK }}")));
|
||||
@@ -37,7 +28,7 @@ fn run_created_props_round_trip_templated_settings() {
|
||||
source_directory: Some("/Users/client/project".to_string()),
|
||||
workflow_slug: Some("demo".to_string()),
|
||||
db_prefix: Some("run_".to_string()),
|
||||
- provenance: test_run_provenance(),
|
||||
+ provenance: engine_run_provenance(),
|
||||
manifest_blob: None,
|
||||
git: Some(GitContext {
|
||||
origin_url: "https://github.com/fabro-sh/fabro.git".to_string(),
|
||||
@@ -99,7 +90,7 @@ fn run_created_props_omits_web_url_when_absent() {
|
||||
source_directory: None,
|
||||
workflow_slug: None,
|
||||
db_prefix: None,
|
||||
- provenance: test_run_provenance(),
|
||||
+ provenance: engine_run_provenance(),
|
||||
manifest_blob: None,
|
||||
git: None,
|
||||
fork_source_ref: None,
|
||||
@@ -137,7 +128,7 @@ fn run_created_props_defaults_retried_from_when_absent() {
|
||||
"graph": Graph::new("ship"),
|
||||
"labels": {},
|
||||
"run_dir": "/tmp/run",
|
||||
- "provenance": test_run_provenance()
|
||||
+ "provenance": engine_run_provenance()
|
||||
});
|
||||
|
||||
let props: RunCreatedProps = serde_json::from_value(json).expect("props should deserialize");
|
||||
diff --git a/lib/crates/fabro-types/tests/run_spec_methods.rs b/lib/crates/fabro-types/tests/run_spec_methods.rs
|
||||
index ef29ce763..5004d87d7 100644
|
||||
--- a/lib/crates/fabro-types/tests/run_spec_methods.rs
|
||||
+++ b/lib/crates/fabro-types/tests/run_spec_methods.rs
|
||||
@@ -3,18 +3,9 @@ use std::collections::HashMap;
|
||||
use fabro_types::graph::Graph;
|
||||
use fabro_types::run::{DirtyStatus, GitContext, PreRunPushOutcome, RunSpec};
|
||||
use fabro_types::settings::{ProjectNamespace, WorkflowNamespace};
|
||||
+use fabro_types::test_support::engine_run_provenance;
|
||||
use fabro_types::{WorkflowSettings, fixtures};
|
||||
|
||||
-fn test_run_provenance() -> fabro_types::RunProvenance {
|
||||
- fabro_types::RunProvenance {
|
||||
- server: None,
|
||||
- client: None,
|
||||
- subject: fabro_types::Principal::System {
|
||||
- system_kind: fabro_types::SystemActorKind::Engine,
|
||||
- },
|
||||
- }
|
||||
-}
|
||||
-
|
||||
fn sample_run_spec() -> RunSpec {
|
||||
let settings = WorkflowSettings {
|
||||
project: ProjectNamespace {
|
||||
@@ -36,7 +27,7 @@ fn sample_run_spec() -> RunSpec {
|
||||
workflow_slug: Some("demo".to_string()),
|
||||
source_directory: Some("/Users/client/project".to_string()),
|
||||
labels: HashMap::from([("team".to_string(), "platform".to_string())]),
|
||||
- provenance: test_run_provenance(),
|
||||
+ provenance: engine_run_provenance(),
|
||||
manifest_blob: None,
|
||||
definition_blob: None,
|
||||
git: Some(GitContext {
|
||||
diff --git a/lib/crates/fabro-types/tests/run_spec_serde.rs b/lib/crates/fabro-types/tests/run_spec_serde.rs
|
||||
index d45c56ac8..ebe556f3d 100644
|
||||
--- a/lib/crates/fabro-types/tests/run_spec_serde.rs
|
||||
+++ b/lib/crates/fabro-types/tests/run_spec_serde.rs
|
||||
@@ -4,18 +4,9 @@ use fabro_types::graph::Graph;
|
||||
use fabro_types::run::{DirtyStatus, ForkSourceRef, GitContext, PreRunPushOutcome, RunSpec};
|
||||
use fabro_types::settings::InterpString;
|
||||
use fabro_types::settings::run::RunGoal;
|
||||
+use fabro_types::test_support::engine_run_provenance;
|
||||
use fabro_types::{WorkflowSettings, fixtures};
|
||||
|
||||
-fn test_run_provenance() -> fabro_types::RunProvenance {
|
||||
- fabro_types::RunProvenance {
|
||||
- server: None,
|
||||
- client: None,
|
||||
- subject: fabro_types::Principal::System {
|
||||
- system_kind: fabro_types::SystemActorKind::Engine,
|
||||
- },
|
||||
- }
|
||||
-}
|
||||
-
|
||||
fn templated_settings() -> WorkflowSettings {
|
||||
let mut settings = WorkflowSettings::default();
|
||||
settings.run.goal = Some(RunGoal::Inline(InterpString::parse("Ship {{ env.TASK }}")));
|
||||
@@ -32,7 +23,7 @@ fn run_spec_round_trips_templated_settings() {
|
||||
workflow_slug: Some("demo".to_string()),
|
||||
source_directory: Some("/Users/client/project".to_string()),
|
||||
labels: HashMap::from([("team".to_string(), "platform".to_string())]),
|
||||
- provenance: test_run_provenance(),
|
||||
+ provenance: engine_run_provenance(),
|
||||
manifest_blob: None,
|
||||
definition_blob: None,
|
||||
git: Some(GitContext {
|
||||
6
stages/006-simplify_opus@1/status.json
Normal file
6
stages/006-simplify_opus@1/status.json
Normal file
|
|
@ -0,0 +1,6 @@
|
|||
{
|
||||
"outcome": "succeeded",
|
||||
"notes": "Stage completed: simplify_opus",
|
||||
"failure_reason": null,
|
||||
"timestamp": "2026-05-27T05:24:20.725108Z"
|
||||
}
|
||||
338
stages/007-simplify_gpt@1/prompt.md
Normal file
338
stages/007-simplify_gpt@1/prompt.md
Normal file
|
|
@ -0,0 +1,338 @@
|
|||
Goal: # Plan: Make run actors and provenance total
|
||||
|
||||
## Context
|
||||
|
||||
This is a greenfield app. Backward compatibility with old serialized runs, old API clients, old generated models, and old tests is not a constraint. Prefer the clean invariant and remove all traces of the placeholder shape.
|
||||
|
||||
`Principal::Anonymous` currently represents "no authenticated actor on this request" inside auth middleware. That is auth state, not an actor. A `Principal` should only mean "who acted."
|
||||
|
||||
Likewise, a persisted run should always have a creator. `Run.created_by`, `RunSpec.provenance`, `RunProvenance.subject`, and `run.created` event provenance should all be total. No `Option<Principal>`, no nullable OpenAPI fields, no legacy deserialization defaults, and no fallback creator in projection code.
|
||||
|
||||
Two commits, in order.
|
||||
|
||||
---
|
||||
|
||||
## Commit 1 - Remove `Principal::Anonymous`
|
||||
|
||||
Breaking cleanup. `Principal` becomes actor-only. Missing/invalid auth is represented as absent request principal, not as an anonymous principal variant.
|
||||
|
||||
### Rust
|
||||
|
||||
`lib/crates/fabro-types/src/principal.rs`:
|
||||
- Drop `Anonymous`.
|
||||
- Drop `Anonymous` arms in `kind()` and `display()`.
|
||||
- Delete anonymous serialization/round-trip test coverage.
|
||||
|
||||
`lib/crates/fabro-server/src/principal_middleware.rs`:
|
||||
- `RequestAuthContext.principal: Principal` -> `Option<Principal>`.
|
||||
- `RequestAuthLogContext.principal: Principal` -> `Option<Principal>`.
|
||||
- `initial()` and `rejected()` set `principal: None`.
|
||||
- `authenticated(...)`, `authenticated_worker(...)`, and `authenticated_user(...)` set `principal: Some(...)`.
|
||||
- Update `principal_without_log_unused_fields` to preserve `None` and strip user avatar data only inside `Some(Principal::User(...))`.
|
||||
- Update all gate helpers to match `Option<Principal>`:
|
||||
- `require_user`
|
||||
- `require_authenticated_user`
|
||||
- `require_run_management_actor`
|
||||
- `require_worker_or_user_for_run`
|
||||
- `require_run_management_target`
|
||||
- `None` routes to the existing `auth_rejection(context.auth_status, context.auth_error_code)` behavior.
|
||||
- `Some(Principal::Worker { .. })` keeps the current forbidden-vs-auth-rejection distinctions.
|
||||
- Update tests that assert the initial/rejected principal to assert `None`.
|
||||
|
||||
`lib/crates/fabro-server/src/server.rs` HTTP logging:
|
||||
- Keep the `principal_kind` field on every HTTP log line.
|
||||
- Compute `principal_kind` as `auth_context.principal.as_ref().map(Principal::kind).unwrap_or("none")`.
|
||||
- Match `auth_context.principal` as an `Option<Principal>`:
|
||||
- `Some(User(...))`, `Some(Worker { ... })`, `Some(Webhook { ... })`, `Some(Slack { ... })` keep their extra fields.
|
||||
- `None | Some(Agent { .. } | System { .. })` emits only the common HTTP fields.
|
||||
|
||||
`docs/internal/logging-strategy.md`:
|
||||
- Replace the `anonymous` HTTP caller category guidance with `none` for requests that have no principal.
|
||||
- Keep `auth_status` as the field that distinguishes missing, invalid, expired, and authenticated auth state.
|
||||
|
||||
### OpenAPI and generated clients
|
||||
|
||||
`docs/public/api-reference/fabro-api.yaml`:
|
||||
- Remove `PrincipalAnonymous` from the `Principal` `oneOf`.
|
||||
- Remove `anonymous` from the `Principal` discriminator mapping.
|
||||
- Delete the `PrincipalAnonymous` schema.
|
||||
|
||||
Regenerate:
|
||||
- `cargo build -p fabro-api`
|
||||
- `cd lib/packages/fabro-api-client && bun run generate`
|
||||
|
||||
Expected generated cleanup:
|
||||
- `lib/packages/fabro-api-client/src/models/principal-anonymous.ts` disappears.
|
||||
- `Principal` union no longer includes `{ kind: "anonymous" }`.
|
||||
- `lib/packages/fabro-api-client/src/models/index.ts` no longer exports `principal-anonymous`.
|
||||
|
||||
### Frontend
|
||||
|
||||
`apps/fabro-web/app/lib/principal-display.tsx`:
|
||||
- Remove the `"anonymous"` switch case and unused icon import.
|
||||
|
||||
`apps/fabro-web/app/components/run-summary-panel.test.tsx` and API-client exhaustiveness tests:
|
||||
- Remove anonymous principal cases.
|
||||
|
||||
### Documentation sweep
|
||||
|
||||
Remove anonymous-principal references from product/API docs and tests. Be careful not to touch unrelated uses of "anonymous" such as telemetry anonymous IDs or Git's `remote_anonymous` API.
|
||||
|
||||
Useful sweep:
|
||||
- `rg -n "Principal::Anonymous|PrincipalAnonymous|kind: 'anonymous'|kind: \"anonymous\"|anonymous actor|anonymous subject|principal_kind.*anonymous|\"anonymous\"" lib/crates apps/fabro-web lib/packages/fabro-api-client docs/public docs/internal`
|
||||
|
||||
### Verification
|
||||
|
||||
- `cargo +nightly-2026-04-14 fmt --check --all`
|
||||
- `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings`
|
||||
- `cargo build --workspace`
|
||||
- `cargo nextest run --workspace`
|
||||
- `cd apps/fabro-web && bun run typecheck && bun test`
|
||||
- Manual: start `fabro server start`, hit a protected endpoint without a token, confirm 401 and an HTTP log with `principal_kind="none"` and `auth_status="missing"`.
|
||||
|
||||
---
|
||||
|
||||
## Commit 2 - Make run provenance and creator non-optional
|
||||
|
||||
Full-chain invariant. Every persisted run has exactly one creator principal. No nullable schema fields, no legacy defaults, no projection fallbacks.
|
||||
|
||||
### Core type changes
|
||||
|
||||
`lib/crates/fabro-types/src/run_summary.rs`:
|
||||
- `Run.created_by: Option<Principal>` -> `Principal`.
|
||||
- Drop `#[serde(default)]`.
|
||||
|
||||
`lib/crates/fabro-types/src/run.rs`:
|
||||
- `RunProvenance.subject: Option<Principal>` -> `Principal`.
|
||||
- Drop `#[serde(default, skip_serializing_if = "Option::is_none")]`.
|
||||
- Drop `Default` derive on `RunProvenance`.
|
||||
- `RunSpec.provenance: Option<RunProvenance>` -> `RunProvenance`.
|
||||
- Drop `#[serde(default, skip_serializing_if = "Option::is_none")]` on `RunSpec.provenance`.
|
||||
|
||||
`lib/crates/fabro-types/src/run_event/run.rs`:
|
||||
- `RunCreatedProps.provenance: Option<RunProvenance>` -> `RunProvenance`.
|
||||
- Drop default/skip serialization attributes for provenance.
|
||||
|
||||
`lib/crates/fabro-workflow/src/event/events.rs`:
|
||||
- `Event::RunCreated.provenance: Option<RunProvenance>` -> `RunProvenance`.
|
||||
- Drop default/skip serialization attributes for provenance.
|
||||
|
||||
### Creation and retry flow
|
||||
|
||||
`lib/crates/fabro-workflow/src/operations/create.rs`:
|
||||
- `CreateRunInput.provenance: Option<RunProvenance>` -> `RunProvenance`.
|
||||
- `PersistCreateOptions.provenance: Option<RunProvenance>` -> `RunProvenance`.
|
||||
- `RunSpec { provenance }` stores the total provenance directly.
|
||||
- `Event::RunCreated { provenance }` emits total provenance directly.
|
||||
|
||||
`lib/crates/fabro-server/src/server/handler/runs.rs`:
|
||||
- `run_provenance(headers, subject)` returns `RunProvenance { subject: subject.clone(), ... }`.
|
||||
- Build provenance before creating `CreateRunInput`.
|
||||
|
||||
`lib/crates/fabro-server/src/run_manifest.rs`:
|
||||
- Change `create_run_input(...)` to accept `provenance: RunProvenance` and set it directly, or stop using the helper for the final `CreateRunInput` construction. Do not create a temporary input with missing provenance.
|
||||
|
||||
`lib/crates/fabro-workflow/src/operations/retry.rs`:
|
||||
- `RetryRunInput.provenance: Option<RunProvenance>` -> `RunProvenance`.
|
||||
- `retry_run(...)` writes the new run's `run.created` event with total provenance.
|
||||
|
||||
`lib/crates/fabro-server/src/server/handler/lifecycle.rs`:
|
||||
- Pass `run_provenance(&headers, &actor)` directly into `RetryRunInput`.
|
||||
|
||||
### Event conversion and projections
|
||||
|
||||
`lib/crates/fabro-workflow/src/event/convert.rs`:
|
||||
- Convert `Event::RunCreated.provenance` into `RunCreatedProps.provenance` directly.
|
||||
- Remove `Some(...)` wrapping for run-created provenance.
|
||||
|
||||
`lib/crates/fabro-workflow/src/event/stored_fields.rs`:
|
||||
- `Event::RunCreated { provenance, .. }` sets `actor: Some(provenance.subject.clone())`.
|
||||
|
||||
`lib/crates/fabro-store/src/run_state.rs`:
|
||||
- `projection_from_created(...)` builds `RunSpec { provenance: props.provenance.clone(), ... }`.
|
||||
- `build_summary(...)` sets `created_by: state.spec.provenance.subject.clone()`.
|
||||
- Delete or rewrite tests that deserialize projections with `"provenance": null`.
|
||||
|
||||
`lib/crates/fabro-types/src/run_projection.rs` and projection tests:
|
||||
- Replace all test `RunSpec` literals with total provenance.
|
||||
- Remove tests whose only purpose is legacy/null provenance tolerance.
|
||||
|
||||
### OpenAPI
|
||||
|
||||
`docs/public/api-reference/fabro-api.yaml`:
|
||||
- `Run.created_by` references `Principal` directly. Remove `oneOf [..., null]`.
|
||||
- `RunProvenance.required` includes `subject`.
|
||||
- `RunProvenance.subject` references `Principal` directly. Remove `oneOf [..., null]`.
|
||||
- `RunSpec.required` includes `provenance`.
|
||||
- `RunSpec.provenance` references `RunProvenance` directly. Remove `oneOf [..., null]`.
|
||||
- If `run.created` event properties are represented separately in the spec, make that event provenance required and non-nullable too.
|
||||
|
||||
Regenerate:
|
||||
- `cargo build -p fabro-api`
|
||||
- `cd lib/packages/fabro-api-client && bun run generate`
|
||||
|
||||
Do not hand-edit generated client files.
|
||||
|
||||
### Demo mode
|
||||
|
||||
`lib/crates/fabro-server/src/demo/mod.rs`:
|
||||
- Add a clearly synthetic demo principal using `AuthMethod::DevToken`, not GitHub:
|
||||
```rust
|
||||
static DEMO_PRINCIPAL: LazyLock<Principal> = LazyLock::new(|| {
|
||||
Principal::user(
|
||||
IdpIdentity::new("fabro:demo", "demo").unwrap(),
|
||||
"demo".to_string(),
|
||||
AuthMethod::DevToken,
|
||||
)
|
||||
});
|
||||
```
|
||||
- Replace `created_by: None` with `created_by: DEMO_PRINCIPAL.clone()`.
|
||||
- If demo creates any full `RunSpec` or `run.created` event data, give it `RunProvenance { subject: DEMO_PRINCIPAL.clone(), ... }`.
|
||||
|
||||
### Test support
|
||||
|
||||
Do not add fake auth helpers to `fabro_types::fixtures`; that module is run-id constants.
|
||||
|
||||
Use the existing `fabro-types` `test-support` feature:
|
||||
- Add `#[cfg(any(test, feature = "test-support"))] pub mod test_support;` in `lib/crates/fabro-types/src/lib.rs` if it does not already exist.
|
||||
- Add `lib/crates/fabro-types/src/test_support.rs` with:
|
||||
- `test_principal() -> Principal`
|
||||
- `test_run_provenance() -> RunProvenance`
|
||||
- Use an obviously fake dev-token identity, e.g. issuer `fabro:test`, subject `test-user`, login `test`.
|
||||
- In crates that need the helper from integration tests or cross-crate tests, dual-list `fabro-types` in `dev-dependencies` with `features = ["test-support"]`, following existing repo patterns.
|
||||
|
||||
Update all constructors:
|
||||
- Replace `provenance: None` in `RunSpec`, `CreateRunInput`, `RetryRunInput`, `Event::RunCreated`, and `RunCreatedProps` literals with `test_run_provenance()` or a locally meaningful provenance.
|
||||
- Replace `subject: Some(...)` with `subject: ...`.
|
||||
- Replace `subject: None` only when it is actually `RunProvenance.subject`; leave unrelated todo/commit/message `subject` fields alone.
|
||||
- Replace `created_by: None` / `created_by: null` with `test_principal()` or a frontend TS principal fixture.
|
||||
- Delete tests that assert nullable or omitted creator/provenance behavior.
|
||||
|
||||
Representative Rust areas:
|
||||
- `lib/crates/fabro-store/src/run_state.rs`
|
||||
- `lib/crates/fabro-store/tests/serializable_projection.rs`
|
||||
- `lib/crates/fabro-workflow/src/operations/{create,retry,start}.rs`
|
||||
- `lib/crates/fabro-workflow/src/event/{convert,sink,stored_fields}.rs`
|
||||
- `lib/crates/fabro-workflow/src/handler/**`
|
||||
- `lib/crates/fabro-workflow/src/pipeline/**`
|
||||
- `lib/crates/fabro-workflow/src/run_{lookup,metadata}.rs`
|
||||
- `lib/crates/fabro-server/src/server/tests.rs`
|
||||
- `lib/crates/fabro-server/src/server/handler/**`
|
||||
- `lib/crates/fabro-server/tests/it/**`
|
||||
- `lib/crates/fabro-cli/tests/it/support/mod.rs`
|
||||
- `lib/crates/fabro-dump/src/lib.rs`
|
||||
- `lib/crates/fabro-tool/src/{common,create,interact,search}.rs`
|
||||
- `lib/crates/fabro-api/tests/{principal_round_trip,run_summary_round_trip,run_projection_round_trip,run_event_round_trip}.rs`
|
||||
- `lib/crates/fabro-types/tests/{run_spec_serde,run_spec_methods,run_event_serde}.rs`
|
||||
|
||||
Representative TypeScript areas:
|
||||
- `apps/fabro-web/app/**` tests with `created_by: null`
|
||||
- `apps/fabro-web/app/data/runs.ts`
|
||||
- `apps/fabro-web/app/components/run-summary-panel.tsx`
|
||||
- `apps/fabro-web/app/components/runs-list/**`
|
||||
- `lib/packages/fabro-api-client/tests/principal-exhaustive.ts`
|
||||
|
||||
Useful sweep after edits:
|
||||
- `rg -n "Principal::Anonymous|PrincipalAnonymous|principal-anonymous|kind: ['\"]anonymous|created_by:\\s*(None|null)|provenance:\\s*None|subject:\\s*Some\\(|subject:\\s*None" lib/crates apps/fabro-web lib/packages/fabro-api-client docs/public docs/internal`
|
||||
|
||||
Review each hit. The only acceptable remaining matches should be unrelated uses of "anonymous" and unrelated non-principal `subject` fields.
|
||||
|
||||
### Frontend
|
||||
|
||||
`apps/fabro-web/app/components/run-summary-panel.tsx`:
|
||||
- `run?.created_by` may still be guarded by `run` loading state, but `created_by` itself is non-null once `run` exists.
|
||||
- Pass `run.created_by` directly to `principalDisplay(...)` inside loaded-run branches.
|
||||
|
||||
`apps/fabro-web/app/data/runs.ts` and run-list components:
|
||||
- Treat `createdBy` as a total principal in UI data derived from a loaded API run.
|
||||
- Remove empty/fallback rendering that only existed for missing creator data.
|
||||
|
||||
### Verification
|
||||
|
||||
- `cargo +nightly-2026-04-14 fmt --check --all`
|
||||
- `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings`
|
||||
- `cargo build --workspace`
|
||||
- `cargo nextest run --workspace`
|
||||
- `cargo nextest run -p fabro-server`
|
||||
- `cd apps/fabro-web && bun run typecheck && bun test && bun run build`
|
||||
- Manual end-to-end:
|
||||
- `fabro server start`
|
||||
- `cd apps/fabro-web && bun run dev`
|
||||
- Authenticate and create a run through the UI.
|
||||
- Confirm `/api/v1/runs/:id` has non-null `created_by`.
|
||||
- Confirm `/api/v1/runs/:id/state` has non-null `spec.provenance.subject`.
|
||||
- Retry a failed run and confirm the retried run has the retrying user as creator.
|
||||
- Hit demo mode with `X-Fabro-Demo: 1` and confirm the run summary renders the synthetic `demo` dev-token user.
|
||||
|
||||
|
||||
## 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`
|
||||
- Output:
|
||||
```
|
||||
cargo 1.95.0 (f2d3ce0bd 2026-03-21)
|
||||
```
|
||||
- **preflight_compile**: succeeded
|
||||
- Script: `cargo check -q --workspace 2>&1`
|
||||
- Output: (empty)
|
||||
- **preflight_lint**: succeeded
|
||||
- Script: `cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1`
|
||||
- Output: (empty)
|
||||
- **implement**: succeeded
|
||||
- Model: gpt-5.5, 9.9m tokens in / 73.0k out
|
||||
- Files: /home/daytona/workspace/fabro/apps/fabro-web/app/lib/test-principal.ts, /home/daytona/workspace/fabro/lib/crates/fabro-types/src/test_support.rs
|
||||
- **simplify_opus**: succeeded
|
||||
- Model: claude-opus-4-7, 69.5k tokens in / 25.2k out
|
||||
- Files: /home/daytona/workspace/fabro/lib/crates/fabro-server/src/run_manifest.rs, /home/daytona/workspace/fabro/lib/crates/fabro-server/src/server.rs, /home/daytona/workspace/fabro/lib/crates/fabro-server/src/server/handler/runs.rs, /home/daytona/workspace/fabro/lib/crates/fabro-types/Cargo.toml, /home/daytona/workspace/fabro/lib/crates/fabro-types/src/test_support.rs, /home/daytona/workspace/fabro/lib/crates/fabro-types/tests/run_event_serde.rs, /home/daytona/workspace/fabro/lib/crates/fabro-types/tests/run_spec_methods.rs, /home/daytona/workspace/fabro/lib/crates/fabro-types/tests/run_spec_serde.rs, /home/daytona/workspace/fabro/lib/crates/fabro-workflow/src/test_support.rs
|
||||
|
||||
|
||||
# Simplify: Code Review and Cleanup
|
||||
|
||||
Review changes vs. origin for reuse, quality, and efficiency. Fix any issues found.
|
||||
|
||||
## Phase 1: Identify Changes
|
||||
|
||||
Run git diff (or git diff HEAD if there are staged changes) to see what changed. If there are no git changes, review the most recently modified files that the user mentioned or that you edited earlier in this conversation.
|
||||
|
||||
## Phase 2: Launch Three Review Agents in Parallel
|
||||
|
||||
Use the Agent tool to launch all three agents concurrently in a single message. Pass each agent the full diff so it has the complete context.
|
||||
|
||||
### Agent 1: Code Reuse Review
|
||||
|
||||
For each change:
|
||||
|
||||
1. Search for existing utilities and helpers that could replace newly written code. Use Grep to find similar patterns elsewhere in the codebase — common locations are utility directories, shared modules, and files adjacent to the changed ones.
|
||||
2. Flag any new function that duplicates existing functionality. Suggest the existing function to use instead.
|
||||
3. Flag any inline logic that could use an existing utility — hand-rolled string manipulation, manual path handling, custom environment checks, ad-hoc type guards, and similar patterns are common candidates.
|
||||
|
||||
Note: This is a greenfield app, so focus on maximizing simplicity and don't worry about changing things to achieve it.
|
||||
|
||||
### Agent 2: Code Quality Review
|
||||
|
||||
Review the same changes for hacky patterns:
|
||||
|
||||
1. Redundant state: state that duplicates existing state, cached values that could be derived, observers/effects that could be direct calls
|
||||
2. Parameter sprawl: adding new parameters to a function instead of generalizing or restructuring existing ones
|
||||
3. Copy-paste with slight variation: near-duplicate code blocks that should be unified with a shared abstraction
|
||||
4. Leaky abstractions: exposing internal details that should be encapsulated, or breaking existing abstraction boundaries
|
||||
5. Stringly-typed code: using raw strings where constants, enums (string unions), or branded types already exist in the codebase
|
||||
|
||||
Note: This is a greenfield app, so be aggressive in optimizing quality.
|
||||
|
||||
### Agent 3: Efficiency Review
|
||||
|
||||
Review the same changes for efficiency:
|
||||
|
||||
1. Unnecessary work: redundant computations, repeated file reads, duplicate network/API calls, N+1 patterns
|
||||
2. Missed concurrency: independent operations run sequentially when they could run in parallel
|
||||
3. Hot-path bloat: new blocking work added to startup or per-request/per-render hot paths
|
||||
4. Unnecessary existence checks: pre-checking file/resource existence before operating (TOCTOU anti-pattern) — operate directly and handle the error
|
||||
5. Memory: unbounded data structures, missing cleanup, event listener leaks
|
||||
6. Overly broad operations: reading entire files when only a portion is needed, loading all items when filtering for one
|
||||
|
||||
## Phase 3: Fix Issues
|
||||
|
||||
Wait for all three agents to complete. Aggregate their findings and fix each issue directly. If a finding is a false positive or not worth addressing, note it and move on — do not argue with the finding, just skip it.
|
||||
|
||||
When done, briefly summarize what was fixed (or confirm the code was already clean).
|
||||
5
stages/007-simplify_gpt@1/provider_used.json
Normal file
5
stages/007-simplify_gpt@1/provider_used.json
Normal file
|
|
@ -0,0 +1,5 @@
|
|||
{
|
||||
"mode": "agent",
|
||||
"provider": "openai",
|
||||
"model": "gpt-5.5"
|
||||
}
|
||||
19
stages/007-simplify_gpt@1/response.md
Normal file
19
stages/007-simplify_gpt@1/response.md
Normal file
|
|
@ -0,0 +1,19 @@
|
|||
Reviewed the branch diff and applied one cleanup.
|
||||
|
||||
Fixed:
|
||||
- Removed the extra `engine_run_provenance()` test helper from `fabro-types::test_support`.
|
||||
- Updated affected `fabro-types` serde/method tests to use the canonical requested `test_run_provenance()` helper instead.
|
||||
- This keeps test-support minimal and aligned with the plan’s intended API: `test_principal()` + `test_run_provenance()`.
|
||||
|
||||
Validation run:
|
||||
- `cargo +nightly-2026-04-14 fmt --check --all` ✅
|
||||
- `cargo check -q --workspace` ✅
|
||||
- `cd apps/fabro-web && bun run typecheck` ✅
|
||||
- `cargo nextest run -p fabro-types` ✅
|
||||
- `cargo nextest run -p fabro-server principal_middleware` ✅
|
||||
|
||||
Working tree now has only the cleanup edits in:
|
||||
- `lib/crates/fabro-types/src/test_support.rs`
|
||||
- `lib/crates/fabro-types/tests/run_event_serde.rs`
|
||||
- `lib/crates/fabro-types/tests/run_spec_methods.rs`
|
||||
- `lib/crates/fabro-types/tests/run_spec_serde.rs`
|
||||
Loading…
Add table
Reference in a new issue