[codex] Update OAuth SDK skill examples#90
Conversation
WalkthroughDocumentation updates modernize OAuth SDK method signatures across SPA and SSR integration examples from object-style to ChangesOAuth SDK Documentation Modernization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR updates the InsForge auth skill documentation to adopt the new
Confidence Score: 5/5Documentation-only changes that consistently update the OAuth call signature across both integration guides with no logic or runtime code affected. Both changed files are pure Markdown skill documentation. The signature migration is applied consistently in every code block and in the method reference table. The additionalParams guidance and server-owned-field restriction prose are identical in both files. No runtime code, migrations, or config are touched. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant App as App (SPA/SSR)
participant SDK as InsForge SDK
participant Backend as InsForge Backend
participant Provider as OAuth Provider (e.g. Google)
Note over App,SDK: New API: signInWithOAuth(provider, { redirectTo, additionalParams? })
App->>SDK: "signInWithOAuth('google', { redirectTo, additionalParams })"
SDK->>Backend: Initiate OAuth (PKCE, provider params merged)
Note over Backend: Sets client_id, scope, redirect_uri, code_challenge, state, response_type
Backend-->>SDK: "{ url, codeVerifier }"
alt SPA flow (skipBrowserRedirect omitted)
SDK->>Provider: Auto-redirect browser to url
Provider-->>App: "Redirect to redirectTo?insforge_code=code"
SDK->>Backend: Auto-exchange insforge_code + codeVerifier
Backend-->>SDK: "{ accessToken, refreshToken }"
else SSR flow (skipBrowserRedirect: true)
SDK-->>App: "{ data: { url, codeVerifier } }"
App->>App: Store codeVerifier in httpOnly cookie
App->>Provider: redirect(data.url)
Provider-->>App: "GET /api/auth/callback?insforge_code=code"
App->>Backend: exchangeOAuthCode(code, codeVerifier)
Backend-->>App: "{ accessToken, refreshToken }"
App->>App: setAuthCookies(response.cookies, ...)
end
Reviews (3): Last reviewed commit: "Update OAuth skill docs" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
skills/insforge/SKILL.md (1)
145-148: 💤 Low valueConsider consistent terminology for SMTP across documentation.
The scope list says "auth SMTP settings" here, while
skills/insforge-cli/SKILL.mdline 187 says "SMTP" (both referring to the same auth SMTP configuration). Both are clear in context, but using identical wording would improve consistency.For example, you could align both to say either "SMTP" or "auth SMTP settings" throughout.
🤖 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 `@skills/insforge/SKILL.md` around lines 145 - 148, The two docs use inconsistent wording for the same setting: change the phrase in skills/insforge/SKILL.md ("auth SMTP settings") and in skills/insforge-cli/SKILL.md ("SMTP") to the same canonical term (pick either "SMTP" or "auth SMTP settings") so both files read identically; update the text in the export/plan/apply description and any other occurrences in those SKILL.md files to the chosen term to ensure consistency across the CLI documentation.
🤖 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 `@skills/insforge/SKILL.md`:
- Around line 145-148: The two docs use inconsistent wording for the same
setting: change the phrase in skills/insforge/SKILL.md ("auth SMTP settings")
and in skills/insforge-cli/SKILL.md ("SMTP") to the same canonical term (pick
either "SMTP" or "auth SMTP settings") so both files read identically; update
the text in the export/plan/apply description and any other occurrences in those
SKILL.md files to the chosen term to ensure consistency across the CLI
documentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca7be3f0-9d4f-4686-8d2d-071e34d8a93d
📒 Files selected for processing (5)
skills/insforge-cli/SKILL.mdskills/insforge-cli/references/config.mdskills/insforge/SKILL.mdskills/insforge/auth/sdk-integration.mdskills/insforge/auth/ssr-integration.md
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
Docs-only PR that rewrites the signInWithOAuth examples to a provider-first signature and broadens the insforge.toml config docs — but both halves document capabilities that don't exist in the currently published @insforge/sdk / @insforge/cli, so an agent following them would generate non-working code/config.
Requirements context
No /docs/superpowers/ (or any specs//plans/) directory exists in this repo — assessing against the PR description and the repo's own conventions in AGENTS.md (validation checklist requires "code examples are syntactically correct" and accurate) and CONTRIBUTING.md. No matching spec/plan found.
I verified the claims against ground truth: the published SDK source (@insforge/sdk@1.3.0, the latest tag), the published CLI bundle (@insforge/cli@0.1.86, latest), and the canonical InsForge SDK docs (two independent context7 sources). Findings below cite that evidence.
Findings
🔴 Critical
1. (Functionality) The new signInWithOAuth(provider, options) signature does not match the published SDK — examples produce broken auth code.
skills/insforge/auth/sdk-integration.md:118-121, :131-134, :143-144 and skills/insforge/auth/ssr-integration.md:163.
The published SDK (@insforge/sdk@1.3.0, the latest dist-tag) implements:
async signInWithOAuth(options) {
const { provider, redirectTo, skipBrowserRedirect } = options;i.e. a single object signInWithOAuth({ provider, redirectTo, skipBrowserRedirect }) — the exact form this PR removes. If an agent follows the new docs and calls signInWithOAuth('google', { ... }), the SDK binds options = 'google', so options.provider is undefined and the OAuth init fails. This is corroborated by the canonical TS SDK docs (docs/sdks/typescript/auth.mdx) and the published web docs (docs.insforge.dev/core-concepts/authentication/sdk), both of which still show the object form.
additionalParams (sdk-integration.md:120, :143) is likewise not read anywhere in the SDK source — no additionalParams, prompt, or select_account handling exists — so the documented Google prompt: 'select_account' param would be silently dropped even if the signature were correct.
If a newer SDK that ships the positional signature + additionalParams is releasing in lockstep: please name the minimum @insforge/sdk version in the doc and confirm it's published before merge, since no published version (including the ssr/dev prerelease tags) currently supports it.
2. (Functionality) Config docs claim insforge.toml sections the latest CLI doesn't support — silent no-op for users.
skills/insforge-cli/references/config.md:49-52 (storage.max_file_size_mb, realtime.retention_days, schedules.retention_days), :31 (disable_signup), :82-83 (the skipped[] example is keyed on storage.max_file_size_mb), plus the SKILL.md scope lines (skills/insforge-cli/SKILL.md) and the sample skip message that now uses storage.max_file_size_mb.
Grepping the published CLI bundle (@insforge/cli@0.1.86, latest) shows these literals never appear: max_file_size_mb → 0, retention_days → 0, disable_signup → 0. (By contrast the rest of the expansion is backed by the CLI: allowed_redirect_urls, require_email_verification, verify_email_method, reset_password_method, password min_length/require_special_char, smtp, and subdomain all appear.) Because the CLI doesn't recognize these keys at all, they won't be exported/planned and an apply won't even route them through the capability-gate skipped[] path the docs promise — the agent writes the knob, reports success, and the setting silently never takes effect (e.g. storage size limit not enforced). Using storage.max_file_size_mb as the headline skip-message example makes a fictional capability the canonical illustration.
Same caveat: if a coordinated @insforge/cli release adds these, state the required CLI version; otherwise drop storage, realtime, schedules, and disable_signup from this PR and document them when they ship.
🟡 Suggestion
- (Software engineering) The repo has no automated check that examples track the SDK/CLI they describe (
AGENTS.mdvalidation is manual). Both Critical items would have been caught by pinning the doc to a concrete@insforge/sdk/@insforge/cliversion and cross-checking. Consider adding a "verified against@insforge/sdk@x.y.z/@insforge/cli@x.y.z" note near these examples so future drift is visible. - (Functionality) The pre-PR SKILL.md was explicit that the scope was "only
auth.allowed_redirect_urls". This PR is a large capability claim; splitting the genuinely-shipped sections (auth flags, password policy, SMTP, subdomain) from the not-yet-shipped ones (storage/retention) keeps the docs truthful at every commit and avoids an all-or-nothing revert.
🔵 Information
- (Security) No security-relevant regressions. OAuth remains PKCE; no auth/authorization checks are weakened. The SMTP block (
config.md:39-47) uses empty-string placeholders only — no secret/credential is embedded or leaked. No new dependencies. No security concerns. - (Performance) Docs-only change; no runtime code paths, queries, or hot-path work affected. No performance concerns.
- Consistency check passed:
rg signInWithOAuthshows no remaining object-form{ provider }examples underskills/— the rewrite is internally consistent (the issue is that the chosen target signature is wrong, not that some call sites were missed).
Verdict
request_changes — two Critical findings: the OAuth examples don't match the shipped SDK (and additionalParams isn't supported), and the config docs advertise CLI sections (storage.max_file_size_mb, realtime/schedules retention_days, disable_signup) that the latest published CLI doesn't implement. Both lead an agent to emit code/config that silently fails. If newer SDK/CLI releases back these claims, gate the docs on those versions and re-request review.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="skills/insforge/SKILL.md">
<violation number="1">
P3: Section heading contradicts the content: it says backend config is not in CLI, but the paragraph immediately says auth redirect allowlist is now managed via CLI.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Fermionic-Lyu
force-pushed
the
codex/update-oauth-skill-docs
branch
from
c582b30 to
d3b6172
Compare
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Review: Update OAuth SDK skill examples
Summary: Docs-only change updating the InsForge auth skill examples from the object-form signInWithOAuth({ provider, ... }) to the provider-first signInWithOAuth(provider, options) signature, adding additionalParams and a warning against passing server-owned OAuth fields.
Requirements context: No /docs/superpowers/ (or equivalent specs/plans) directory exists in this repo — assessing against the PR description and the actual SDK API alone. Final scope confirmed as exactly 2 files (skills/insforge/auth/sdk-integration.md, skills/insforge/auth/ssr-integration.md); the config-management / insforge.toml changes referenced in some auto-generated bot summaries are not part of the final diff (they were dropped in c582b30), so they are out of scope for this review.
Findings
Critical
(none) — I could not confirm a definite correctness bug. See the signature note under Suggestions, which is the one item worth verifying before merge.
Suggestion
Functionality — confirm the positional signature + additionalParams against the shipped SDK (skills/insforge/auth/sdk-integration.md:118, :133, :145-146; skills/insforge/auth/ssr-integration.md:163)
The canonical InsForge TypeScript SDK docs (insforge/insforge → docs/sdks/typescript/auth.mdx, as indexed by context7) currently show the object-form call:
await insforge.auth.signInWithOAuth({ provider: 'google', redirectTo: '...' });and do not document an additionalParams field — only provider, redirectTo, and skipBrowserRedirect. That is the exact form this PR is moving away from, so there's a direct contradiction between this change and the published SDK reference.
Because these skill docs are consumed by AI agents to generate working auth code, an incorrect call signature would produce non-functional flows — so this is the one thing to nail down. I'm filing it as a Suggestion rather than blocking because I can't independently verify the released @insforge/sdk source from here, the PR asserts a recent SDK change added positional + additionalParams support, and a well-designed SDK may accept both forms via runtime arg detection. Please confirm against the actually-released SDK version, and if the positional form is correct, update the canonical insforge/insforge SDK docs in the same change so the two sources don't diverge. If the SDK only accepts the object form, this PR should be reverted/reworked.
Information
- Consistency (software engineering):
rg signInWithOAuthacross the repo confirms no leftover object-form examples remain after this change; the method-reference table (sdk-integration.md:145-146) was updated to match the examples, andskills/insforge/SKILL.md:173only lists the bare method name (no signature) so it needs no update. Internally consistent. 👍 - Security: No security-relevant code changes. The added warning against passing server-owned OAuth fields (
client_id,scope,redirect_uri,code_challenge,state,response_type) is sound defensive guidance and improves the docs; theadditionalParams: { prompt: 'select_account' }example correctly uses only a provider-specific hint. No secrets/PII introduced. - Performance: N/A — documentation only.
- Nice touch: the SSR example keeps
additionalParamsas a commented-out line (ssr-integration.md:165), which mirrors the SPA guidance without cluttering the primary flow.
Verdict: approved (informational)
No Critical findings. The single material item — whether the new positional signature and additionalParams match the shipped SDK — is a verification step the author is best positioned to confirm, not a defect I can demonstrate. Posting as a comment, not a block. (GitHub green-checkmark approval remains a separate human action.)
2 participants
What changed
signInWithOAuth(provider, options).additionalParamsto the SPA OAuth example for provider-specific params such as Googleprompt: 'select_account'.Why
The SDK OAuth init flow now supports provider-specific OAuth params through
additionalParams, while the wire format remains flat GET query params.Validation
rgconfirmed no remainingsignInWithOAuth({ provider, ... })examples underskills/insforge.git diff --checkpassed.Summary by cubic
Updated OAuth skill docs to use the provider-first
signInWithOAuth(provider, options)API and clarify the SSR flow withskipBrowserRedirect. Examples now showadditionalParamsand add warnings to avoid server-owned OAuth params.New Features
signInWithOAuth('google', { redirectTo, additionalParams })and warns against passing server-owned fields.additionalParams, andskipBrowserRedirect: true.Migration
signInWithOAuth({ provider, ... })withsignInWithOAuth(provider, {...}).additionalParamsonly for provider-specific hints (e.g., Googleprompt: 'select_account').Written for commit d3b6172. Summary will update on new commits.
Note
Update OAuth SDK skill examples and expand config management documentation
signInWithOAuthcall signature in sdk-integration.md and ssr-integration.md to take provider as a positional argument and options as a second parameter, with optionaladditionalParamssupport.insforge.tomlsections: auth verification flags, password policy, SMTP, storage upload size, realtime/schedule retention, and deployment subdomain.Changes since #90 opened
auth.allowed_redirect_urls[c582b30]additionalParamsusage in OAuth SDK methods [c582b30]Macroscope summarized d3b6172.
Summary by CodeRabbit