Skip to content

Split PR #522 [4/9]: Admin scripts package#560

Draft
seanspeaks wants to merge 1 commit intonextfrom
claude/split-522-04-admin-scripts
Draft

Split PR #522 [4/9]: Admin scripts package#560
seanspeaks wants to merge 1 commit intonextfrom
claude/split-522-04-admin-scripts

Conversation

@seanspeaks
Copy link
Copy Markdown
Contributor

Summary

Stack

This is PR 4 of 9 splitting #522 into reviewable chunks:

  1. Docs → claude/split-522-01-docs
  2. Schemas → claude/split-522-02-schemas
  3. Core → claude/split-522-03-core
  4. Admin Scripts ← you are here
  5. AI Agents → claude/split-522-05-ai-agents
  6. UI Library → claude/split-522-06-ui-lib
  7. CLI → claude/split-522-07-cli
  8. Management UI → claude/split-522-08-management-ui
  9. E2E & Lockfile → claude/split-522-09-e2e-and-lockfile

Test plan

  • Run admin-scripts tests
  • Verify imports from @friggframework/core resolve

https://claude.ai/code/session_01D3qzNKrmQXgSJoV6BJkcbQ

Split 4/9 of PR #522 (integration-router-v2).
Contains the Admin Script Runner service:
- Script execution infrastructure (runner, registry, base class)
- Hybrid scheduling (EventBridge + local adapters)
- Dry-run mode with repository wrapper and HTTP interceptor
- Admin router, SQS worker handler, builder

Depends on: core (PR 3) for repository factories and queue utilities.
@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot

See analysis details on SonarQube Cloud

this._validateScriptExists(scriptName);

// Delete from database
const deleteResult = await this.commands.deleteSchedule(scriptName);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably try/catch that.

* This use case encapsulates the business logic that was previously
* embedded in the router, reducing cognitive complexity and improving testability.
*/
class ScheduleManagementUseCase {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not a good use case name, a use case is usually an intention.

results: []
};

this.context.log('info', 'Starting integration health check', {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this.context?

@d-klotz d-klotz changed the base branch from claude/split-522-03-core to next April 8, 2026 09:17
@d-klotz
Copy link
Copy Markdown
Contributor

d-klotz commented Apr 8, 2026

Code review

Found 2 issues (both runtime crashes):

  1. commands.findRecentExecutions() does not exist -- runtime TypeError on GET /scripts/:scriptName/executions. The router calls commands.findRecentExecutions({...}) but the actual method in @friggframework/core admin-script-commands (from PR Split PR #522 [3/9]: Core framework (DDD refactor) #559) is findRecentAdminProcesses() (or findAdminProcessesByName() for script-specific filtering). The test mocks findRecentExecutions on the mock object, masking the mismatch. This is the same category of bug found in PR Split PR #522 [3/9]: Core framework (DDD refactor) #559 (wrong method names passing through because tests mock the nonexistent method).

const executions = await commands.findRecentExecutions({
scriptName,
status,
limit: Number.parseInt(limit, 10),
});

  1. integrationRepository.findIntegrations({}) does not exist on the repository interface -- both builtin scripts crash. Both IntegrationHealthCheckScript and OAuthTokenRefreshScript call this.context.integrationRepository.findIntegrations({}) in their getAllIntegrations() helper, but IntegrationRepositoryInterface has no findIntegrations() method. Available methods are: findIntegrationsByUserId, findIntegrationByName, findIntegrationById, findIntegrationByUserId. This will throw TypeError: ...findIntegrations is not a function whenever these scripts run without explicit integrationIds.

async getAllIntegrations() {
return this.context.integrationRepository.findIntegrations({});
}

// In production, would need pagination for large datasets
return this.context.integrationRepository.findIntegrations({});
}


Additional notes (scored just below the reporting threshold but worth flagging given this is part of the same #522 split that had extensive issues in PR #559):

  • schedule-management-use-case.js (~230 lines) is dead code -- never imported by any file. The router uses the 3 split use cases instead. Should be deleted.
  • The router's POST /scripts/:scriptName async execution path calls commands.createAdminProcess() and QueuerUtil.send() directly, bypassing use cases -- same "handlers call repositories directly" pattern flagged in PR Split PR #522 [3/9]: Core framework (DDD refactor) #559 (CLAUDE.md Golden Rule).
  • oauth-token-refresh.js checks integration.config?.credentials?.access_token but OAuth tokens in Frigg live on separate Credential documents, not Integration.config -- the refresh script will be a no-op in practice.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants