feat(server): standalone scheduler, attach listener and dashboard - #584
Conversation
One binary runs the scheduler, accepts executor attachments, and serves the dashboard with no language runtime, so one small image fits every SDK. The dashboard is a full port rather than a subset because one SPA build is served by every implementation; auth and feature-store keys match the SDKs so sessions interoperate.
An id_token naming an unknown kid fell back to the only published key, so a token signed by a retired key still verified. Fall back only when the token names no kid at all.
A stub provider — discovery, JWKS, and a token endpoint signing with a fixture key — is what makes the token exchange, the JWKS fetch, and the signature check testable at all. Seven forged or stale id_tokens must each be refused, and each case asserts the exchange happened so none can pass by failing earlier.
One SPA build is served by every implementation, so the JSON has to agree. Nineteen routes must match byte for byte, three after dropping clock-derived fields, and six only in shape because they read a language runtime the standalone server does not have. Skipped unless that server is built.
Password verification is deliberately expensive, so an unthrottled login is both a guessing oracle and a cheap way to occupy every blocking thread: ten failures per identity per five minutes, cleared on success. A lockout answers 429 with Retry-After, and hides a hit from a miss while it holds. Expired sessions and abandoned OAuth state were only swept at startup, which never happens on a server that stays up; they now sweep on a cadence.
Signing, headers, and the recorded outcome are only observable from the receiving end, so the store tests could not see any of them. A stub endpoint proves the signature covers the exact bytes sent, that a rejecting or unreachable endpoint is reported rather than swallowed, and that a replay appends a new record instead of rewriting the original.
GitHub publishes no discovery document, so its endpoints were hardcoded and the whole exchange was unreachable from a test. Making them overridable is what lets a stub stand in, and the flow's real decisions — the account id as the subject, the primary verified email, org membership — now have coverage. A failing membership check reports oauth_failed, not oauth_denied: a broken check is not a verdict about the user.
Only the standalone server serves /api/executors; an SDK dashboard answers 404. The nav entry stays hidden until the server confirms the route, and the page explains itself rather than erroring if someone navigates to it anyway — one SPA build is served by every implementation, so it cannot assume the route exists.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds a standalone ChangesStandalone server and dashboard
Executor dashboard
Cross-SDK parity
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant Dashboard
participant Storage
participant Executor
Browser->>Dashboard: request dashboard route
Dashboard->>Storage: query jobs, queues, users, or settings
Storage-->>Dashboard: return stored data
Executor->>Dashboard: attach executor connection
Dashboard-->>Browser: return dashboard response
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
pratyush618
changed the title
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
crates/taskito-server/src/dashboard/webhook_sender.rs-76-84 (1)
76-84: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSubscription headers can shadow
Content-Typeand the signature header.
RequestBuilder::headerappends a value; it does not replace one. A stored header namedX-Taskito-Signaturetherefore ships next to the computed signature, and a receiver that reads the first value verifies operator-supplied bytes instead. A storedContent-Typehas the same effect.Reject reserved header names in
routes/webhooks.rs::coerce_headers, or insert the custom headers first throughheaders_mutso the computed values overwrite them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/webhook_sender.rs` around lines 76 - 84, The webhook request construction allows subscription headers to duplicate reserved computed headers. Update routes/webhooks.rs::coerce_headers to reject case-insensitive Content-Type and SIGNATURE_HEADER names before storing custom headers, preserving the existing computed Content-Type and signature behavior in the sender.crates/taskito-server/src/dashboard/routes/webhooks.rs-201-208 (1)
201-208: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA negative
offsetwraps to a hugeusizeand returns an empty page.
limitis clamped,offsetis not. For?offset=-1,offset as usizebecomesusize::MAX,deliveries::list_forskips everything, and the response reports"offset": -1with an emptyitemsarray whiletotalis non-zero. Either reject the value or clamp it.🐛 Proposed fix
let limit = params.int("limit", 50)?.clamp(1, MAX_PAGE_SIZE); - let offset = params.int("offset", 0)?; + let offset = params.int("offset", 0)?.max(0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/routes/webhooks.rs` around lines 201 - 208, Validate the parsed offset in the webhook route before constructing DeliveryFilter, preventing negative values from being cast to usize; either reject negative offsets or clamp them to zero, while preserving valid offset behavior and ensuring the response uses the normalized value.crates/taskito-server/src/dashboard/stores/url_safety.rs-116-154 (1)
116-154: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winIPv4-compatible and NAT64 IPv6 forms skip the IPv4 screening.
is_privatemaps only IPv4-mapped addresses back to IPv4.::127.0.0.1(IPv4-compatible) and64:ff9b::7f00:1(NAT64) reachis_private_v6, which does not cover either range, so a loopback or link-local IPv4 target passes.🛡️ Proposed fix
fn is_private_v6(address: Ipv6Addr) -> bool { let segments = address.segments(); + // NAT64 well-known prefix 64:ff9b::/96 and IPv4-compatible ::a.b.c.d both + // embed an IPv4 destination. + if segments[0] == 0x0064 && segments[1] == 0xff9b { + return true; + } + if let Some(compatible) = address.to_ipv4() { + return is_private_v4(compatible); + } address.is_loopback()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/stores/url_safety.rs` around lines 116 - 154, Update is_private to recognize IPv4-compatible and NAT64 IPv6 addresses and convert their embedded IPv4 destinations before screening; route those extracted addresses through is_private_v4, while retaining is_private_v6 for ordinary IPv6 addresses. Ensure embedded loopback, private, link-local, and other blocked IPv4 targets cannot bypass validation.crates/taskito-server/tests/support/github_stub.rs-81-85 (1)
81-85: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the doc comment on
with_membership_status.The comment states "Not a member of any allowed org." The method sets an arbitrary status.
crates/taskito-server/tests/github_login.rscalls it with 204 for a member (line 169), 404 for a non-member (line 190), and 500 for a broken check (line 213). The current comment describes only one caller and contradicts the 204 case.📝 Proposed doc fix
- /// Not a member of any allowed org. + /// Answer membership checks with `status`: 204 member, 404 not a member, + /// anything else a failed check. pub fn with_membership_status(mut self, status: u16) -> Self {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/tests/support/github_stub.rs` around lines 81 - 85, Update the doc comment on the with_membership_status method to accurately describe its purpose. The current comment "Not a member of any allowed org." describes only one specific status value, but the method accepts and sets arbitrary HTTP status codes (204 for member, 404 for non-member, 500 for error). Rewrite the comment to reflect that this method configures the membership check response status.crates/taskito-server/src/config/backend.rs-34-43 (1)
34-43: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject an unrecognised URL scheme instead of treating it as a SQLite path.
open_by_schemesends every non-Postgres, non-Redis DSN toopen_sqlite.open_sqlitestrips onlysqlite://, so a DSN likemysql://host/db, or a typo likepostgre://host/db, becomes a SQLite filename. SQLite creates that file, the server starts against an empty database, and the operator sees an empty dashboard instead of a configuration error.Bare paths and
:memory:contain no://, so a scheme guard keeps them working.🛡️ Proposed fix
fn open_by_scheme(dsn: &str) -> Result<Backend> { let lower = dsn.to_ascii_lowercase(); if lower.starts_with("postgres://") || lower.starts_with("postgresql://") { open_postgres(dsn) } else if lower.starts_with("redis://") || lower.starts_with("rediss://") { open_redis(dsn) + } else if let Some((scheme, _)) = lower.split_once("://").filter(|(s, _)| *s != "sqlite") { + // Don't silently create a SQLite file named after the URL. + bail!( + "TASKITO_DSN uses an unsupported scheme '{scheme}://'; \ + use sqlite, postgres, or redis, or set TASKITO_BACKEND" + ) } else { open_sqlite(dsn) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/config/backend.rs` around lines 34 - 43, Update open_by_scheme to reject DSNs containing an unrecognized “://” scheme instead of passing them to open_sqlite; preserve SQLite handling for bare paths and :memory:, while continuing to route recognized PostgreSQL and Redis schemes to their existing openers.crates/taskito-server/src/config/dashboard.rs-87-91 (1)
87-91: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFail when only one half of the admin bootstrap is set.
admin_bootstrapreturnsNoneif either variable is missing. WithTASKITO_DASHBOARD_AUTH=sessionand onlyTASKITO_DASHBOARD_ADMIN_USERset, the server starts with no account and gives the operator no reason. A typo in the password variable name produces the same silent outcome.Report the missing half instead. This changes the expectation in the
bootstrap_needs_both_halvestest at lines 165-177.🛡️ Proposed fix
-fn admin_bootstrap(env: &Env) -> Option<(String, String)> { - let username = value(env, "TASKITO_DASHBOARD_ADMIN_USER")?; - let password = value(env, "TASKITO_DASHBOARD_ADMIN_PASSWORD")?; - Some((username, password)) +fn admin_bootstrap(env: &Env) -> Result<Option<(String, String)>> { + match ( + value(env, "TASKITO_DASHBOARD_ADMIN_USER"), + value(env, "TASKITO_DASHBOARD_ADMIN_PASSWORD"), + ) { + (Some(username), Some(password)) => Ok(Some((username, password))), + (None, None) => Ok(None), + (Some(_), None) => bail!( + "TASKITO_DASHBOARD_ADMIN_USER is set without TASKITO_DASHBOARD_ADMIN_PASSWORD; \ + set both to bootstrap the first admin, or neither" + ), + (None, Some(_)) => bail!( + "TASKITO_DASHBOARD_ADMIN_PASSWORD is set without TASKITO_DASHBOARD_ADMIN_USER; \ + set both to bootstrap the first admin, or neither" + ), + } }Then propagate at line 66:
admin_bootstrap: admin_bootstrap(env)?,.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/config/dashboard.rs` around lines 87 - 91, The admin_bootstrap function currently returns None silently when either TASKITO_DASHBOARD_ADMIN_USER or TASKITO_DASHBOARD_ADMIN_PASSWORD is missing, making it impossible to distinguish between intentionally disabling admin bootstrap and accidentally misconfiguring it. Update admin_bootstrap to validate that either both environment variables are present or both are absent. When only one variable is set, the function should report an error indicating which variable is missing rather than returning None. This ensures misconfigurations like a typo in the password variable name are caught instead of silently proceeding with no admin account. Update the test bootstrap_needs_both_halves to reflect this new error-reporting behavior for incomplete configurations.crates/taskito-server/build.rs-20-29 (1)
20-29: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRegister the candidate directories so a later SPA build triggers a rebuild.
The build script prints
cargo:rerun-if-env-changed, which disables the default rebuild-on-any-file-change behaviour. In theNonebranch nocargo:rerun-if-changedis printed at all. If a developer builds the server beforedashboard/distexists and then runs the SPA build, cargo does not re-run this script. The binary keeps the empty table and serves the "assets not bundled" page until the target is cleaned.Print the candidate paths in both branches.
🐛 Proposed fix
let out_dir = PathBuf::from(env::var("OUT_DIR").expect("cargo always sets OUT_DIR")); + // Track the candidates even when none exists yet, so building the SPA + // afterwards re-runs this script instead of keeping an empty table. + let workspace_root = workspace_root(); + for relative in CANDIDATE_DIRS { + println!( + "cargo:rerun-if-changed={}", + workspace_root.join(relative).display() + ); + } let generated = match locate_assets() { Some(root) => { - println!("cargo:rerun-if-changed={}", root.display()); let mut files = Vec::new();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/build.rs` around lines 20 - 29, The None branch of locate_assets() fails to register candidate directories with cargo:rerun-if-changed, preventing rebuild when the asset directory is created later. Add cargo:rerun-if-changed println statements in the None branch to register the candidate paths that locate_assets would have searched, ensuring cargo re-runs this build script when the SPA build creates dashboard/dist and other asset directories.crates/taskito-server/src/dashboard/routes/overrides.rs-132-150 (1)
132-150: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReorder writes in
put_queueto avoid a partial-failure inconsistency.
put_queuepersists the override first (Line 138), then applies the live pause/resume change (Lines 139-148). If the live storage call fails after the override write succeeds, the override record reportspausedas the new value while the queue's actual live state has not changed. Apply the live change first, and only persist the override once the live change succeeds. This way, a failure surfaces before any state is recorded, and a retry using the same idempotent PUT body converges on the correct state.🐛 Proposed reorder
pub async fn put_queue( State(state): State<SharedState>, Path(queue_name): Path<String>, Json(body): Json<Value>, ) -> ApiResult<Json<Value>> { let paused = body.get("paused").and_then(Value::as_bool); - let response = put_override(state.clone(), Scope::Queue, queue_name.clone(), body).await?; if let Some(paused) = paused { + let name = queue_name.clone(); on_storage(&state, move |storage| { if paused { - storage.pause_queue(&queue_name) + storage.pause_queue(&name) } else { - storage.resume_queue(&queue_name) + storage.resume_queue(&name) } }) .await?; } + let response = put_override(state, Scope::Queue, queue_name, body).await?; Ok(response) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/routes/overrides.rs` around lines 132 - 150, Update put_queue so the live pause_queue or resume_queue operation in on_storage runs before calling put_override. Only persist the override after the live storage change succeeds, while preserving the existing behavior when paused is absent and returning the persisted override response.crates/taskito-server/src/dashboard/auth/oauth/config.rs-202-216 (1)
202-216: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject OIDC slots whose derived env prefix collides.
Line 209 maps '-' to '_' when it builds the prefix.
acme-oktaandacme_oktatherefore both deriveTASKITO_DASHBOARD_OAUTH_OIDC_ACME_OKTA_*. The duplicate check on line 204 compares slot strings, so both slots are accepted and both read the same client id, secret, and discovery URL. The second provider then authenticates against the first provider's issuer, and startup reports nothing.Compare derived prefixes instead of raw slots.
🐛 Proposed fix
fn oidc_provider(env: &Env, slot: &str, existing: &[ProviderConfig]) -> Result<ProviderConfig> { validate_slot(slot)?; - if existing.iter().any(|provider| provider.slot == slot) { - bail!("OIDC slot '{slot}' is listed twice"); - } - let prefix = format!( - "TASKITO_DASHBOARD_OAUTH_OIDC_{}", - slot.to_ascii_uppercase().replace('-', "_") - ); + let prefix = format!( + "TASKITO_DASHBOARD_OAUTH_OIDC_{}", + slot.to_ascii_uppercase().replace('-', "_") + ); + // '-' and '_' collapse to the same prefix, so two distinct slots can + // otherwise read one set of credentials. + if existing.iter().any(|provider| { + provider.slot == slot + || provider.slot.to_ascii_uppercase().replace('-', "_") + == slot.to_ascii_uppercase().replace('-', "_") + }) { + bail!("OIDC slot '{slot}' collides with another slot on the env prefix {prefix}"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/auth/oauth/config.rs` around lines 202 - 216, Update oidc_provider to reject duplicate derived environment prefixes, not just identical raw slot names: construct the normalized prefix before the duplicate check and compare it against each existing provider’s derived prefix using the same uppercase and hyphen-to-underscore normalization. Preserve the existing duplicate-slot error behavior while reporting collisions for distinct slots that map to the same prefix.crates/taskito-server/src/dashboard/auth/oauth/providers/oidc.rs-171-175 (1)
171-175: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUse the JWK's declared algorithm instead of the token header's algorithm for validation.
The code currently passes
header.alg(controlled by the token presenter) toValidation::new(). Thejsonwebtokenlibrary does validate that this algorithm matches the key type derived from the JWK, which provides some protection. However, OpenID Connect security best practices recommend using the issuer's declared algorithm (jwk.common.key_algorithm) rather than the presenter's header claim. This makes the security policy explicit in the code and eliminates reliance on implicit library behavior.If
jwk.common.key_algorithmexists, convert it to ajsonwebtoken::Algorithmand use that for validation. Fall back toheader.algonly if the JWK does not declare an algorithm:Proposed fix
- let mut validation = Validation::new(header.alg); + // Use the issuer's declared algorithm from the JWK, not the + // presenter's header claim, to follow OpenID Connect best practices. + let algorithm = jwk + .common + .key_algorithm + .and_then(|alg| alg.to_string().parse::<jsonwebtoken::Algorithm>().ok()) + .unwrap_or(header.alg); + let mut validation = Validation::new(algorithm);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/auth/oauth/providers/oidc.rs` around lines 171 - 175, Update the validation setup around Validation::new in the OIDC provider to derive the algorithm from jwk.common.key_algorithm when present, converting it to jsonwebtoken::Algorithm, and fall back to header.alg only when the JWK omits it. Keep the existing audience, issuer, leeway, and expiration validation settings unchanged.crates/taskito-server/src/dashboard/static_assets.rs-105-115 (1)
105-115: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winThe
starts_withcheck does not detect the symlink case the comment describes.
Path::starts_withcompares components lexically. It does not resolve symlinks. A symlink inside the bundle directory therefore still passes this check, andstd::fs::readfollows it and serves the target. The comment states that this branch catches a path that leaves the root through a symlink, so the code and the comment disagree.The bundle directory comes from
TASKITO_DASHBOARD_ASSETSand is operator-supplied, so this is not directly attacker-controlled. It still matters when the directory is a mounted volume that another workload can write.Canonicalize both paths before you compare them, or correct the comment to state that only lexical escapes are rejected.
🔒 Proposed fix
Source::Directory(root) => { let path = root.join(relative); - // `sanitize` already rejected traversal, so a path that leaves - // the root can only come from a symlink inside the bundle. - if !path.starts_with(root) || !path.is_file() { - return None; - } - std::fs::read(path).ok().map(Cow::Owned) + // `sanitize` already rejected traversal; canonicalizing also + // resolves a symlink inside the bundle that points outside it. + let resolved = path.canonicalize().ok()?; + let root = root.canonicalize().ok()?; + if !resolved.starts_with(&root) || !resolved.is_file() { + return None; + } + std::fs::read(resolved).ok().map(Cow::Owned) }
canonicalizecalls the filesystem on every request. If that cost matters on the asset path, canonicalize the root once inStaticAssets::newand store it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/static_assets.rs` around lines 105 - 115, Update the Source::Directory handling to canonicalize the requested path and the configured root before comparing them, so symlinks escaping the bundle are rejected; preserve the existing None behavior for invalid or non-file paths. Prefer canonicalizing the root once in StaticAssets::new and reuse it during requests if that fits the existing structure.crates/taskito-server/src/dashboard/auth/oauth/state.rs-62-80 (1)
62-80: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winMake the single-use check depend on the delete result.
consumereads the row and then deletes it in two separate storage operations. Two concurrent callbacks that carry the same state token can both complete the read before either delete runs. Both then receive a liveOAuthState, so the single-use guarantee stated in the doc comment does not hold under concurrency. The authorization server still rejects the second code exchange, so the exposure is limited to duplicate exchange attempts.
delete_settingalready returns a boolean, as used on Line 90. Use that result as the claim check.🔒 Proposed fix to claim the row before returning it
let key = format!("{STATE_PREFIX}{token}"); let Some(raw) = storage.get_setting(&key)? else { return Ok(None); }; - storage.delete_setting(&key)?; + // Only the caller that removes the row owns this flow; a concurrent + // replay observes `false` and is refused. + if !storage.delete_setting(&key)? { + return Ok(None); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/auth/oauth/state.rs` around lines 62 - 80, Update OAuthState::consume to use the boolean result from delete_setting as the single-use claim check: return None when deletion reports that the row was not deleted, and only parse and return the state after a successful delete. Preserve the existing empty-token, missing-row, malformed-state, and expiration handling.crates/taskito-server/src/dashboard/auth/cookies.rs-17-29 (1)
17-29: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUse
SameSite=Laxfor the session cookie. The OAuth callback sets the cookie before redirecting torow.next_url, but browsers can treat the redirect chain as cross-site and omit the newSameSite=Strictcookie from the landing request. KeepSameSite=Strictfor the CSRF cookie.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/auth/cookies.rs` around lines 17 - 29, Update established so the session cookie built from SESSION_COOKIE uses SameSite=Lax, while the CSRF_COOKIE remains SameSite=Strict. Preserve the existing security attributes, expiration, and redirect behavior.
🧹 Nitpick comments (19)
crates/taskito-server/src/dashboard/stores/kv.rs (1)
35-48: 🚀 Performance & Scalability | 🔵 Trivial
scan_prefixreads every setting value to serve a prefix listing.
list_settings()returns all keys and all values. Delivery logs are stored in the same table and hold up to 200 records per webhook, so an overrides or middleware listing transfers and decodes those large values as well. The dashboard polls these routes, so the cost repeats.If
Storagegains a prefix-scoped or keys-only listing, prefer it here. Alternatively cache the listing for a short interval in the dashboard state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/stores/kv.rs` around lines 35 - 48, Update scan_prefix to avoid loading and decoding every setting value when serving prefix listings, preferably by using a prefix-scoped or keys-only Storage listing API if available, then fetch values only for matching keys. Preserve the existing suffix formatting, sorting, and Result behavior; avoid adding unrelated caching unless no suitable Storage API exists.sdks/python/tests/dashboard/test_rust_server_parity.py (1)
299-303: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the listing status before you read the body.
Line 299 discards the status code. If
/api/jobs?limit=50returns an error body, line 300 raisesTypeErrororKeyErrorinstead of reporting the failed request. Add the status assertion so the failure names the cause.♻️ Proposed refactor
- _, listed = _get(python_url, "/api/jobs?limit=50") + listing_status, listed = _get(python_url, "/api/jobs?limit=50") + assert listing_status == 200, listed ids = [job["id"] for job in listed]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/python/tests/dashboard/test_rust_server_parity.py` around lines 299 - 303, Capture the status code returned from the _get call to "/api/jobs?limit=50" instead of discarding it, and add an assertion that the status code indicates success before attempting to access fields like job["id"] and job["error"] in the subsequent lines. This ensures that if the endpoint returns an error response, the failure message clearly identifies the failed request rather than raising a confusing TypeError or KeyError when trying to read an invalid body.crates/taskito-server/tests/http_api.rs (1)
141-143: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winStrengthen the traversal assertion.
The assertion accepts both
NOT_FOUNDandOK, so it passes whatever the router does. It cannot detect a regression that serves the repository file. Assert on the response content instead of only the status.♻️ Proposed change
// Traversal is refused before it reaches the filesystem. - let (status, _, _) = call(&state, get("/../Cargo.toml")).await; - assert!(status == StatusCode::NOT_FOUND || status == StatusCode::OK); + let (status, headers, _) = call(&state, get("/../Cargo.toml")).await; + assert!( + status == StatusCode::NOT_FOUND || is_html(&headers), + "traversal must 404 or fall back to the SPA shell, never serve a repo file" + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/tests/http_api.rs` around lines 141 - 143, Strengthen the traversal test around call by inspecting the response body rather than accepting either status code. Assert that the response does not contain the contents of Cargo.toml, ensuring a traversal request cannot serve the repository file while preserving the existing request setup.crates/taskito-server/tests/backends.rs (1)
30-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
namespacetotask_name.The returned string is used as the job
task_nameand as a settings key suffix.new_jobsetsnamespace: None, so no namespace is involved. The current name and the module doc at Lines 11-12 describe a mechanism the test does not use.♻️ Proposed rename
-/// Open the backend named by `variable`, or `None` when it is not configured. +/// Open the backend named by `variable`, or `None` when it is not configured. +/// The second tuple element is a per-run task name used to isolate rows. fn backend_state(variable: &str) -> Option<(SharedState, String)> { let dsn = std::env::var(variable).ok().filter(|dsn| !dsn.is_empty())?; let opened = backend::open(&dsn, None).unwrap_or_else(|error| { // The DSN is not interpolated — it carries credentials. panic!("{variable} is set but the backend could not be opened: {error}") }); // A per-run task name keeps assertions exact against a shared database. - let namespace = format!("server-test-{}-{}", std::process::id(), now_millis()); + let task_name = format!("server-test-{}-{}", std::process::id(), now_millis()); let state = dashboard_state_for( opened.storage, opened.workflows, AuthMode::Open, StaticAssets::new(None), ); - Some((state, namespace)) + Some((state, task_name)) }Update both call sites at Lines 150 and 160 accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/tests/backends.rs` around lines 30 - 46, Rename the local `namespace` in `backend_state` to `task_name`, update both returned-value call sites around `new_job` accordingly, and revise the module documentation to describe task names rather than namespaces. Preserve the existing generated value and settings-key behavior.crates/taskito-server/tests/support/webhook_receiver.rs (1)
27-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNormalise the requested header name in
Received::header.
receivestores header names in lowercase (line 114).headercompares the caller'snameverbatim. A caller that passes"Content-Type"getsNone. That makes a negative assertion such asassert!(delivery.header("X-Taskito-Signature").is_none())pass without testing anything. Current callers use lowercase names, so behaviour is correct today.♻️ Proposed change
/// One header's value. pub fn header(&self, name: &str) -> Option<&str> { + let name = name.to_ascii_lowercase(); self.headers .iter() - .find(|(key, _)| key == name) + .find(|(key, _)| *key == name) .map(|(_, value)| value.as_str()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/tests/support/webhook_receiver.rs` around lines 27 - 35, Update Received::header to normalize the requested name to lowercase before comparing it with the lowercase keys stored by receive, while preserving the existing Option<&str> return behavior.crates/taskito-server/tests/support/oidc_issuer.rs (1)
178-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn an error response instead of panicking on an unarmed exchange.
Line 187 panics inside the spawned server task. The test task does not observe that panic message. The client sees a dropped connection or a 500, so the flow reports a generic
oauth_failed, and the real cause stays hidden.crates/taskito-server/tests/oidc_login.rsline 290 depends on unarmed exchanges never happening, so this path only runs when a test is written incorrectly — which is exactly when a readable failure matters.Return an explicit body that the test can recognise, and keep the exchange counter accurate.
♻️ Proposed change to fail readably
-async fn token(State(issuer): State<Arc<Issuer>>) -> Json<Value> { +async fn token(State(issuer): State<Arc<Issuer>>) -> (StatusCode, Json<Value>) { issuer .exchanges .fetch_add(1, std::sync::atomic::Ordering::SeqCst); - let armed = issuer - .next - .lock() - .expect("stub lock") - .take() - .expect("a test armed the stub before triggering the exchange"); + let Some(armed) = issuer.next.lock().expect("stub lock").take() else { + // A test reached the exchange without arming it. Say so on the wire. + return ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(json!({ "error": "stub_not_armed" })), + ); + };Add the import and wrap the success return:
use axum::http::StatusCode; // ... at the end of `token`: ( StatusCode::OK, Json(json!({ "access_token": "stub-access-token", "token_type": "Bearer", "expires_in": 3_600, "id_token": id_token, })), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/tests/support/oidc_issuer.rs` around lines 178 - 192, Update token so an unarmed issuer.next exchange returns an explicit HTTP error response with a recognizable JSON body instead of panicking on expect("a test armed..."). Keep the exchanges counter incremented before this check, and preserve the existing signed-token success response for armed exchanges; import and use StatusCode as needed to unify both response paths.crates/taskito-server/src/config/listen.rs (1)
73-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider keeping the loopback guard with the parser.
parseis public and returns aTcpvariant with no address policy applied. Onlyfrom_envenforces the loopback rule. A future caller that usesparsedirectly obtains an unguarded non-loopback listener address. Moving the check into aAttachListen::ensure_localmethod, or makingparseprivate, keeps the guard attached to the value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/config/listen.rs` around lines 73 - 86, Ensure the loopback-only policy is enforced within the public AttachListen parsing flow rather than only in from_env. Update parse or introduce and invoke an AttachListen::ensure_local method so every returned Tcp address is validated, while preserving Unix socket handling and existing error behavior.crates/taskito-server/src/config/mod.rs (1)
87-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReject an unrecognised boolean instead of falling back to the default.
flagaccepts any value and silently usesdefault.TASKITO_MAINTENANCE=oftherefore keeps retention enabled on every replica, which is the case the PR objectives want operators to be able to switch off. Every other parser in this module fails loudly on an invalid value.Return a
Resultand report the key and the value.♻️ Proposed refactor
-pub fn flag(env: &Env, key: &str, default: bool) -> bool { +pub fn flag(env: &Env, key: &str, default: bool) -> Result<bool> { match value(env, key) { - None => default, + None => Ok(default), Some(raw) => match raw.to_ascii_lowercase().as_str() { - "1" | "true" | "on" | "yes" => true, - "0" | "false" | "off" | "no" => false, - _ => default, + "1" | "true" | "on" | "yes" => Ok(true), + "0" | "false" | "off" | "no" => Ok(false), + other => bail!("{key} must be on or off, got '{other}'"), }, } }Update the two call sites at lines 54 and 65 of this file and the
secure_cookiescall site incrates/taskito-server/src/config/dashboard.rsto propagate with?.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/config/mod.rs` around lines 87 - 96, Change config::flag to return a Result<bool, ...>, preserving the default only when the environment key is absent and returning an error containing both key and raw value for unrecognized inputs. Update its two call sites in config/mod.rs and the secure_cookies call site in config/dashboard.rs to propagate the parsing error with ?, allowing invalid boolean configuration to fail loudly.crates/taskito-server/src/main.rs (1)
10-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the remaining dashboard variables in
ENV_HELP.
ENV_HELPis the only reference for a binary that takes no flags.crates/taskito-server/src/config/dashboard.rsalso readsTASKITO_DASHBOARD_ADMIN_USER,TASKITO_DASHBOARD_ADMIN_PASSWORD,TASKITO_DASHBOARD_INSECURE_COOKIES, and the OAuth variables. An operator who enablesTASKITO_DASHBOARD_AUTH=sessionneeds the first two to create the first account.📝 Proposed addition
TASKITO_DASHBOARD_AUTH off | session (default: off) + TASKITO_DASHBOARD_ADMIN_USER first admin username, created on start (session auth) + TASKITO_DASHBOARD_ADMIN_PASSWORD first admin password; read once, then scrubbed + TASKITO_DASHBOARD_INSECURE_COOKIES 1 to drop `Secure` from session cookies (plain HTTP) TASKITO_DASHBOARD_ASSETS serve the SPA from this directory🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/main.rs` around lines 10 - 26, The ENV_HELP constant is missing dashboard authentication and OAuth configuration variables. Update ENV_HELP to document TASKITO_DASHBOARD_ADMIN_USER, TASKITO_DASHBOARD_ADMIN_PASSWORD, TASKITO_DASHBOARD_INSECURE_COOKIES, and all OAuth variables read by the dashboard configuration, including the required first-account credentials for session authentication.crates/taskito-server/src/dashboard/routes/queues.rs (1)
91-139: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCombine the three
on_storagecalls inscalerinto one.
scalerissues three separateon_storagecalls (Lines 92, 93, 94) wherelist_queuesinoverrides.rsshows the established pattern of combining several storage reads into a single closure. Since/api/scaleris polled periodically by the autoscaler, combining these into oneon_storagecall reduces the number of blocking-pool round trips per poll.♻️ Proposed combination
- let overall = on_storage(&state, |storage| storage.stats()).await?; - let workers = on_storage(&state, |storage| storage.list_workers()).await?; - let per_queue = on_storage(&state, |storage| storage.stats_all_queues()).await?; + let (overall, workers, per_queue) = on_storage(&state, |storage| { + Ok(( + storage.stats()?, + storage.list_workers()?, + storage.stats_all_queues()?, + )) + }) + .await?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/routes/queues.rs` around lines 91 - 139, Update scaler to combine the stats, worker listing, and per-queue stats reads into a single on_storage closure, returning all three results together and preserving the existing error propagation and downstream calculations. Follow the established multi-read pattern used by list_queues in overrides.rs.crates/taskito-server/src/dashboard/auth/middleware.rs (1)
79-83: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the setup-complete state instead of counting users on every request.
Line 81 runs
store::count_userson the blocking pool for every non-public/api/request. Together with the session lookup inresolve_context, each dashboard API call makes two storage round trips. The count only matters while it is zero, and it does not return to zero after setup completes.Latch the result in
SharedStateafter the first non-zero count, then skip the query.⚡ Proposed latch
- if is_api && !gate::is_public_path(path) && on_storage(state, store::count_users).await? == 0 { - return Err(ApiError::SetupRequired); + if is_api && !gate::is_public_path(path) && !setup_complete(state).await? { + return Err(ApiError::SetupRequired); }Add the helper, backed by an
AtomicBoolonSharedState:/// Once a user exists the count cannot return to zero, so the answer is /// latched and the per-request query disappears. async fn setup_complete(state: &SharedState) -> Result<bool, ApiError> { if state.setup_complete.load(std::sync::atomic::Ordering::Relaxed) { return Ok(true); } let complete = on_storage(state, store::count_users).await? > 0; if complete { state .setup_complete .store(true, std::sync::atomic::Ordering::Relaxed); } Ok(complete) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/auth/middleware.rs` around lines 79 - 83, Add an AtomicBool setup-complete latch to SharedState and implement a setup_complete helper that returns immediately when latched, otherwise checks store::count_users through on_storage and stores true after the first non-zero result. Update the middleware condition around gate::is_public_path to call setup_complete instead of querying store::count_users directly, preserving setup_required behavior while avoiding repeated storage calls.crates/taskito-server/src/dashboard/static_assets.rs (2)
83-97: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
available()re-readsindex.htmlon every request in directory mode.
available()callsread("index.html")and discards the bytes.serve_spaincrates/taskito-server/src/dashboard/mod.rscallsavailable()on every non-API request, then callsresolve, and for a client-routed path it then callsindex(). In directory mode that is two or three full synchronousstd::fs::readcalls per request, and they run on the async runtime thread.Two changes fix both points:
- Give
available()a metadata check instead of a read, or cache the result inStaticAssets::new.- Have
serve_sparely onresolveandindexreturningNoneinstead of probing withavailable()first.The embedded source is unaffected in cost terms, because
EMBEDDED_ASSETSis a small in-memory table.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/static_assets.rs` around lines 83 - 97, Avoid redundant asset reads in the SPA request flow: change StaticAssets::available to use metadata or a cached result rather than reading index.html, and update serve_spa to rely on resolve and index returning None instead of calling available before them. Preserve embedded and directory asset resolution behavior while eliminating the repeated synchronous reads.
178-194: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the temporary directory unique and clean it up on failure.
The directory name uses only
std::process::id(). Every test in this binary shares that pid, so a second test that adopts the same pattern collides, and tests run concurrently as threads. Theremove_dir_allon Line 193 also never runs if an earlier assertion panics, so the directory leaks.Use the
tempfilecrate, which gives a unique path and removes it on drop. If you keep the manual approach, add a unique suffix such as a counter or arandom_token()fragment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/static_assets.rs` around lines 178 - 194, Update the test a_directory_source_serves_files_and_rejects_escapes to create its temporary directory through the tempfile crate, ensuring uniqueness across concurrent tests and automatic cleanup when assertions fail; remove the manual process-ID path construction and explicit remove_dir_all cleanup.crates/taskito-server/src/dashboard/security.rs (2)
11-21: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueTwo optional hardening steps for the header set.
The set is correct as written.
x-frame-optionsisDENYandframe-ancestors 'none'agrees with it.Two refinements:
strict-transport-securityis absent. If the deployment terminates TLS at this process, add it whenconfig.secure_cookiesis true. If a proxy terminates TLS, the proxy normally sets it, and no change is needed.'unsafe-inline'instyle-srcalso permits inline<style>elements, not only the inline style attributes named in the comment.style-src 'self'; style-src-attr 'unsafe-inline'grants only what the SPA needs. Verify the SPA renders correctly before you narrow it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/security.rs` around lines 11 - 21, Update the SECURITY_HEADERS configuration to conditionally add Strict-Transport-Security only when config.secure_cookies is true and this process terminates TLS; otherwise leave proxy-terminated deployments unchanged. Also verify the SPA before narrowing the CSP style policy from 'unsafe-inline' in style-src to self with style-src-attr 'unsafe-inline', preserving rendering behavior.
50-62: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse
subtle::ConstantTimeEqfor secret comparisons.This function is used for metrics-token authentication and password verification. The hand-written fold is not protected against compiler transformations that can introduce data-dependent control flow. Add
subtleas a direct dependency and useleft.ct_eq(right)after the length check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/security.rs` around lines 50 - 62, Update constant_time_eq to use subtle::ConstantTimeEq: add subtle as a direct dependency, import the trait, and replace the hand-written byte fold with left.ct_eq(right) after the existing length check, converting the result to bool while preserving the current mismatch behavior.crates/taskito-server/src/dashboard/probes.rs (1)
135-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMatch the
Bearerscheme case-insensitively.RFC 7235 defines the authentication scheme token as case-insensitive. Line 145 compares the whole header value against
Bearer {token}, so a scraper that sendsbearer <token>orBEARER <token>is rejected. Split the scheme from the credential, compare the scheme witheq_ignore_ascii_case, and keepconstant_time_eqfor the credential.The fail-closed interaction is correct: when
metrics_tokenis set, open mode no longer exempts the probe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/probes.rs` around lines 135 - 155, Update require_probe_access to parse the authorization header into its scheme and credential, compare the scheme with eq_ignore_ascii_case against Bearer, and continue using constant_time_eq for the credential token. Preserve the existing fail-closed behavior when metrics_token is configured, including the authenticated-user fallback.crates/taskito-server/src/dashboard/mod.rs (2)
81-97: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid copying every asset body on each request.
bytes.into_owned()allocates and copies the whole file for every request, including theCow::Borrowedcase that the embedded source always returns. The bundle is about 1.6 MB per the note incrates/taskito-server/src/dashboard/static_assets.rs, so each SPA load copies the served files needlessly.Convert the borrowed case to
axum::body::Bytes::from_staticand keep the owned case as is. That makes the embedded path a zero-copy clone of a reference-counted buffer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/mod.rs` around lines 81 - 97, Update the asset response in the resolve(path) block to convert Cow::Borrowed data with axum::body::Bytes::from_static and preserve owned data without copying unnecessarily. Replace the unconditional bytes.into_owned() conversion while keeping the existing headers and response behavior unchanged.
46-61: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftBound graceful shutdown and add a request timeout.
Shutdown::wait()only starts the Axum drain. The server then waits for open connections without a deadline. Bound the completeaxum::serve(...).with_graceful_shutdown(...)future, not onlyshutdown.wait().The runtime uses
spawn_blockingfor storage. CallRuntime::shutdown_timeoutduring teardown because Tokio cannot abort started blocking tasks. Addtower_http::timeout::TimeoutLayer; its default response is408 Request Timeout, and it does not cancel an underlying blocking task.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/mod.rs` around lines 46 - 61, Update serve to wrap the complete axum::serve(...).with_graceful_shutdown(...) future in a shutdown deadline, rather than timing only shutdown.wait(); during runtime teardown, invoke Tokio Runtime::shutdown_timeout so started spawn_blocking storage tasks receive a bounded shutdown. Add tower_http::timeout::TimeoutLayer to the router or service stack with the intended request deadline, preserving the default 408 response behavior.crates/taskito-server/src/dashboard/dto.rs (1)
240-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the empty-error branch.
Lines 167-169 return
Some(String::new())for an empty stored error, which is distinct fromNone. No test pins that behavior. The SPA distinguishes""fromnull, so a regression here changes a rendered cell.💚 Proposed test
#[test] fn no_error_stays_absent() { assert_eq!(summarize_error(None), None); } + + #[test] + fn an_empty_error_stays_an_empty_string() { + assert_eq!(summarize_error(Some("")).as_deref(), Some("")); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/dashboard/dto.rs` around lines 240 - 244, Add a test alongside no_error_stays_absent for summarize_error that passes an empty stored error and asserts the result is Some(String::new()), preserving the distinction between an empty string and None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a3a56cc-8372-4ea6-bdbf-90388d0ed6b2
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcrates/taskito-server/tests/fixtures/oidc_test_key.pemis excluded by!**/*.pem
📒 Files selected for processing (92)
Cargo.tomlcrates/taskito-server/Cargo.tomlcrates/taskito-server/README.mdcrates/taskito-server/build.rscrates/taskito-server/src/config/backend.rscrates/taskito-server/src/config/dashboard.rscrates/taskito-server/src/config/listen.rscrates/taskito-server/src/config/mod.rscrates/taskito-server/src/dashboard/auth/bootstrap.rscrates/taskito-server/src/dashboard/auth/context.rscrates/taskito-server/src/dashboard/auth/cookies.rscrates/taskito-server/src/dashboard/auth/gate.rscrates/taskito-server/src/dashboard/auth/middleware.rscrates/taskito-server/src/dashboard/auth/mod.rscrates/taskito-server/src/dashboard/auth/model.rscrates/taskito-server/src/dashboard/auth/oauth/config.rscrates/taskito-server/src/dashboard/auth/oauth/mod.rscrates/taskito-server/src/dashboard/auth/oauth/providers/github.rscrates/taskito-server/src/dashboard/auth/oauth/providers/mod.rscrates/taskito-server/src/dashboard/auth/oauth/providers/oidc.rscrates/taskito-server/src/dashboard/auth/oauth/state.rscrates/taskito-server/src/dashboard/auth/password.rscrates/taskito-server/src/dashboard/auth/routes.rscrates/taskito-server/src/dashboard/auth/store.rscrates/taskito-server/src/dashboard/auth/throttle.rscrates/taskito-server/src/dashboard/blocking.rscrates/taskito-server/src/dashboard/dto.rscrates/taskito-server/src/dashboard/error.rscrates/taskito-server/src/dashboard/mod.rscrates/taskito-server/src/dashboard/probes.rscrates/taskito-server/src/dashboard/query.rscrates/taskito-server/src/dashboard/routes/dead_letters.rscrates/taskito-server/src/dashboard/routes/executors.rscrates/taskito-server/src/dashboard/routes/jobs.rscrates/taskito-server/src/dashboard/routes/logs.rscrates/taskito-server/src/dashboard/routes/metrics.rscrates/taskito-server/src/dashboard/routes/middleware.rscrates/taskito-server/src/dashboard/routes/mod.rscrates/taskito-server/src/dashboard/routes/overrides.rscrates/taskito-server/src/dashboard/routes/queues.rscrates/taskito-server/src/dashboard/routes/resources.rscrates/taskito-server/src/dashboard/routes/retention.rscrates/taskito-server/src/dashboard/routes/settings.rscrates/taskito-server/src/dashboard/routes/topics.rscrates/taskito-server/src/dashboard/routes/webhooks.rscrates/taskito-server/src/dashboard/routes/workflows.rscrates/taskito-server/src/dashboard/security.rscrates/taskito-server/src/dashboard/state.rscrates/taskito-server/src/dashboard/static_assets.rscrates/taskito-server/src/dashboard/stores/deliveries.rscrates/taskito-server/src/dashboard/stores/kv.rscrates/taskito-server/src/dashboard/stores/middleware.rscrates/taskito-server/src/dashboard/stores/mod.rscrates/taskito-server/src/dashboard/stores/overrides.rscrates/taskito-server/src/dashboard/stores/url_safety.rscrates/taskito-server/src/dashboard/stores/webhooks.rscrates/taskito-server/src/dashboard/webhook_sender.rscrates/taskito-server/src/lib.rscrates/taskito-server/src/main.rscrates/taskito-server/src/runtime/listener.rscrates/taskito-server/src/runtime/mod.rscrates/taskito-server/src/runtime/scheduler.rscrates/taskito-server/src/runtime/shutdown.rscrates/taskito-server/src/runtime/upkeep.rscrates/taskito-server/tests/attach_e2e.rscrates/taskito-server/tests/auth_flow.rscrates/taskito-server/tests/backends.rscrates/taskito-server/tests/fixtures/oidc_test_jwks.jsoncrates/taskito-server/tests/github_login.rscrates/taskito-server/tests/http_api.rscrates/taskito-server/tests/oauth_flow.rscrates/taskito-server/tests/oidc_login.rscrates/taskito-server/tests/support/github_stub.rscrates/taskito-server/tests/support/mod.rscrates/taskito-server/tests/support/oidc_issuer.rscrates/taskito-server/tests/support/webhook_receiver.rscrates/taskito-server/tests/webhook_delivery.rsdashboard/src/components/layout/command-palette.tsxdashboard/src/components/layout/mobile-menu.tsxdashboard/src/components/layout/nav-config.test.tsdashboard/src/components/layout/nav-config.tsdashboard/src/components/layout/sidebar.tsxdashboard/src/features/executors/api.tsdashboard/src/features/executors/components/executors-table.tsxdashboard/src/features/executors/components/index.tsdashboard/src/features/executors/hooks.tsdashboard/src/features/executors/index.tsdashboard/src/features/executors/utils.test.tsdashboard/src/features/executors/utils.tsdashboard/src/lib/api-types.tsdashboard/src/routes/executors.tsxsdks/python/tests/dashboard/test_rust_server_parity.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ByteVeda/taskito(manual)
A user row this build cannot parse was dropped from the listing, which shrank count_users, reported setup_required, and reopened unauthenticated setup. Roles now degrade to viewer instead of failing to deserialize, and the count reads stored rows. An OAuth callback carried nothing tying it to the browser that began the flow, so an attacker holding a valid code could have a victim's browser finish it and land in the attacker's account. A short-lived HttpOnly SameSite=Lax marker is now required, checked before the state row is read so a forged callback cannot cancel a pending login either. A cached JWKS never expired, so every login failed after the issuer rotated keys until the process restarted; a kid miss now refetches once. Readiness answers 503 when degraded, since orchestrators read the status and not the body. A password change revokes the user's other sessions. The webhook response read is bounded rather than buffered whole, and the readiness error detail is one capped line rather than a driver string that can carry a DSN.
The guard split the authority by hand while reqwest parsed it properly, so the two could disagree about where the host ends — and a check that inspects one host while the request dials another is not a check. Both now use url::Url.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/taskito-server/tests/oidc_login.rs (1)
400-415: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the no-op
upsert_provider_usercall.
store::upsert_provider_user(backend, SLOT, "provider-subject-1", None, None, Role::Admin)has no effect on the role here: the user already exists from the earlier login, soupsert_provider_userignoresrole_on_createand only refreshesdisplay_name, bothNonein this call. The actual promotion happens through the following manualuser.role = Role::Admin; store::replace_user(...). Drop theupsert_provider_usercall, since it implies a promotion path that this test does not exercise.♻️ Suggested cleanup
- let username = format!("{SLOT}:provider-subject-1"); - store::upsert_provider_user(backend, SLOT, "provider-subject-1", None, None, Role::Admin) - .expect("storage"); - let mut users = store::list_users(backend).expect("storage"); + let username = format!("{SLOT}:provider-subject-1"); + let mut users = store::list_users(backend).expect("storage"); let user = users.get_mut(&username).expect("the user exists"); user.role = Role::Admin; store::replace_user(backend, user).expect("persist the promotion");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/tests/oidc_login.rs` around lines 400 - 415, Remove the no-op store::upsert_provider_user call from the promotion setup in the OIDC login test. Keep the existing list_users lookup, user.role = Role::Admin assignment, store::replace_user persistence, and final role assertion unchanged so the test exercises only manual promotion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/taskito-server/tests/oidc_login.rs`:
- Around line 400-415: Remove the no-op store::upsert_provider_user call from
the promotion setup in the OIDC login test. Keep the existing list_users lookup,
user.role = Role::Admin assignment, store::replace_user persistence, and final
role assertion unchanged so the test exercises only manual promotion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04496be2-626e-4068-8fe8-0085d3b53754
⛔ Files ignored due to path filters (1)
crates/taskito-server/tests/fixtures/oidc_test_key_rotated.pemis excluded by!**/*.pem
📒 Files selected for processing (22)
crates/taskito-server/src/config/dashboard.rscrates/taskito-server/src/dashboard/auth/context.rscrates/taskito-server/src/dashboard/auth/cookies.rscrates/taskito-server/src/dashboard/auth/model.rscrates/taskito-server/src/dashboard/auth/oauth/mod.rscrates/taskito-server/src/dashboard/auth/oauth/providers/mod.rscrates/taskito-server/src/dashboard/auth/oauth/providers/oidc.rscrates/taskito-server/src/dashboard/auth/routes.rscrates/taskito-server/src/dashboard/auth/store.rscrates/taskito-server/src/dashboard/probes.rscrates/taskito-server/src/dashboard/routes/webhooks.rscrates/taskito-server/src/dashboard/routes/workflows.rscrates/taskito-server/src/dashboard/stores/url_safety.rscrates/taskito-server/src/dashboard/webhook_sender.rscrates/taskito-server/tests/fixtures/oidc_test_jwks_rotated.jsoncrates/taskito-server/tests/github_login.rscrates/taskito-server/tests/oauth_flow.rscrates/taskito-server/tests/oidc_login.rscrates/taskito-server/tests/support/mod.rscrates/taskito-server/tests/support/oidc_issuer.rscrates/taskito-server/tests/webhook_delivery.rssdks/python/tests/dashboard/test_rust_server_parity.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ByteVeda/taskito(manual)
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/taskito-server/tests/webhook_delivery.rs
- crates/taskito-server/tests/github_login.rs
- crates/taskito-server/tests/oauth_flow.rs
- crates/taskito-server/src/dashboard/auth/oauth/providers/mod.rs
- crates/taskito-server/src/config/dashboard.rs
- sdks/python/tests/dashboard/test_rust_server_parity.py
- crates/taskito-server/src/dashboard/auth/oauth/mod.rs
- crates/taskito-server/src/dashboard/routes/workflows.rs
- crates/taskito-server/src/dashboard/routes/webhooks.rs
- crates/taskito-server/src/dashboard/auth/routes.rs
- crates/taskito-server/src/dashboard/auth/context.rs
- crates/taskito-server/src/dashboard/auth/model.rs
2 participants
Closes #549. Phase S3 of executor attach (#546), on top of S2 (#547) and S1 (#548).
Adds
crates/taskito-server— the scheduler, the executor attach listener, and the dashboard in one binary with no language runtime. Task bodies stay in the app's container; executors dial in over the worker frame protocol. Configuration is environment-only.Decisions worth reviewing
The scheduler starts on the first attach, not at boot. With nothing attached the poller would claim jobs no executor advertises and fail each one retryably once
placement_timeoutelapsed — a retry storm against an idle deployment.SchedulerSupervisor::ensure_startedis idempotent and the listener calls it after every successful handshake.The dashboard is a full port, not a subset. One SPA build is served by every implementation, so a missing route is a broken page rather than a smaller feature set: ~60 routes, session auth (PBKDF2/CSRF/RBAC), and Google/GitHub/OIDC login. Auth and feature-store keys are byte-identical to the SDK dashboards (
auth:users,auth:session:*,webhooks:subscriptions,overrides:task:*, …), so a session minted by one is accepted by another against the same backend — asserted intests/auth_flow.rs.Routes that need a language runtime degrade honestly rather than pretending:
/api/proxy-stats→[],/api/interception-stats→{},/api/resourcesreconstructed from worker heartbeats,/api/middlewarelimited to the disable list./metricspublishes storage-derived gauges instead of proxying an in-process Prometheus registry.Security defaults fail closed. A non-loopback
TASKITO_LISTENrefuses to start — the handshake carries no credential until S4 — and a setTASKITO_ATTACH_TOKENis an error rather than a silently ignored control. An off-host dashboard withAUTH=offneedsTASKITO_ALLOW_INSECURE=1. Webhook URLs are SSRF-checked on write and immediately before each send. Failed logins are throttled: ten per identity per five minutes, 429 withRetry-After, cleared on success.Open questions from the plan, settled here
GET /api/executors, plus a page that hides itself when the route 404s, so the shared SPA still works on the SDK dashboards.TASKITO_MAINTENANCE=offdisables retention so an attach-listener replica can scale independently; dead-worker reaping stays on because in-flight recovery depends on it.Verification
cargo fmt --check,clippy --all-targets --all-features,cargo test --workspace;--features postgresand--features redisboth build and test.id_tokens are each refused, and every guard was mutation-tested — removing the audience, issuer, expiry, nonce, orkidcheck flips its case to a failure. That work found one real weakness, fixed here: a token naming an unknownkidused to fall back to the only published key.sdks/python/tests/dashboard/test_rust_server_parity.pyserves one seeded database from both dashboards and diffs every route the SPA calls: 19 byte-identical, 3 after dropping clock-derived fields, 6 shape-only. Skipped unless the server binary is built.Not in this PR
Attach credentials and TLS (S4), the per-SDK
executorsubcommands (S5–S7), the image (S8), the deployment docs (S9), and the Helm chart (S10). Until S4 lands the listener is loopback-only.Summary by CodeRabbit