NOT READY : Freeform onboarding#68
Conversation
…; drop requestable from usage policy
alchezar
changed the title
wanguardd
left a comment
wanguardd
left a comment
There was a problem hiding this comment.
System of Review completed analysis of 'Freeform onboarding'. Found 15 findings across 8 files.
📋 Review Summary
| Category | 🔴 Blocking | 🟠🟡 Non-Blocking | Total |
|---|---|---|---|
| 🏛️ ORGANIZATIONAL | 1 | 1 | 2 |
| 🐛 CORRECTNESS | 3 | 1 | 4 |
| 🛡️ SECURITY | 2 | 0 | 2 |
| 📋 REQUIREMENTS | 1 | 1 | 2 |
| 📐 DESIGN | 0 | 1 | 1 |
| 🔍 QUALITY | 0 | 1 | 1 |
| ✅ TESTING | 0 | 2 | 2 |
| 📖 DOCUMENTATION | 0 | 1 | 1 |
| Total | 7 | 8 | 15 |
🏛️ ORGANIZATIONAL
🔴 Blocking · module/iron_control_api/src/freeform/ missing readme.md
A new permanent directory was added with no readme.md. The File Creation Protocol requires every permanent directory to have a readme.md with a Responsibility Table documenting its files.
Evidence:
ls module/iron_control_api/src/freeform/
company_setup.rs invites.rs mod.rs providers.rs usage_policy.rs
# no readme.md present
Action:
- Create
module/iron_control_api/src/freeform/readme.mdwith a Responsibility Table listing all five files and their single-sentence responsibilities
🟠 Important · src/routes/readme.md not updated for three new route files
freeform.rs, invites.rs, and workspace.rs were added to src/routes/ but not registered in the directory's Responsibility Table. The readme now has a stale inventory.
Evidence:
# routes/readme.md Responsibility Table contains:
agent_provider_key.rs, agents.rs, analytics/, auth/, budget/, health.rs,
ic_token.rs, keys.rs, limits.rs, mod.rs, providers.rs, tokens/, usage.rs,
users.rs, version.rs
# freeform.rs / invites.rs / workspace.rs — absent
Action:
- Add rows for
freeform.rs,invites.rs, andworkspace.rsto the Responsibility Table inmodule/iron_control_api/src/routes/readme.md
🐛 CORRECTNESS
🔴 Blocking · Race condition in post_invite_accept — TOCTOU on seat limit
The seat availability check and seat counter increment are three separate un-transacted statements. Under concurrent requests for the same invite link, two threads can both read seats_used < seats_total, both proceed through user creation and seat insertion, and both increment the counter — resulting in more accepted seats than the link allows.
Evidence:
// module/iron_control_api/src/routes/invites.rs:295–356
// No BEGIN / COMMIT wraps this entire sequence:
SELECT seats_total, seats_used FROM invite_links WHERE token_hash = ? // line 295
if seats_used >= seats_total { return Err(BadRequest) } // line 314
INSERT INTO users ... // line 320
INSERT INTO invite_seats ... // line 333
UPDATE invite_links SET seats_used = seats_used + 1 WHERE id = ? // line 356Action:
- Wrap lines 295–356 of
post_invite_acceptin a single SQLiteBEGIN IMMEDIATEtransaction so the check, user creation, seat record, and counter update are atomic
🔴 Blocking · Approval bypass — JWT issued before approved_at is set
post_invite_accept returns a fully usable JWT access token immediately after the user row and seat record are created. The approved_at column on invite_seats is only set later by a separate admin approve endpoint. A member who accepts any invite link gets immediate, unrestricted system access without admin approval.
Evidence:
// module/iron_control_api/src/routes/invites.rs:365–374
let access_token = state
.jwt_secret
.generate_access_token(&user_id, &user_email, "developer", &token_jti)?;
Ok((StatusCode::CREATED, Json(AcceptInviteResponse { user_id, access_token })))
// invite_seats.approved_at is NULL at this point — never checkedAction:
- On accept, return a pending-state response (e.g.,
{ user_id, status: "pending_approval" }) instead of a JWT. Issue the full JWT only after an admin calls the approve endpoint, which setsapproved_at
🟠 Important · Wrong RBAC permission on POST /workspace/budget
post_workspace_budget guards itself with Permission::ManageUsers — the permission for user account management — instead of Permission::ManageIcTokens, which governs IC token budget controls. A user who holds ManageUsers but not ManageIcTokens can set the workspace spending policy.
Evidence:
// module/iron_control_api/src/routes/workspace.rs:90
check_permission(&claims.role, Permission::ManageUsers)?;
// Should be: Permission::ManageIcTokensAction:
- Change
Permission::ManageUserstoPermission::ManageIcTokenson line 90 ofworkspace.rs
🔴 Blocking · test_preview_invite_ok fails in local and CI test runs
Migration 034 seeds workspace id=1 with name = 'Workspace' via INSERT OR IGNORE. The test creates an invite expecting a workspace named 'Test Corp', but reads back 'Workspace' from the pre-existing seed row. The test fails with an assertion panic.
Evidence:
FAIL [0.239s] iron_control_api::invites_api_test test_preview_invite_ok
thread panicked at tests/invites_api_test.rs:255:
assertion `left == right` failed
left: String("Workspace")
right: "Test Corp"
# Root cause: migration 034_seed_default_workspace.sql line 7:
INSERT OR IGNORE INTO workspaces (id, name, ...) VALUES (1, 'Workspace', 'example.com', ...)
# Seed row is never overwritten before the test reads it back
Action:
- After applying migrations in
setup_test_db(), executeDELETE FROM workspaces(orINSERT OR REPLACE) before each test inserts its own workspace fixture, so tests are isolated from the seed data
🛡️ SECURITY
🔴 Blocking · No rate limiting on bcrypt-heavy POST /invites/{token}/accept
post_invite_accept performs a bcrypt hash at BCRYPT_COST on every request — a deliberately expensive CPU operation. The endpoint requires no authentication. An attacker can flood it with requests and saturate server CPU, causing denial of service.
Evidence:
// module/iron_control_api/src/routes/invites.rs:322 — no middleware guard
let password_hash = bcrypt::hash(&body.password, BCRYPT_COST)?;
// iron_control_api_server.rs:820 — route registered with no rate-limit layer
.route("/api/v1/invites/{token}/accept", post(routes::invites::post_invite_accept))Action:
- Apply a per-IP (or per-token) rate-limiter middleware to
/invites/{token}/accept; or wrap the bcrypt call in a Semaphore to cap concurrent CPU-intensive operations
🔴 Blocking · Setup routes accessible to any authenticated user — admin guard missing
The four /setup/* routes (/setup/company, /setup/providers, /setup/budget, /setup/users) have meta: { requiresAuth: true } only. The navigation guard checks only isAuthenticated — it has no requiresAdmin branch. Any logged-in developer or member can navigate directly to admin-only setup pages by entering the URL.
Evidence:
// module/iron_dashboard/src/router/index.ts:61–82
{ path: '/setup/company', meta: { requiresAuth: true } }, // no requiresAdmin
{ path: '/setup/providers', meta: { requiresAuth: true } },
{ path: '/setup/budget', meta: { requiresAuth: true } },
{ path: '/setup/users', meta: { requiresAuth: true } },
// Navigation guard at line 96 — no admin check:
if (requiresAuth && !authStore.isAuthenticated) { next('/login') }
// else: next() — any authenticated user proceedsAction:
- Add
requiresAdmin: trueto themetaof all four setup routes; add a check in thebeforeEachguard to redirect non-admins to/dashboardwhento.meta.requiresAdmin && !authStore.isAdmin
📋 REQUIREMENTS
🔴 Blocking · POST /api/v1/freeform/providers silently discards email addresses from mixed paste
Task 029 Slide 10–11 specifies that the Providers FreeForm paste handles both provider keys (provider: key lines) and team invite emails (bare email addresses) in a single block. post_providers only calls providers::parse() — the freeform::invites module exists and is unit-tested but has no HTTP route, and email addresses in the paste are silently discarded.
Evidence:
// module/iron_control_api/src/routes/freeform.rs:204
let entries = providers::parse(&body.text)?; // only provider keys
// invites::parse() never called
// iron_control_api_server.rs:796–804 — no /freeform/invites route:
.route("/api/v1/freeform/company", post(routes::freeform::post_company))
.route("/api/v1/freeform/providers", post(routes::freeform::post_providers))
// /api/v1/freeform/invites — absent
// task/029_freeform_onboarding.md slide 10 paste block:
// gemini: AIzaSy... openai: sk-proj-... alice@acmecorp.com bob@acmecorp.com
// slide 11: "✓ 10 team members (queued, not yet invited)"Action:
- Extend
post_providersto also callfreeform::invites::parse()on the same paste text, queue found email addresses as pending invite seats, and include aqueued_invitescount in the response; or create a dedicatedPOST /api/v1/freeform/inviteshandler
🟠 Important · requestable: policy directive not implemented in usage_policy.rs
Task 029 Slide 12–13 defines a requestable: directive that sets a workspace-wide list of models gated behind admin-approval requests. ParsedPolicy has only spending_cap and default_model; the parser has no branch for requestable:; and post_workspace_budget does not store or return requestable model data.
Evidence:
// module/iron_control_api/src/freeform/usage_policy.rs — ParsedPolicy struct:
pub struct ParsedPolicy {
pub spending_cap: Option<SpendingCap>,
pub default_model: Option<String>,
// no requestable_models field
}
// No match arm for "requestable" in parse()
// task/029_freeform_onboarding.md slide 12:
// "requestable: claude-4-6-sonnet, gemini-3.1-pro-preview"
// slide 13: "✓ Requestable — claude-4-6-sonnet, gemini-3.1-pro-preview"Action:
- Add
requestable_models: Vec<String>toParsedPolicy; add a"requestable"parse branch that splits on commas; persist the list (new DB column or separate table); return it fromGET /me/workspace
📐 DESIGN
🟠 Important · check_permission defined three times with identical implementation
The same check_permission function body is copy-pasted into freeform.rs, invites.rs, and workspace.rs. Any change to permission logic must be applied in three places.
Evidence:
// module/iron_control_api/src/routes/freeform.rs:88
fn check_permission(role_str: &str, permission: Permission) -> Result<(), ApiError> { ... }
// module/iron_control_api/src/routes/invites.rs:53 — identical
fn check_permission(role_str: &str, permission: Permission) -> Result<(), ApiError> { ... }
// module/iron_control_api/src/routes/workspace.rs:24 — identical
fn check_permission(role_str: &str, permission: Permission) -> Result<(), ApiError> { ... }Action:
- Move
check_permissiontomodule/iron_control_api/src/routes/mod.rs(or a dedicatedroutes::auth_helpersmodule), make itpub(super), and replace all three local definitions withuse super::check_permission
🔍 QUALITY
🟡 Minor · nextest.toml status-level = "slow" suppresses passing test output
With status-level = "slow", only tests that exceed the 30-second slow threshold emit output. Passing tests are invisible in both local runs and CI logs, making it harder to confirm which tests ran or to debug test isolation issues.
Evidence:
# .config/nextest.toml
[profile.default]
status-level = "slow" # only slow (>30s) or failing tests emit output
final-status-level = "fail"Action:
- Change
status-levelto"pass"or"fail"so test output is visible; if"slow"is intentional to reduce log noise, add a comment explaining the rationale
✅ TESTING
🟠 Important · test_kind: markers absent from all seven new test files
All seven test files added in this PR lack the //! test_kind: doc-comment header required by the project's test organization rulebook. Without markers, the test surface inventory cannot classify these tests.
Evidence:
# grep test_kind across all 7 new test files:
grep -r "test_kind" freeform_company_setup_test.rs freeform_invites_test.rs \
freeform_providers_test.rs freeform_usage_policy_test.rs freeform_api_test.rs \
invites_api_test.rs workspace_budget_test.rs
# 0 matches
Action:
- Add
//! test_kind: unitto the four parser unit tests (freeform_*_setup_test.rs,freeform_*_test.rs); add//! test_kind: integrationtofreeform_api_test.rs,invites_api_test.rs, andworkspace_budget_test.rs
🟠 Important · Seven new test files not registered in tests/readme.md
The tests/readme.md Responsibility Table and directory tree list no entry for any of the seven new test files. The test inventory is stale.
Evidence:
grep "freeform\|invites_api\|workspace_budget" module/iron_control_api/tests/readme.md
# 0 matches
# 7 new files: freeform_api_test.rs, freeform_company_setup_test.rs,
# freeform_invites_test.rs, freeform_providers_test.rs,
# freeform_usage_policy_test.rs, invites_api_test.rs, workspace_budget_test.rs
Action:
- Add rows for all seven new test files to the Responsibility Table in
module/iron_control_api/tests/readme.md, with a brief responsibility description for each
📖 DOCUMENTATION
🟠 Important · module/iron_dashboard/src/components/freeform/ missing readme.md
A new permanent Vue component directory was added with no readme.md. The File Creation Protocol requires every permanent directory to have a readme.md with a Responsibility Table.
Evidence:
ls module/iron_dashboard/src/components/freeform/
AutoSetupWizard.vue FreeFormDialog.vue FreeFormToggle.vue index.ts
# no readme.md
Action:
- Create
module/iron_dashboard/src/components/freeform/readme.mdwith a Responsibility Table for all four files
This is a solid effort on a complex feature. The parser layer is clean and well-tested in isolation. Let me know if anything needs clarification.
❗ > Note: This System of Review operates with the same information access and constraints as any human developer working on this repository. While the system is highly accurate (99%+ reliability), any confusion or apparent misunderstanding typically indicates gaps in repository documentation, unclear code organization, or insufficient project discipline — the same issues that would impact any team member. If the System cannot locate critical information and draws incorrect conclusions, a human developer would face identical challenges. We maintain high standards to respect our teammates' time, our clients' investment, and the integrity of this project. Please ensure the repository provides clear, discoverable context for all reviewers.
Review approval is a quality floor, not a quality ceiling. It confirms that identified issues are resolved — not that the code is free of all issues. The System reviews the changes in this PR, not the entire codebase. Proactive quality is a developer responsibility: unit test coverage, edge case analysis, and consistency with the surrounding codebase cannot be delegated to any review process. If the review surfaces one instance of a pattern, the developer is responsible for auditing all occurrences of that pattern throughout the codebase — not just the specific line cited.
When addressing review feedback: Open a separate commit for each point you address, clearly referencing the finding in the commit message. This ensures knowledge is captured in the repository history and helps other developers avoid the same pitfalls. If you cannot address a point, leave a detailed comment in this PR review thread explaining specifically what is wrong with the finding or why it cannot be addressed — never ignore feedback silently. Use comments only in rare cases when the System has genuinely missed existing context in the repository; prefer commits as the primary response mechanism to build institutional knowledge.
wanguardd
changed the title
Review #1 ResponseNot applicable (stale / contradicts spec)
Commits1. c7b8276
|
alchezar
changed the title
wanguardd
left a comment
wanguardd
left a comment
There was a problem hiding this comment.
Re-Review: Response to Review #1
Thank you for the thorough, well-organized response. 14 of 15 findings were addressed with clear commits. One finding is retracted as incorrect. One new blocking issue has been identified from the post-fix CI run.
🔁 Retracted Finding
CORRECTNESS (Blocking — "Approval bypass: JWT issued before approved_at is set") — Retracted. Finding was incorrect.
The author is right. Task 029 Slide 17 explicitly states: "Confirm consumes one invite seat (atomically), issues an IC token, and enrolls the member into the workspace with the snapshot policy." Slide 18 confirms the approve/deny flow governs requestable: model access only — not the IC token itself. The approved_at column and /approve endpoint are audit/visibility surfaces, not an access gate. Immediate token issuance on accept is the specified behavior, not a bypass. Apologies for the noise.
Summary: Original 15 Findings
| # | Category | Finding | Status |
|---|---|---|---|
| 1 | 🏛️ ORGANIZATIONAL | freeform/ missing readme.md |
✅ Fixed — c7b8276 |
| 2 | 🏛️ ORGANIZATIONAL | routes/readme.md stale |
✅ Fixed — 3b01782 |
| 3 | 🐛 CORRECTNESS | Race condition / TOCTOU on seat claim | ✅ Fixed — db31032 |
| 4 | 🐛 CORRECTNESS | Approval bypass — JWT before approved_at |
⬛ Retracted (spec-compliant) |
| 5 | 🐛 CORRECTNESS | Wrong RBAC permission on budget endpoint | ✅ Fixed — 5ce7e4c (author note accepted: direction clarified) |
| 6 | 🐛 CORRECTNESS | test_preview_invite_ok fails |
✅ Fixed — fec12a7 |
| 7 | 🛡️ SECURITY | No rate limit on bcrypt-heavy accept endpoint | ✅ Fixed — 06d449f |
| 8 | 🛡️ SECURITY | Setup routes accessible to any authenticated user | ✅ Fixed — 743f1ff |
| 9 | 📋 REQUIREMENTS | Providers paste silently discards invite emails | ✅ Fixed — 93269b5 |
| 10 | 📋 REQUIREMENTS | requestable: policy directive not implemented |
✅ Fixed — efed3a8 |
| 11 | 📐 DESIGN | check_permission defined three times |
✅ Fixed — 8551938 |
| 12 | 🔍 QUALITY | nextest.toml status-level = "slow" undocumented |
✅ Addressed — 0c704f9 (comment added; intentional) |
| 13 | ✅ TESTING | test_kind: markers absent from 7 new test files |
✅ Fixed — 038cc03 |
| 14 | ✅ TESTING | 7 new test files not in tests/readme.md |
✅ Fixed — 91b0c6b |
| 15 | 📖 DOCUMENTATION | components/freeform/ missing readme.md |
✅ Fixed — 4a081e0 |
🐛 CORRECTNESS
🔴 Blocking · Schema validation tests stale — CI still failing (2 tests)
The PR adds 4 new application tables via migrations 031–034 (workspaces, workspace_policy, invite_links, invite_seats) and 5 net new indexes via migrations 032–035. Two tests in iron_token_manager::database_initialization were not updated to reflect the new schema and are now failing in CI.
Evidence:
FAIL iron_token_manager::database_initialization test_migrations_are_idempotent
assertion `left == right` failed: Should have exactly 18 application tables after multiple runs
left: 22
right: 18
FAIL iron_token_manager::database_initialization test_production_schema_matches_test_schema
assertion `left == right` failed: Production schema should match test schema
left: ["agent_budgets", ..., "invite_links", "invite_seats", ..., "workspace_policy", "workspaces"]
right: ["agent_budgets", ..., "project_provider_key_assignments", ..., "users"]
# "invite_links", "invite_seats", "workspace_policy", "workspaces" absent from right
Three fixes required in module/iron_token_manager/tests/database_initialization.rs:
- Line 85 — update table count:
// Before:
assert_eq!(table_count, 18, "Should have exactly 18 application tables after multiple runs");
// After:
assert_eq!(table_count, 22, "Should have exactly 22 application tables after multiple runs");- Lines 144–163 — add 4 missing tables to
expected_tables(alphabetical positions):
let expected_tables = vec![
"agent_budgets", "agents", "ai_provider_keys", "analytics_events",
"api_call_traces", "api_tokens", "audit_log", "blacklist",
"budget_change_requests", "budget_leases", "budget_modification_history",
"invite_links", // new — migration 032
"invite_seats", // new — migration 033
"project_provider_key_assignments", "system_config", "token_blacklist",
"token_usage", "usage_limits", "user_audit_log", "users",
"workspace_policy", // new — migration 031
"workspaces", // new — migration 031
];- Line 178 — update index count (032: +2, 033: +2, 035: drops 033's 2 then creates 3 = net +1; total +5):
// Before:
assert_eq!(index_count, 54, "Should have 54 indexes across all migrations");
// After:
assert_eq!(index_count, 59, "Should have 59 indexes across all migrations");Also (non-blocking, but worth catching in the same commit): test_all_migrations_have_guards at line 222 checks only migrations 1..=30. The 6 new migrations (031–036) all have guard tables — extend the range to 1..=36 for complete coverage.
Action:
- Update
database_initialization.rs: table count18→22, add 4 tables toexpected_tables, index count54→59, and extend guard check range to1..=36
The fix is a small, mechanical update to one test file. Once CI passes this PR is ready to merge.
wanguardd
changed the title
wanguardd
left a comment
wanguardd
left a comment
There was a problem hiding this comment.
Re-Review #3 — Freeform Onboarding
CI-BLOCK-1 from Re-Review #2 is still unresolved. This pass extends the analysis with requirements, security, and test-quality findings from a thorough read of the new migrations, parsers, route handlers, and frontend components.
BLOCKING
CI-BLOCK-1: database_initialization.rs — four assertions stale after PR migrations (unresolved from Re-Review #2)
module/iron_token_manager/tests/database_initialization.rs was not updated when migrations 031–036 were added. Three assertion values and one range literal must change before CI can pass.
test_schema_table_count — line 85
// current (wrong)
assert_eq!(table_count, 18, "Should have exactly 18 application tables after multiple runs");
// required
assert_eq!(table_count, 22, "Should have exactly 22 application tables after multiple runs");The four new application tables are: workspaces (031), workspace_policy (031), invite_links (032), invite_seats (033/035).
test_production_schema_matches_test_schema — lines 143–163
Four entries are missing from expected_tables. Insert in alphabetical order:
let expected_tables = vec![
"agent_budgets",
"agents",
"ai_provider_keys",
"analytics_events",
"api_call_traces",
"api_tokens",
"audit_log",
"blacklist",
"budget_change_requests",
"budget_leases",
"budget_modification_history",
"invite_links", // ADD — migration 032
"invite_seats", // ADD — migration 033/035
"project_provider_key_assignments",
"system_config",
"token_blacklist",
"token_usage",
"usage_limits",
"user_audit_log",
"users",
"workspace_policy", // ADD — migration 031
"workspaces", // ADD — migration 031
];test_production_schema_matches_test_schema — line 178
// current (wrong — 54)
assert_eq!(index_count, 54, "Should have 54 indexes across all migrations");
// required (59)
// Arithmetic: 032 +2, 033 +2, 035 drops 033's 2 and creates 3 (net +3) → total +5
assert_eq!(index_count, 59, "Should have 59 indexes across all migrations");test_all_migrations_have_guards — line 223
// current — only verifies 001–030
let guard_tables: Vec<String> = (1..=30)
// required — migrations 031–036 all have guard tables; they must be verified
let guard_tables: Vec<String> = (1..=36)REQUIREMENTS
REQ-W1: Magic-link login not implemented — spec Slide 1
Task 029 Slide 1: "The admin enters their email and receives a magic-link sign-in. No password. This is the only authentication surface — there is no separate sign-up."
auth/handlers.rs implements POST /api/v1/auth/login using email + password credentials. No magic-link endpoint exists. Task 029 does not list magic-link login in its "Out of scope" section, and the acceptance criteria states the full 20-slide flow must work end-to-end.
Options:
- Implement magic-link login as specified by Slide 1.
- If this is intentionally deferred, add a comment to the PR referencing the task that will cover it so reviewers can verify the scope decision.
REQ-W2: Registration FreeForm (first/last/birthday) not implemented — spec Slides 2–4, 15–17
Spec Slides 2–4 describe the admin landing on "Complete Your Registration" after clicking the magic link — a form with First Name, Last Name, Birthday, and a FreeForm toggle. Spec Slides 15–17 mirror this for members accepting an invite link.
InviteAcceptPage.vue shows only a password-entry form; there is no FreeForm toggle, no first/last/birthday fields, and no registration FreeForm endpoint. This is directly coupled to REQ-W1: without magic-link tokens there is no registration-trigger to hook the form onto.
Same resolution paths as REQ-W1.
REQ-W3: Spend-limit chip absent from invite preview — spec Slide 14
Spec Slide 14 shows three chips in the invite preview dialog:
gpt-5.4-mini default— present ✓$100/week limit— missing10 seats remaining— present ✓
Backend (routes/invites.rs:107–116): InvitePreviewResponse lacks budget_amount_cents and budget_period. The existing query at line 221 already JOINs workspace_policy to fetch default_model; two additional SELECT columns are all that is needed.
// Add to InvitePreviewResponse
pub budget_amount_cents: Option<i64>,
pub budget_period: Option<String>,Frontend (composables/useApi.ts:1022–1027): InvitePreviewResponse interface and InviteAcceptPage.vue both need updating to surface the chip. MeWorkspaceResponse (line 1052) already has these fields, showing the pattern is understood — it just needs to be applied here too.
SECURITY
SEC-W1: Email validator accepts whitespace in local part — freeform/invites.rs:20–29
fn is_valid_email(s: &str) -> bool {
let Some((local, domain)) = s.split_once('@') else { return false; };
!local.is_empty()
&& domain.contains('.')
&& !domain.starts_with('.')
&& !domain.ends_with('.')
&& domain.len() > 2
}The caller trims leading/trailing whitespace (raw.trim()), but internal whitespace in the local part is not checked. "user name@example.com" passes: split_once('@') yields local = "user name", and !local.is_empty() is true. No SQL injection risk (parameterised queries throughout), but whitespace-containing strings queued as pending invites will fail silently if email delivery is ever added.
Fix: Add && !local.contains(char::is_whitespace) (and symmetrically && !domain.contains(char::is_whitespace)), or replace the predicate with a minimal regex:
// Catches the most common invalid cases without over-engineering
static EMAIL_RE: once_cell::sync::Lazy<regex::Regex> =
once_cell::sync::Lazy::new(|| regex::Regex::new(r"^[^\s@]+@[^\s@]+\.[^\s@]{2,}$").unwrap());
fn is_valid_email(s: &str) -> bool {
EMAIL_RE.is_match(s)
}Or, if keeping the manual predicate, simply add the whitespace check:
!local.is_empty()
&& !local.contains(char::is_whitespace)
&& domain.contains('.')
&& !domain.starts_with('.')
&& !domain.ends_with('.')
&& !domain.contains(char::is_whitespace)
&& domain.len() > 2TESTING
TEST-W1: Bare .unwrap() throughout all four parser unit test files
All four new parser unit test files use bare .unwrap() without a .expect("LOUD FAILURE: …") message. When a test fails in CI, the output shows called \Result::unwrap()` on an `Err` value` with no indication of which test case failed or what the input was.
Affected call sites:
freeform_company_setup_test.rs:9,17,23—company_setup::parse(…).unwrap()freeform_invites_test.rs:14,33,40,47,67—invites::parse(…).unwrap()freeform_providers_test.rs:14,30,67,74,75—providers::parse(…).unwrap()freeform_usage_policy_test.rs:13,33,66,67,93,106,123—usage_policy::parse(…).unwrap()
Fix: Replace every .unwrap() with .expect("LOUD FAILURE: <input> should parse as <expected result>") throughout all four files. Example:
// current
let result = company_setup::parse("Acme Corp, acme.com, Client").unwrap();
// required
let result = company_setup::parse("Acme Corp, acme.com, Client")
.expect("LOUD FAILURE: 'Acme Corp, acme.com, Client' is valid client company setup");TEST-W2: No end-to-end test for requestable_models persist-and-retrieve
freeform_usage_policy_test.rs verifies the parser recognises the requestable: directive and populates the struct correctly. However, workspace_budget_test.rs has no test for the full path:
POST /workspace/budgetwithtext: "limit all users $100/week - requestable: gpt-5, claude-3"GET /me/workspace- Assert
requestable_models == ["gpt-5", "claude-3"]
Migration 036 adds the requestable_models TEXT column. Without an end-to-end test, a regression in serialization, the budget-handler write path, or the GET /me/workspace read path would go undetected.
Note: pre-existing test failures in iron_cli
CI shows 8 failures in iron_cli::parameters_test that pre-date this branch. Nextest's fail-fast default halts the run before iron_token_manager tests execute, which means CI-BLOCK-1 does not yet appear in the CI run output as an explicit iron_token_manager failure — it will surface once the iron_cli failures are fixed. Both need to be addressed before merge: CI-BLOCK-1 in this PR, the iron_cli failures in a separate fix.
Summary: CI-BLOCK-1 (one assertion-update commit away from green) remains the only hard blocker. REQ-W1 and REQ-W2 are the most significant new findings — they represent a fundamental auth-surface deviation from the spec and should be clarified even if they are intentionally deferred. REQ-W3, SEC-W1, TEST-W1, and TEST-W2 are straightforward fixes. The core architecture — deterministic parsers, BEGIN IMMEDIATE seat-claim, policy snapshot on invite generation, TCP-peer rate-limiting — is correct and well-constructed.
2 participants
FreeForm Onboarding
Zero-to-full-account workspace onboarding via structured paste. Admin walks
through a 4-step wizard (company -> providers -> budget -> invite link),
member joins via the link, generates an IC token, admin approves. All parsers
are deterministic CSV-style - no LLM.
Backend (
iron_control_api)company_setup,providers,invites,usage_policyPOST /freeform/{company,providers}- paste -> upsertPOST /workspace/budget- structured or freeform; persistsdefault_modelGET /me/workspace- workspace info for current user (any role)POST /invites/generate,GET /invites/{token},POST /invites/{token}/acceptGET /invites/pending,POST /invites/seats/{id}/approve- seat-based approvalworkspaces,workspace_policy,invite_links,invite_seats, + seed row so a fresh DB doesn't 404Frontend (
iron_dashboard)FreeFormDialog/FreeFormToggle/AutoSetupWizardcomponents/setup/{company,providers,budget,users}/join/{token}-> member dashboard/member(Gateway URL, IC token, default model)VITE_GATEWAY_URLenv var (falls back towindow.location.origin)Manual test
Acme Corp, acme.com, Clientopenai: sk-test-fake\nanthropic: sk-ant-test-fakelimit all users $100/week\ndefault: claude-3-haiku/memberSecurity
ManageUsersfor workspace/invite admin endpoints,ManageProviderKeysfor provider endpoints