Skip to content

refactor(agents): remove needsGitHubToken from agent definitions#609

Merged
zbigniewsobiecki merged 1 commit intodevfrom
feature/remove-needs-github-token
Mar 2, 2026
Merged

refactor(agents): remove needsGitHubToken from agent definitions#609
zbigniewsobiecki merged 1 commit intodevfrom
feature/remove-needs-github-token

Conversation

@aaight
Copy link
Copy Markdown
Collaborator

@aaight aaight commented Mar 2, 2026

Summary

Removes the redundant needsGitHubToken field from all built-in agent YAML definitions and the BackendSchema. GitHub token access is now automatically derived from whether scm appears in integrations.required.

Closes: https://trello.com/c/69a59d518718debdf3ea7056

Changes

  • src/agents/definitions/schema.ts: Changed needsGitHubToken: z.boolean() to z.boolean().optional() in BackendSchema for backward compatibility (existing DB blobs with the field will still parse without errors; the field is ignored at runtime)
  • src/agents/definitions/profiles.ts: Added requiresScmIntegration(def) helper that checks def.integrations?.required.includes('scm') with capability-derived fallback; updated buildProfileFromDefinition to use it instead of reading def.backend.needsGitHubToken
  • All 10 YAML definitions: Removed needsGitHubToken line; five YAMLs that had bare backend: null were updated to backend: {}
  • web/src/components/settings/agent-definition-editor.tsx: Removed the "Needs GitHub Token" toggle from the Backend Settings section and the needsGitHubToken: false default
  • 8 test files: Removed needsGitHubToken from AgentDefinition fixtures; updated loader.test.ts assertions to check def.integrations?.required instead of def.backend.needsGitHubToken

Key Decisions

  • AgentProfile.needsGitHubToken: boolean stays on the interfaceadapter.ts and secretBuilder.ts read it at runtime. Only the source of truth moves from the YAML to derivation logic.
  • Zod's optional() ensures no DB migration is needed — existing JSONB blobs with the field will parse correctly.
  • requiresScmIntegration() checks explicit integrations.required first, then falls back to capability-derived integrations.

Test Plan

  • All 218 unit test files pass (3727 tests)
  • TypeScript type check passes (0 errors)
  • Biome lint passes (0 errors)

🤖 Generated with Claude Code

@nhopeatall
Copy link
Copy Markdown
Collaborator

nhopeatall commented Mar 2, 2026

✨ On it — checking the PR changes now


Progress: [█░░░░░░░░░] 6% (iteration 4/70)

🔍 Code Review Update (1 min)

I've started my review by examining the requiresScmIntegration function and its dependencies, using squint symbols show to understand its structure and related symbols. I'm currently analyzing deriveRequiredIntegrations and buildProfileFromDefinition to ensure the changes are sound. My next step is to delve deeper into the key files involved in the PR.

Last updated: iteration 4 · review

Copy link
Copy Markdown
Collaborator

@nhopeatall nhopeatall left a comment

Choose a reason for hiding this comment

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

LGTM — Clean removal of a redundant field with correct derivation logic.

I verified all 10 YAML agents: every old needsGitHubToken value (5× true, 5× false) matches what requiresScmIntegration() now derives from integrations.required. The fallback path through deriveRequiredIntegrations() is a sensible safety net for definitions without explicit integrations.

Key things confirmed:

  • Backward compatibility: z.boolean().optional() in BackendSchema keeps existing DB JSONB blobs parsing cleanly — no migration needed.
  • Runtime contract preserved: AgentProfile.needsGitHubToken stays on the interface; adapter.ts and secretBuilder.ts continue consuming it unchanged.
  • Test updates are correct: loader.test.ts assertions properly shifted from def.backend.needsGitHubToken to def.integrations?.required checks. Test fixtures in other files correctly drop the now-optional field.
  • Frontend editor cleanup: Removing the "Needs GitHub Token" toggle makes sense since the value is now derived and shouldn't be manually overridden.
  • All CI checks pass (lint, unit tests, integration tests).

Copy link
Copy Markdown
Collaborator

@nhopeatall nhopeatall left a comment

Choose a reason for hiding this comment

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

LGTM — verified all 10 YAML agents produce identical needsGitHubToken values through derivation. Clean refactoring with correct backward compatibility.

@zbigniewsobiecki zbigniewsobiecki merged commit daec960 into dev Mar 2, 2026
6 checks passed
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