Conversation
|
|
@greptileai Please re-review the latest commit d17ce80. It restores sandbox API-key-only callback registration after validation, keeps API-key and dashboard write paths exclusive, fails installation on callback registration failure, and removes unsupported Pages Router choices from CLI help and the picker. All CI checks pass; the current 3/5 summary still refers to 3366b2a. |
gjtorikian
left a comment
There was a problem hiding this comment.
Verified locally on d17ce80 with skills 0.7.3: bun run test (163 files / 2,824), typecheck, and lint all pass as described.
The dashboard path (read → dry-run → recheck → write → read-back, sandbox-only, no cross-target writes) is careful and well-tested. Two regressions undercut the PR's own goal, though:
- The new validation gate in
agent-runner.tsis not scoped to Next.js. The other rulesets were advisory and are stale; a correct modern TanStack Start install (per the bundled skill) now hard-fails. - With a dashboard session present, any precondition failure leaves the callback unregistered behind exit 0 — logged in to a different team than the API key, production env, stale sign-out/initiate-login values from another project, or any dashboard error. All of these worked pre-PR.
Both fixes are small and local; details inline, plus a few MEDIUM/LOW items.
5274ae8 to
f540e89
Compare
|
@greptileai Please re-review f540e89. The branch is now rebased onto the latest origin/main (7f6eac2), removing the already-merged SDK/dependency changes from the diff. The remaining runtime-router finding is fixed without widening the app-only type or CLI choices: unsupported supplied values fail before detection, provisioning, or writes, including in empty projects. Nine new regression cases fail before the fix and pass now; all 2,848 tests, typecheck, lint, changed-file formatting, build, and compiled smoke checks pass. All previously addressed review threads have been resolved. |
Summary
Rebased onto
origin/mainat7f6eac2, which includes merged #243 and@workos/skills@0.7.3. The SDK/dependency upgrade is no longer part of this PR's diff. The skills explain the required application settings; this PR makes the installer create the sign-in route and configure supported settings itself, rather than relying on its restricted agent to run management commands./sign-inroute using the SDK, separate from the callback. Include missing Next.js requirements in agent retries and fail validation instead of reporting incomplete code as installed. Other frameworks keep their existing advisory completeness checks; their security/build checks are unchanged.--homepage-urlexplicitly requests a change. Register and verify the callback before optional URL updates. Conflicting sign-out defaults or initiate-login URLs are left unchanged without blocking the callback or unrelated settings.Authentication and scope
callbackRegistered: truebutverified: false. Sign-out URI, Initiate login URI, and any requested homepage still need dashboard setup; browser flows remain untested.--router app; there is no Pages Router option in the interactive picker. Known Pages-only projects are declined before credential provisioning, including with--force. Explicit--router pagesis rejected during argument parsing; internal router options are also narrowed toapp. A shared runtime guard rejects unexpected values that bypass those type/parser checks, including before provisioning in an empty project. Preflight and installer use the same detection, sopages/api/alone is not mistaken for a Pages Router app. Mixed-router projects use App Router without modifying the Pages tree./account/sign-indoes not satisfy/sign-in. Existing pages at/sign-in, including within route groups, are refused before provisioning rather than overwritten; this installer currently requires that URL to be a dedicated route handler.--no-validateso the installer does not configure a missing route.Verification
bun run test: 2,848 tests passed across 163 files with published skills 0.7.3 installed.bun run typecheckandbun run lintpassed..pi/artifacts/cli-0-22-0-ship-message.html; it was left untouched and is not part of the PR.bun run buildpassed.workos internal verify-assets --jsonpassed and reported bundled skills 0.7.3.app, and compiled argument parsing rejects--router pages.install,integrate, anddashboardrefusing Pages Router, plus an existing route-group sign-in page being preserved before provisioning.Live smoke evidence
d17ce80, a fresh Next.js 16.3.5 project with isolated file-backed credentials completed the full compiled install with exit 0, no CLI login, andcallbackRegistered: true.5274ae8(rebased as13e5b3d), the callback setup helper was rerun against that same isolated unclaimed environment: callback registration passed, the CLI remained logged out, and full application URL verification correctly remained false. Existing account environments were not used.End-to-end browser sign-in/sign-out and password-reset/invitation flows have not been verified for this PR. Unit tests use mocked dashboard responses; they do not prove a live dashboard configuration succeeds.