This repository was archived by the owner on May 9, 2026. It is now read-only.
refactor(notion+gmail): unify auth lifecycle on start(), fix proxy /v1 prefix regression#12
Merged
Merged
Conversation
…re effectively. Introduced interfaces for start arguments, improved credential handling, and removed unused OAuth complete functions. Enhanced logging for better debugging and streamlined sync scheduling logic. Updated openhuman submodule to the latest commit.
…lidation and error handling. Introduced `validate` flag in start arguments to trigger API checks during authentication. Improved logging for better debugging and streamlined the overall structure of the code. Updated openhuman submodule to the latest commit for alignment with recent changes.
…zed state publishing and improving credential validation. Added `publishSkillState` and `start` functions for both skills to streamline state management and connection handling. Removed unused credential validation functions and refactored code for better readability and maintainability. This update enhances the overall functionality and user experience of the integrations.
…ns to streamline code and improve maintainability. This update enhances readability and focuses on essential functionality, aligning with recent improvements in credential handling and state management.
…zed OAuth validation functions. Updated validation logic to streamline credential checks and improve error handling during the authentication process. This refactor enhances code maintainability and aligns with recent improvements in state management.
…nt with recent changes and improvements in the project.
… maintainability. Removed unnecessary whitespace and standardized function signatures for credential validation. This update enhances overall code clarity and aligns with recent improvements in state management.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughCentralizes auth into a new start(args?) lifecycle, removes callback auth-completion hooks, extracts start and state-publish logic for Gmail and Notion into dedicated modules, makes test-harness runtime URL read env at call time, updates validators/tests, and advances the openhuman submodule pointer. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Orchestrator as Skill Orchestrator
participant Skill as Skill Module
participant State as State/DB
participant Cron as CronRegistry
Client->>Orchestrator: POST /start(args)
Orchestrator->>Skill: invoke start(args)
Skill->>Skill: validate credentials (oauth/self-hosted/text)
alt validation fails
Skill->>Orchestrator: return {status:'error', errors}
else validation succeeds
Skill->>State: persist config (credentialId, userEmail/workspace)
Skill->>Cron: unregister existing sync job
Skill->>Cron: register sync with interval
Skill->>State: publishSkillState()/publishState()
Skill->>Orchestrator: return {status:'complete', message?}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
… and OAuth validation. Introduced a dynamic `runtimeUrl` function for better flexibility in setting the skills runtime URL. Enhanced error handling in the Notion live test script to verify skill authentication status before proceeding with stress tests. These changes improve code maintainability and ensure more reliable integration with the Notion API.
4 tasks
senamakel
changed the title
…g a unified `start` function requirement. Updated the validation process to check for legacy hooks and ensure skills export the `start` function, which is now the primary entry point for OAuth and auth lifecycle events. This refactor improves clarity and maintainability of the authentication setup process.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/core/notion/helpers.ts (1)
125-127: Consider loggingproxyPathinstead ofpathfor OAuth proxy calls.Line 126 logs
pathfor all calls, but for OAuth proxy calls the actual request usesproxyPath(with/v1prefix). This could cause confusion when debugging proxy-related issues.Suggested fix
+ const logPath = notionAuth.type === 'proxy' ? `/v1${path}` : path; console.log( - `[notion][fetch] ${method} ${path} status=${response.status} (${elapsed}ms, ${bodyLen}b)` + `[notion][fetch] ${method} ${logPath} status=${response.status} (${elapsed}ms, ${bodyLen}b)` );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/notion/helpers.ts` around lines 125 - 127, The log currently prints the original request `path` which is misleading for OAuth proxy calls that actually use `proxyPath`; update the logging in the fetch/HTTP helper (the console.log that references `method`, `path`, `response.status`, `elapsed`, and `bodyLen`) to print `proxyPath` when it is defined/used (falling back to `path` otherwise) so proxy requests show the real URL used for the request.scripts/validate.mjs (1)
234-245: Consider strengthening the./startimport detection heuristic.Pattern 3 (
from\s+['"]\.\/start['"]) assumes that any import from./startimplies astartfunction is exported. This could produce false positives if a file imports something else from./start(e.g.,import { someHelper } from './start').A more robust check might combine this with verifying
startappears in the import specifiers or is re-exported:Suggested improvement
function hasStartLifecycle(allContent) { if (/export\s+function\s+start\s*\(/.test(allContent)) return true; if (/export\s*\{[^}]*\bstart\b[^}]*\}/.test(allContent)) return true; - if (/from\s+['"]\.\/start['"]/.test(allContent)) return true; + // Check for re-export from ./start: `export { start } from './start'` + // or import+re-export: `import { start } from './start'` + `export { start }` + if (/export\s*\{[^}]*\bstart\b[^}]*\}\s*from\s+['"]\.\/start['"]/.test(allContent)) return true; + if (/import\s*\{[^}]*\bstart\b[^}]*\}\s*from\s+['"]\.\/start['"]/.test(allContent)) return true; return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validate.mjs` around lines 234 - 245, The heuristic in hasStartLifecycle is too broad for the "./start" case: update the third check so it only returns true when the import/export actually mentions start (e.g., detect import specifiers like "import { start" or "import start" from './start', re-exports "export { start } from './start'", or wildcard re-exports "export * from './start'"); specifically modify the regex for the "./start" branch in hasStartLifecycle to require "start" in the import/export specifiers or allow a true re-export, ensuring we still catch re-exports and default/name imports that actually refer to start.src/core/notion/start.ts (1)
22-30: Reuse the shared lifecycle types here.
NotionStartArgsandStartResultduplicate the new contract intypes/skill.d.ts, andsrc/core/gmail/start.tsnow carries the same copy. ReusingSkillStartArgs/SkillStartResultwill keep future host-side lifecycle changes from drifting across skills.As per coding guidelines: Keep concerns separated into dedicated files and folders. Use modular file layout with separate files for types, state, setup, sync, database schema/helpers, API calls per domain, tools per function, and update handlers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/notion/start.ts` around lines 22 - 30, The NotionStartArgs and StartResult types duplicate the shared contract—replace these local types with imports of SkillStartArgs and SkillStartResult from the central types/skill.d.ts and update any usages in this file (and ensure consistency with src/core/gmail/start.ts) to refer to those imported names; remove the duplicate interfaces NotionStartArgs and StartResult and export or re-export the shared types as needed so the skill uses the canonical lifecycle types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/gmail/publish-state.ts`:
- Around line 36-37: The code unconditionally calls new Date(...).toISOString()
on s.syncStatus.lastSyncTime and s.syncStatus.nextSyncTime which throws if the
value is undefined (and yields epoch for 0); update the mapping in
src/core/gmail/publish-state.ts so that lastSyncTime and nextSyncTime are only
converted when truthy (e.g., check s.syncStatus.lastSyncTime and
s.syncStatus.nextSyncTime) and otherwise set to null/undefined (or omit),
mirroring the Notion publish-state pattern; update references to lastSyncTime,
nextSyncTime and s.syncStatus accordingly.
In `@src/core/gmail/start.ts`:
- Around line 229-242: When args.validate is true ensure unknown auth.mode
values are rejected: inside the validation block that checks auth and auth.mode,
add an explicit else branch that sets validationError to an error describing
"unsupported auth mode" (or missing/invalid credentials) when auth.mode is
present but not 'self_hosted', 'text', or 'managed'; also handle the case where
args.auth exists but auth.mode is undefined/invalid by treating it as an invalid
mode. Update the logic surrounding validateGmailSelfHosted, validateGmailText,
and validateGmailOAuth so validationError is non-null for unsupported modes,
preventing connected from becoming truthy with unusable credentials.
- Around line 211-218: The OAuth handling currently only sets s.config.userEmail
when it is blank, so when reconnecting a different account the previous
userEmail remains; update the block that processes args.oauth (the oauthCred
handling in start.ts) to always overwrite s.config.userEmail when
oauthCred.accountLabel is present (remove the if (!s.config.userEmail) guard)
and persist with state.set('config', s.config); also remove the identical guard
inside loadGmailProfile() so that loadGmailProfile() will likewise refresh the
stored userEmail when the OAuth account changes.
In `@src/core/notion/live-test-stress.ts`:
- Around line 257-258: The code uses optional chaining when reading
connection_status into connState; replace "(verify.state as {
connection_status?: string } | undefined)?.connection_status" with an explicit
null/undefined check: first cast or access verify.state, check if verify &&
verify.state (or if verify.state !== undefined/null) then read
verify.state.connection_status into connState, then compare connState !==
'connected'. Update the variable access around connState, verify.state, and
connection_status to avoid any "?.", ensuring the same behavior when state is
missing.
In `@src/core/notion/start.ts`:
- Around line 43-46: The code only sets
getNotionSkillState().config.workspaceName when the existing value is empty,
which prevents updating the stored workspace label on reconnection; change the
logic in the OAuth validation block (where the local variable name is derived
from user.name) to assign getNotionSkillState().config.workspaceName = name
whenever name is defined (remove the "only if empty" guard) so the workspaceName
refreshes on reconnects; apply the same unconditional-overwrite fix to the spot
that copies accountLabel (the code that currently copies accountLabel only if
blank) so credential changes replace the stored label.
- Around line 106-114: The parsing assumes a `results` array but the
`/v1/users/me` endpoint returns a single user object; update the parsing logic
in the token validation block that reads `response.body` so it handles both
shapes: if the parsed `data` has a top-level `type` (e.g., data.type === 'bot')
use `data.name` to set getNotionSkillState().config.workspaceName, otherwise
fall back to the existing `results` array logic (find u.type === 'bot'). Ensure
this change touches the block that defines `botUser` and assigns to
getNotionSkillState().config.workspaceName so metadata extraction works for the
`/users/me` response.
---
Nitpick comments:
In `@scripts/validate.mjs`:
- Around line 234-245: The heuristic in hasStartLifecycle is too broad for the
"./start" case: update the third check so it only returns true when the
import/export actually mentions start (e.g., detect import specifiers like
"import { start" or "import start" from './start', re-exports "export { start }
from './start'", or wildcard re-exports "export * from './start'"); specifically
modify the regex for the "./start" branch in hasStartLifecycle to require
"start" in the import/export specifiers or allow a true re-export, ensuring we
still catch re-exports and default/name imports that actually refer to start.
In `@src/core/notion/helpers.ts`:
- Around line 125-127: The log currently prints the original request `path`
which is misleading for OAuth proxy calls that actually use `proxyPath`; update
the logging in the fetch/HTTP helper (the console.log that references `method`,
`path`, `response.status`, `elapsed`, and `bodyLen`) to print `proxyPath` when
it is defined/used (falling back to `path` otherwise) so proxy requests show the
real URL used for the request.
In `@src/core/notion/start.ts`:
- Around line 22-30: The NotionStartArgs and StartResult types duplicate the
shared contract—replace these local types with imports of SkillStartArgs and
SkillStartResult from the central types/skill.d.ts and update any usages in this
file (and ensure consistency with src/core/gmail/start.ts) to refer to those
imported names; remove the duplicate interfaces NotionStartArgs and StartResult
and export or re-export the shared types as needed so the skill uses the
canonical lifecycle types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69db152d-f342-42a1-a5c6-50a342b7df5e
📒 Files selected for processing (12)
dev/test-harness/index.tsopenhumanscripts/validate.mjssrc/core/gmail/index.tssrc/core/gmail/publish-state.tssrc/core/gmail/start.tssrc/core/notion/helpers.tssrc/core/notion/index.tssrc/core/notion/live-test-stress.tssrc/core/notion/publish-state.tssrc/core/notion/start.tstypes/skill.d.ts
…l and Notion skill authentication by refining validation logic for credential handling. Improved error handling in the `start` functions and ensured consistent state management across skills. These changes enhance code clarity and maintainability while aligning with recent improvements in the project.
…th` and `start` function signatures to use more specific types. This change enhances type safety and improves code clarity, aligning with recent improvements in the project's authentication logic.
…consistency. Updated test cases to handle asynchronous operations in skill lifecycle methods and tool calls, enhancing error handling and ensuring proper cleanup after tests. This change aligns with recent improvements in the project's testing framework.
…solidated assertion logic to enhance clarity while maintaining functionality. This change aligns with ongoing efforts to improve code quality and maintainability in the testing framework.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/validate.mjs`:
- Around line 240-249: The helper hasStartLifecycle currently scans any file
content and treats imports/exports from './start' as satisfying the lifecycle;
restrict this by changing hasStartLifecycle to accept an optional filename (or
update callers) and only consider imports/exports from './start' as valid when
the examined file is the skill entry (e.g., its basename is index.ts, index.tsx,
index.js, or index.jsx), otherwise only count explicit exported function/ named
export occurrences; update calls in validateSetupFlow to pass the file name or
run hasStartLifecycle only against the entry file content so private helper
files can't falsely satisfy the contract.
In `@src/core/gmail/__tests__/test-gmail.ts`:
- Around line 53-57: Replace empty catch blocks used in test teardown hooks
(e.g., the afterAll wrapping stopSkill(SKILL_ID) and the other similar try/catch
blocks) with minimal handling to satisfy no-empty: either add a concise comment
like // ignore errors during cleanup or log the error (e.g.,
process.stderr.write or console.debug) inside each catch. Update the catch
bodies for the occurrences around the afterAll at startLine 53 and the other
blocks referenced (around lines 65-70, 80-84, 92-97, 137-141) so they are
non-empty while keeping semantics unchanged.
In `@src/core/gmail/start.ts`:
- Around line 205-212: The code currently mutates s.config and calls
state.set('config') before running validateGmailOAuth(), which can persist
invalid OAuth data; instead, keep the incoming oauth values in local temporaries
(e.g., tempCredentialId, tempAccountLabel) and pass them to
validateGmailOAuth(); only if validateGmailOAuth() succeeds assign them to
s.config and call state.set('config'); apply the same change for the later block
that updates s.config (the other occurrence referenced around validateGmailOAuth
usage).
In `@src/core/notion/start.ts`:
- Around line 136-145: The code writes oauthCred (credentialId/accountLabel)
into s.config and calls state.set('config', s.config) before validation;
instead, move the state.set call (and any mutation of persisted config) to after
validateNotionOAuth() succeeds in start(), so update s.config locally when
args.oauth is present but only persist via state.set('config', s.config) after
validateNotionOAuth() returns successfully; apply the same change for the
duplicate oauth-handling block that also writes state (the other block around
validateNotionOAuth usage) to ensure rejected credentials are never persisted.
- Around line 156-194: The code currently treats any auth with a non-'managed'
mode as direct and skips validation when auth.mode is missing, allowing cron
registration with invalid/unknown modes; update the start logic to explicitly
handle supported modes (e.g., 'managed' and 'direct'), reject missing or unknown
auth.mode before proceeding, and only set connected/allow cron.register when
validation passes: in start.ts adjust the auth/validation block (references:
args.auth, auth.mode, validateNotionAuthDirect, validateNotionOAuth,
validationError, connected, cron.register) so that if auth is present but
auth.mode is undefined or not in the allowed set you assign a validationError
(or return it), otherwise call validateNotionOAuth for 'managed' and
validateNotionAuthDirect for 'direct', and ensure connected is derived only
after successful validation to prevent scheduling 'notion-sync' when auth is
invalid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37fef112-e2f1-489b-a0fc-255be0a51a4f
📒 Files selected for processing (9)
openhumanscripts/validate.mjssrc/core/gmail/__tests__/test-gmail.tssrc/core/gmail/api/helpers.tssrc/core/gmail/publish-state.tssrc/core/gmail/start.tssrc/core/notion/helpers.tssrc/core/notion/live-test-stress.tssrc/core/notion/start.ts
✅ Files skipped from review due to trivial changes (1)
- openhuman
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/notion/helpers.ts
- src/core/gmail/publish-state.ts
…nt with recent changes and improvements in the project.
…cycle` function to ensure proper checks for exported `start` functions in entry files. Updated the `start` functions in Gmail and Notion skills to temporarily store OAuth metadata until validation succeeds, preventing overwrites of existing credentials. Improved error handling in test cases to ignore cleanup errors. These changes enhance code clarity, maintainability, and ensure robust credential management across skills.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
start.tsper skill: one entry point that picks up credentials, optionally validates them against the upstream API, and registers cron — replacing the oldonOAuthComplete/onAuthCompletehooks./v1proxy-prefix regression insrc/core/notion/helpers.ts: PR Notion and Gmail Auth Fetch Changes #11 dropped the/v1prefix fromoauth.fetch()calls assuming the encrypted-proxy backend prependsapiBaseUrlfrom the manifest. It does not — it forwards paths verbatim. Result: every Notionoauth.fetch('/users/me')reached Notion as/users/meand came back 400 "Invalid request URL", which madeoauth/completevalidation fail and rolled back the credential. This was the root cause of the "Notion not connected" cascade reported in #484.runtimeUrl()is now a getter (re-evaluated per call so live scripts can flipSKILLS_RUNTIME_URLafter import) and defaults to port 7799 to matchopenhuman-core skills run --port 7799.src/core/notion/live-test-stress.ts) now inspects theoauth/completeresult, fails fast on{status:'error'}with the validation error and a pointer tolive-test.tsfor fresh OAuth, and verifiesstate.connection_status === 'connected'before kicking off the stress phase.publish-state.tsin both skills extracts thestate.setPartialcontract sostart.tsandindex.tsshare the same shape without re-introducing a circular import.types/skill.d.tsupdated for the newSkill.start: (args?: SkillStartArgs) => MaybeAsync<SkillStartResult | void>signature;onOAuthComplete/onAuthCompleteremoved from the interface.openhumansubmodule to pull in the matching Rust-side rewrite (refactor(skills): unify auth/oauth handshake on start({validate}) and dropenabledflag openhuman#484).Problem
Two long-standing issues collided here:
onOAuthComplete,onAuthComplete,start) and two parallel "is the skill on?" flags upstream (enabledvssetup_complete). The handshake was order-dependent and easy to half-finish — credential persisted to disk, but cron never registered, or cron registered against an upstream the API rejects on the next call./v1prefix fromoauth.fetchcalls innotionFetch, claiming "path is relative to manifest `apiBaseUrl`, same as Gmail." The encrypted-proxy backend doesn't actually prependapiBaseUrl— it forwards paths verbatim — so every Notion proxy call started returning Notion's400 invalid_request_url. The stress test never noticed because it ignored theoauth/completereturn value and printedOKwhenever the RPC didn't throw.Solution
src/core/notion/start.ts(new) andsrc/core/gmail/start.ts(new): own credential pickup, validation, cron registration, andpublishState(). Both validators (validateNotionOAuth/validateGmailOAuthetc.) hit the upstream API the same way the runtime will at tool-call time, so a bad credential fails the handshake instead of poisoning the activated state.src/core/notion/publish-state.ts(new) andsrc/core/gmail/publish-state.ts(new): singlepublishState()/publishSkillState()function shared betweenstart.tsandindex.ts, breaking the circular import that previously kept this logic inlined.src/core/notion/index.ts/src/core/gmail/index.ts: stripped of inlinestart, validators,onOAuthComplete,onAuthComplete, and the duplicated state-publish helpers.src/core/notion/helpers.ts:106: restore\/v1${path}`` on the proxy path with a comment explaining why the backend can't prepend it (so a future drive-by refactor doesn't reintroduce the same regression).dev/test-harness/index.ts:28:runtimeUrl()getter, default127.0.0.1:7799. Re-evaluating per call lets live-test scripts overrideSKILLS_RUNTIME_URLafter the harness has been imported.src/core/notion/live-test-stress.ts: inspectoauthCompleteresult; on{status:'error'}print the validation errors and exit non-zero with a pointer tonpx tsx src/core/notion/live-test.ts(which has the interactive OAuth flow that mints fresh credentials). Adds a "Verifying connection..." step that checksgetSkillStatus(...).state.connection_status === 'connected'before the warmup/stress phase so a silent rollback can never go undetected again.types/skill.d.ts: addSkillStartArgs/SkillStartResult; broadenSkill.startto accept the credential bag and return a status; droponOAuthComplete/onAuthComplete.Submission Checklist
npx tsx src/core/notion/live-test-stress.ts 12→ 12/12 passed acrosslist-users,list-pages,list-databases,search(avg 2504ms, p95 6174ms).npx tsx src/core/notion/live-test.tswalks the full happy path (start → oauth → connect → tools → sync) end-to-end against the real Rust runtime.yarn typecheckclean.yarn buildregenerates the bundledskills/notion/index.jsandskills/gmail/index.jswith the new/v1prefix path.~/.openhuman/users/<active>/auth-profiles.jsonvia the harness, not the env.start.tsandpublish-state.tsfiles lead with a header explaining the contract; the/v1proxy comment inhelpers.tscalls out why the prefix is required.Impact
enabledflag openhuman#484 — old runtimes will still call the removedonOAuthComplete/onAuthCompletehooks (which now no-op cleanly because they're undefined on the JS side, but without the validate-then-persist contract).oauth/completeloudly with{status:'error', errors:[...]}instead of silently persisting and breaking on the first tool call. Existing UI code that ignored the result will need to surface the error.Related
enabledflag openhuman#484/v1regression: Notion and Gmail Auth Fetch Changes #11 "Notion and Gmail Auth Fetch Changes"src/core/gmail/live-test*.tsonce we settle on the harness pattern.Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores