[codex] Port GSuite WASM capabilities to Reborn - #4326
Conversation
There was a problem hiding this comment.
Code Review
This pull request integrates Google Drive, Docs, Sheets, and Slides as host-bundled WASM packages, introducing operation-level capability IDs and updating host-runtime credential obligations to preserve provider scopes. The review feedback suggests using the established urlencoding crate instead of hand-rolling percent-encoding functions across the GSuite WASM tools. Additionally, it recommends defensively handling empty or whitespace-only parameter strings in params_with_action to prevent parsing failures when actions have no required parameters.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Quick review pass: nice direction overall. I especially like the provider-scope propagation in runtime credential obligations and the operation-level split for the GSuite WASM assets. One thing I’d want an extra sanity check on before merge: several Docs operations expose raw positional indices ( Not saying this PR must solve that now, but I’d at least consider either:
Aside from that, the shape looks consistent. |
serrrfirat
left a comment
There was a problem hiding this comment.
Multi-agent review completed with --force against 6d9af2f69380762985fc46757126789039fe3c70.
Result: COMMENT (GitHub does not allow requesting changes on your own PR; treat the Medium+ findings below as blocking review feedback.)
Summary:
- 1 High finding: the new GSuite scopes are not accepted by the existing Google OAuth allowlist, so auth-required recovery cannot create matching accounts for these capabilities.
- Several Medium findings around least-privilege OAuth scopes, payload logging/data leakage, fail-closed manifest validation, local trust wiring, and missing caller-level tests.
- Low findings around production
.expect()in the WASM schema path and avoidable bundled asset copies are included in the summary but are not blocking by themselves.
Reviewers: security, bugs, performance/concurrency, tests, conventions.
Key items to address before landing:
- Add allowed Google OAuth scopes for Drive/Docs/Sheets/Slides, preferably readonly scopes for readonly capabilities and write scopes only for mutating capabilities.
- Stop logging full GSuite action structs; those payloads can contain uploaded content, document text, spreadsheet values, recipients, file IDs, and raw batchUpdate bodies.
- Reject
provider_scopeson non-product_auth_accountruntime credentials instead of accepting meaningless declarations. - Add trust-policy digest entries for the new bundled GSuite packages or document and test why they intentionally differ from Calendar/Gmail.
- Add contract tests for
provider_scopesmanifest validation, auth-required scope propagation, context-derived action rejection, and host-runtime smoke tests for Docs/Sheets/Slides.
|
Deeper pass on #4326 after reviewing the file list and existing comments. A few line-level themes I’d flag:
Overall the direction makes sense; these are mainly robustness / maintainability edges. |
serrrfirat
left a comment
There was a problem hiding this comment.
Code Review — [codex] Port GSuite WASM capabilities to Reborn
Reviewers: Security · Bugs · Performance · Tests · Conventions
Head: 6d9af2f
Findings: 2 High · 3 Medium · 5 Low/Nit
Event: REQUEST_CHANGES (1 High finding ≥75% confidence)
🔴 High
f-bug-1 — Fixed multipart boundary causes silent upload corruption
File: crates/ironclaw_first_party_extensions/assets/google-drive/wasm-src/src/api.rs:226
upload_file uses a hardcoded MIME boundary "ironclaw_upload_boundary_42". If the uploaded content contains this exact string, the multipart body is structurally corrupted — the Drive API will either reject it or truncate the content at the first boundary match. RFC 2046 requires boundaries to be chosen to not appear in the body.
Fix: Generate a unique boundary per call, e.g. use a random suffix or content-hash fragment.
f-sec-1 — Info-level log emits full action params including document content
File: crates/ironclaw_first_party_extensions/assets/google-docs/wasm-src/src/lib.rs:100
format!("Executing Google Docs action: {:?}", action) at LogLevel::Info serializes the entire action struct. For InsertText, ReplaceText, BatchUpdate, this includes the full document text supplied by the user. Logs are infrastructure-accessible and may persist beyond the session. The Drive API calls (line 29) correctly use Debug for this.
Fix: Downgrade to LogLevel::Debug, or log only the action variant name: std::mem::discriminant(&action).
🟡 Medium
f-bug-2 — format_text silently drops invalid hex colors with no error
File: crates/ironclaw_first_party_extensions/assets/google-docs/wasm-src/src/api.rs:352-363
When parse_hex_color returns None (bad hex), the color field is silently skipped. If the user passes foreground_color but no other options, fields stays empty and the function returns Err("No formatting options specified") — confusingly attributing the failure to missing options rather than an invalid color value. If other options are also set, the color is silently dropped without any warning.
Fix: Return Err(format!("invalid hex color: {}", color)) when parse_hex_color returns None.
f-bug-3 — extract_text_from_elements silently omits table-of-contents and footnote content
File: crates/ironclaw_first_party_extensions/assets/google-docs/wasm-src/src/api.rs:155
read_content only dispatches on paragraph and table elements. The Docs API structural element list also includes tableOfContents, sectionBreak, and inline objects. Text in these elements is silently omitted, so read_content can return materially incomplete text for real-world documents. Headers/footers are in separate segments and are also not read.
Fix: Document the limitation in the function's doc-comment (e.g. "reads body segment only; headers, footers, and footnotes are excluded") so callers understand the scope, or handle tableOfContents by recursing into its inner content array.
f-tests-1 — validate_runtime_credential_provider_scopes error paths have no unit tests
File: crates/ironclaw_extensions/src/v2.rs:1450
The new function rejects empty strings, whitespace-padded strings, and duplicates — but only the happy path is exercised via the manifest loading tests. There are no direct unit tests for any of these three rejection paths.
Fix: Add tests::validate_runtime_credential_provider_scopes_rejects_empty_scope, ..._rejects_whitespace_padded_scope, ..._rejects_duplicate_scope in the same file's #[cfg(test)] module.
🟢 Low / Nit
f-tests-2 — params_with_action action-injection guard not tested
File: crates/ironclaw_first_party_extensions/assets/google-docs/wasm-src/src/lib.rs:228
The function returns Err("invalid_parameters") when params already contain an "action" key. This is a security invariant (prevents action-spoofing) with no test coverage.
Fix: Add tests::params_with_action_rejects_caller_supplied_action_key in lib.rs test module.
f-tests-3 — Only Drive has a host-runtime integration test; Docs/Sheets/Slides do not
File: PR body
The PR validates the Drive WASM list-files path with a full host-runtime integration test (host_runtime_services_routes_google_drive_wasm_list_files_with_scoped_google_credential) but adds no equivalent for the other three tools. Given the PR notes "Full workspace test suite was not run", the three tools have no runtime-level signal at all.
Fix: Add at minimum one host-runtime contract test per new tool (e.g. google_docs_wasm_read_content_with_scoped_google_credential), or document explicitly that coverage will be added in a follow-on.
f-bug-4 — Any negative index triggers end-of-segment insertion, not just -1
File: crates/ironclaw_first_party_extensions/assets/google-docs/wasm-src/src/api.rs:165
The check if index < 0 maps all negative values to endOfSegmentLocation. The schema doc says "Use -1 to append at end" implying this is the only sentinel; passing -5 silently appends too. A user debugging an off-by-one might not realize -2 is not "two before end".
Fix: Change to if index == -1 and return an error for other negative values.
f-conv-1 — url_encode and api_call bodies duplicated across all 4 WASM crates
File: crates/ironclaw_first_party_extensions/assets/google-docs/wasm-src/src/api.rs:497 vs google-drive/wasm-src/src/api.rs:497
The url_encode function is byte-for-byte identical in all four WASM crates. api_call is functionally identical (same pattern, different constant). This is fine for now given WASM sandboxing (no shared crates), but worth tracking.
Anchor: crates/ironclaw_first_party_extensions/assets/google-drive/wasm-src/src/api.rs:497
f-perf-1 — download_file makes 2 sequential API calls (metadata + download)
File: crates/ironclaw_first_party_extensions/assets/google-drive/wasm-src/src/api.rs:176
download_file calls get_file first to determine MIME type, then performs the actual download. For the common case (known mime type or non-Workspace file), the first call is wasted latency. A fields=mimeType,name param-only request or accepting an optional mime_type hint from the caller would eliminate the extra round-trip.
Fix: Accept an optional mime_type: Option<&str> parameter; only call get_file when it's None.
…suite-reborn-parity
abbyshekit
left a comment
There was a problem hiding this comment.
Code review — [codex] Port GSuite WASM capabilities to Reborn
Multi-agent review (security · bugs · performance · tests · conventions) at b7ff17b2. Diff-only mode.
Retrospective review (this PR is already merged) — findings inform follow-ups, not blocking.
3 findings — 2 Medium, 1 Low. Posted as a comment (advisory). Confidence ≥ 50, deduplicated across reviewers.
| Sev | Conf | Reviewer | Location | Finding |
|---|---|---|---|---|
| Medium | 80% | conventions | capability.rs:83 |
New runtime-credential provider_scopes crosses 5 authority-bearing layers as Vec instead of the existing ProviderScope newtype |
| Medium | 55% | tests | runtime_credentials_contract.rs:213 |
No positive / over-scope test for scope-based account selection (the PR's stated priority) |
| Low | 55% | bugs | obligations.rs:1764 |
Re-auth requirement carries provider_scopes that no consumer reads, so scope-gated re-auth prompts cannot request the right scopes |
Generated by near-ai-code-review (5 parallel reviewer agents + intent analysis). Diff-only; confidence ≥ 50; ≤ 15 inline comments.
| #[serde(default)] | ||
| pub source: RuntimeCredentialRequirementSource, | ||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
| pub provider_scopes: Vec<String>, |
There was a problem hiding this comment.
[Medium · 80% confidence] New runtime-credential provider_scopes crosses 5 authority-bearing layers as Vec instead of the existing ProviderScope newtype
conventions reviewer
The new provider_scopes field is added as a raw Vec<String> on RuntimeCredentialRequirement (capability.rs:83) and then threaded unchanged through every internal authority layer: Obligation::InjectCredentialAccountOnce and RuntimeCredentialAuthRequirement (ironclaw_host_api/src/decision.rs:61,83), RuntimeCredentialAccountRequest<'a>.provider_scopes: &'a [String] (ironclaw_host_runtime/src/obligations.rs:44), the authorization obligation builder (ironclaw_authorization/src/lib.rs:1181), and only at the very end is it parsed into the domain newtype ProviderScope::new(...) in ProductAuthRuntimeCredentialResolver (ironclaw_reborn_composition/src/product_auth_runtime_credentials.rs:131-144). This is the exact anti-pattern .claude/rules/types.md forbids: 'Raw String is a boundary format ... converted to a domain type at the earliest opportunity. Everything flowing between internal modules must carry a type that makes misuse a compile error' and 'Don't return String from an internal function — return the newtype.' The repo already has the correct shape one struct away: CredentialAccount.scopes: Vec<ProviderScope> (ironclaw_auth/src/credential.rs:53), and the sibling fields on this very struct are all newtypes (handle: SecretHandle line 79, audience: NetworkTargetPattern line 84). The ironclaw_host_api crate doc additionally states it 'intentionally contains authority-bearing types, validation, and serialization contracts only' (lib.rs:9) — an unvalidated scope String on an authority-bearing capability type contradicts that contract. Consequence beyond style: the manifest layer hand-rolls trim/empty/dedup validation in validate_runtime_credential_provider_scopes (ironclaw_extensions/src/v2.rs:1451), then the resolver re-validates the same strings via ProviderScope::new (which runs validate_public_text: empty + leading/trailing-whitespace + NUL/control checks). Two validators of record for one value is precisely the divergence-over-layers failure types.md was written to prevent (#2574-class). Note: this is not in the bugs/security/perf lens — account_has_provider_scopes (product_auth_runtime_credentials.rs:170) correctly requires the account scope set to contain every required scope, so account selection is sound; the issue is the typing/validation discipline of the new internal contract.
Fix: Carry the scope as a validated newtype across the internal layers rather than Vec<String>. Because ironclaw_host_api is the dependency leaf (it has zero ironclaw deps; ironclaw_auth depends on it) while ProviderScope currently lives in ironclaw_auth, the clean move is to define the scope newtype in ironclaw_host_api (alongside RuntimeCredentialAccountProviderId/SecretHandle in ids.rs) and have ironclaw_auth re-export it, then type provider_scopes: Vec<ProviderScope> on RuntimeCredentialRequirement, the two Obligation/RuntimeCredentialAuthRequirement variants, and RuntimeCredentialAccountRequest. Validation then happens once at construction (ProviderScope::new/try_from), validate_runtime_credential_provider_scopes keeps only the source-gating + dedup rules and drops the duplicated trim/empty checks, and the resolver's re-parse loop disappears. If the cross-crate move is judged out of scope for a follow-up, at minimum delete the duplicated string-validation in v2.rs and document that ProviderScope::new is the single source of truth so the two validators cannot drift.
// crates/ironclaw_host_api/src/capability.rs
pub struct RuntimeCredentialRequirement {
pub handle: SecretHandle,
#[serde(default)]
pub source: RuntimeCredentialRequirementSource,
#[serde(default, skip_serializing_if = "Vec::is_empty")]
pub provider_scopes: Vec<ProviderScope>, // newtype defined in ironclaw_host_api, re-exported by ironclaw_auth
pub audience: NetworkTargetPattern,
pub target: RuntimeCredentialTarget,
pub required: bool,
}| &[Obligation::InjectCredentialAccountOnce { | ||
| handle: slot, | ||
| provider: RuntimeCredentialAccountProviderId::new("github").unwrap(), | ||
| provider_scopes: vec!["repo".to_string()], |
There was a problem hiding this comment.
[Medium · 55% confidence] No positive / over-scope test for scope-based account selection (the PR's stated priority)
tests reviewer
This PR's entire purpose is to thread provider_scopes from the manifest into runtime-credential selection so product-auth picks a least-privilege Google account. The propagation chain is well covered by new tests: the manifest validator (v2::tests::validate_runtime_credential_provider_scopes_* and manifest_v2_contract), the obligation emission (runtime_credentials_contract::capability_access_resolves_product_auth_account_runtime_credentials, asserting provider_scopes: vec!["repo"]), and the host obligation handler (builtin_obligation_handler_contract.rs:1333-1364, asserting a non-empty drive.readonly scope reaches credential_requirements[0].provider_scopes).
The actual selection DECISION that those scopes drive is account_has_provider_scopes in crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials.rs:170 (required.all(|r| account.scopes.any(|s| s == r))), consumed by ProductAuthRuntimeCredentialResolver::resolve_access_secret. Of the 11 resolver tests in that module's mod tests, exactly one passes a non-empty provider_scopes (resolver_requires_requested_provider_scopes, line 482), and it asserts only the NEGATIVE branch: account holds gmail.send, request requires drive, result AuthRequired. Every test that asserts a successful Ok(secret) selection (lines 285/327/370/413/701) passes provider_scopes: &[].
Result: the positive branch of account_has_provider_scopes is never exercised end-to-end, and the over-scoped case the PR explicitly calls out ("can it select an over-scoped/wrong account") has zero coverage. A regression inverting the filter (e.g. !account.scopes.contains, or swapping the subset/superset direction so a documents.readonly-only account satisfies a documents write requirement) would still pass every existing test, because no positive selection ever supplies a required scope. This is the same caller-level coverage class the repo's testing.md "Test Through the Caller" rule mandates for a predicate that gates a secret read. (Selection module is pre-existing, not edited by this PR; anchored here on the in-scope obligation test this PR modified.)
Fix: Add tests::product_auth_runtime_credentials::resolver_selects_account_whose_scopes_superset_required covering the positive/over-scope path: create one Configured UserReusable google account whose scopes are a strict superset of the request (e.g. account has [.../drive, .../gmail.send], request provider_scopes = [".../drive"]) and assert resolve_access_secret returns Ok(access_secret); plus tests::product_auth_runtime_credentials::resolver_requires_exact_scope_string_not_readonly_variant asserting a documents-only account does NOT satisfy a documents.readonly requirement (and vice versa), pinning the exact-string match semantics. Both belong in the mod tests of crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials.rs.
#[tokio::test]
async fn resolver_selects_account_whose_scopes_superset_required() {
// account.scopes = [drive, gmail.send]; request requires only [drive] -> Ok(secret)
// guards account_has_provider_scopes positive/over-scope branch
}| vec![RuntimeCredentialAuthRequirement { | ||
| provider: obligation.provider.clone(), | ||
| requester_extension: obligation.requester_extension.clone(), | ||
| provider_scopes: obligation.provider_scopes.to_vec(), |
There was a problem hiding this comment.
[Low · 55% confidence] Re-auth requirement carries provider_scopes that no consumer reads, so scope-gated re-auth prompts cannot request the right scopes
bugs reviewer
This PR adds provider_scopes to RuntimeCredentialAuthRequirement (crates/ironclaw_host_api/src/decision.rs:84) and populates it in the AuthRequired recovery path here at obligations.rs:1764 (provider_scopes: obligation.provider_scopes.to_vec()). The field is then propagated through ironclaw_turns (events.rs, store.rs, memory.rs, loop_exit.rs) into durable turn state. However, the ONLY non-test code that reads RuntimeCredentialAuthRequirement to build a re-auth prompt is auth_prompt_from_credential_requirement in crates/ironclaw_reborn_composition/src/projection/turn_events.rs:391-402, which reads only requirement.provider and emits an AuthPromptChallengeKind::ManualToken view; it never reads requirement.provider_scopes. Concrete consequence: when account selection fails specifically because no Configured account holds the required scopes (account_has_provider_scopes filter in product_auth_runtime_credentials.rs rejects every account), the recovery fires an AuthRequired prompt that has lost the information about which scopes were missing. For the current ManualToken flow this is only a usability gap (the user must already know which scopes their pasted token needs), and it fails closed rather than selecting a wrong/over-scoped account, so it is not a correctness or privilege defect. But it means the PR's stated goal -- letting product-auth select a Google account with the required OAuth scopes -- is not completed on the re-auth side: a future OAuth-consent prompt cannot derive the consent scope set from this requirement as wired, and a user who re-auths with the same insufficient-scope token will loop back to the same AuthRequired state.
Fix: Have the re-auth prompt projection consume requirement.provider_scopes. In auth_prompt_from_credential_requirement (turn_events.rs) thread provider_scopes into the AuthPromptView so the consent/OAuth flow (or at minimum the manual-token instructions) can communicate the required scopes; or, if ManualToken is intentionally scope-agnostic, drop the field from the recovery population to avoid a written-but-unused contract that suggests scope-aware re-auth that does not happen.
// turn_events.rs: auth_prompt_from_credential_requirement
let provider = requirement.provider.as_str().to_string();
view.challenge_kind = Some(AuthPromptChallengeKind::ManualToken);
view.provider = Some(provider.clone());
view.account_label = Some(provider);
// surface required scopes so the re-auth flow can request them
view.required_scopes = requirement.provider_scopes.clone();
view…d behavior (#5105) Three guard tests failed on main because they asserted pre-change behavior that was intentionally updated, not because the guards regressed. provider_tool_call_validation_rejects_sensitive_metadata (loop_support) and provider_reference_validation_rejects_sensitive_arguments_and_text (threads) both delegate to ironclaw_safety::provider_validation. #5001 deliberately dropped the crude bare-word substring markers (traceback / password / stack trace) on model reasoning/metadata text, leaving the entropy-based LeakDetector as the sole guard there (locked in by the safety crate's own updated tests). The secret-token half of each test already passed; only the bare-word assertion failed. Switch those assertions to a real secret-like token so they verify the actual guard: secrets leaked into reasoning text are rejected. google_callback_state_rejects_unapproved_requested_scopes (auth): #4326 intentionally added the Drive/Docs/Sheets/Slides scopes to the approved GSuite set (is_allowed_google_scope) to support ported GSuite capabilities, so .../auth/drive is no longer unapproved. Use .../auth/gmail.insert -- a real sensitive scope still outside the approved set -- so the test keeps guarding the real boundary. These crates were outside the CI closure, so the stale tests went unnoticed. Test-only change; no production behavior change. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…d behavior (nearai#5105) Three guard tests failed on main because they asserted pre-change behavior that was intentionally updated, not because the guards regressed. provider_tool_call_validation_rejects_sensitive_metadata (loop_support) and provider_reference_validation_rejects_sensitive_arguments_and_text (threads) both delegate to ironclaw_safety::provider_validation. nearai#5001 deliberately dropped the crude bare-word substring markers (traceback / password / stack trace) on model reasoning/metadata text, leaving the entropy-based LeakDetector as the sole guard there (locked in by the safety crate's own updated tests). The secret-token half of each test already passed; only the bare-word assertion failed. Switch those assertions to a real secret-like token so they verify the actual guard: secrets leaked into reasoning text are rejected. google_callback_state_rejects_unapproved_requested_scopes (auth): nearai#4326 intentionally added the Drive/Docs/Sheets/Slides scopes to the approved GSuite set (is_allowed_google_scope) to support ported GSuite capabilities, so .../auth/drive is no longer unapproved. Use .../auth/gmail.insert -- a real sensitive scope still outside the approved set -- so the test keeps guarding the real boundary. These crates were outside the CI closure, so the stale tests went unnoticed. Test-only change; no production behavior change. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* Port GSuite WASM capabilities to Reborn * Fix GSuite Reborn PR review findings * Address follow-up GSuite review comments * Fix GSuite Reborn parity test expectations * Fix Reborn credential resolver merge test * Fix clippy in WASM runtime contract test * Fix clippy in GSuite capability test --------- Co-authored-by: IronClaw Agent <agent@ironclaw.com>
Summary
Validation
Notes