Skip to content

refactor: remove sessionRepositories and activitySinks maps; add issueTrackers TODOs (CYPACK-968)#992

Open
cyrusagent wants to merge 6 commits intomainfrom
cypack-968
Open

refactor: remove sessionRepositories and activitySinks maps; add issueTrackers TODOs (CYPACK-968)#992
cyrusagent wants to merge 6 commits intomainfrom
cypack-968

Conversation

@cyrusagent
Copy link
Copy Markdown
Contributor

@cyrusagent cyrusagent commented Mar 16, 2026

Summary

  • Removed redundant sessionRepositories: Map<string, string> from EdgeWorker — it was a lossy cache of session.repositories[0].repositoryId that only stored the primary repo
  • Deleted activitySinks: Map<string, IActivitySink> entirely — LinearActivitySink is a stateless wrapper around IIssueTrackerService, so sinks are now created on-the-fly via createActivitySink() helper at session registration time
  • Added TODO comments on issueTrackers (in both EdgeWorker.ts and CLAUDE.md) noting composite key needed for multi-platform support

Implementation approach

sessionRepositories removal

Each sessionRepositories.get(sessionId) call was replaced based on context:

  • Event handler callbacks: Used session from the event payload directly
  • Constructor callback and feedback delivery: Used this.agentSessionManager.getSession(id) to fetch the session first
  • Fallback resolution paths: Used firstSession.repositories[0]?.repositoryId
  • Repo deletion reverse-lookup: Used s.repositories.some() for correct multi-repo matching

activitySinks deletion

  • Removed initialization loop, repo addition sink creation, and repo deletion cleanup
  • Added createActivitySink(workspaceId) helper that looks up issueTracker and creates LinearActivitySink on-the-fly, falling back to NoopActivitySink
  • All session registration sites now call createActivitySink() instead of map lookup

Testing

  • All 628 edge-worker tests passing
  • Build succeeds across all packages
  • Linting clean, TypeScript type checking passes

Links

cyrusagent and others added 2 commits March 15, 2026 17:14
…K-968)

Delete the redundant sessionRepositories: Map<string, string> from
EdgeWorker, replacing all ~14 usage sites with direct reads from
session.repositories[0]?.repositoryId. The reverse-lookup in repo
deletion now uses session.repositories.some() which is more correct
for multi-repo sessions.

Also adds TODO comments documenting:
- activitySinks multi-workspace limitation for cross-workspace sessions
- issueTrackers composite key needed for multi-platform support

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@cyrusagent cyrusagent marked this pull request as ready for review March 16, 2026 00:16
…PACK-968)

Change activitySinks from Map<repoId, sink> to Map<workspaceId, sink>.
This eliminates redundant sink instances (one per workspace instead of
one per repo), decouples the lookup from which repo is "primary", and
makes the data model honest about what it represents.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@cyrusagent cyrusagent changed the title refactor: remove sessionRepositories map; add multi-repo TODOs (CYPACK-968) refactor: remove sessionRepositories; re-key activitySinks by workspace ID; add issueTrackers TODOs (CYPACK-968) Mar 16, 2026
cyrusagent and others added 2 commits March 15, 2026 17:38
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
LinearActivitySink is a thin, stateless wrapper around IIssueTrackerService.
Instead of maintaining a pre-built map, create sinks on-the-fly at session
registration time via createActivitySink() helper. Falls back to
NoopActivitySink when no issue tracker is available.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@cyrusagent cyrusagent changed the title refactor: remove sessionRepositories; re-key activitySinks by workspace ID; add issueTrackers TODOs (CYPACK-968) refactor: remove sessionRepositories and activitySinks maps; add issueTrackers TODOs (CYPACK-968) Mar 16, 2026
Comment on lines 2022 to 2025
const allActiveSessions = this.agentSessionManager.getActiveSessions();
const activeSessions = allActiveSessions.filter(
(s) => this.sessionRepositories.get(s.id) === repo.id,
const activeSessions = allActiveSessions.filter((s) =>
s.repositories.some((r) => r.repositoryId === repo.id),
);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is also wrong now that CYPACK-911 has been merged. There is no longer a requirement for sessions to be linked to repos, now that we have support for surfaces like Slack. This will also need to be changed once multi-repo support is added

Comment on lines +322 to +325
const repoId = this.sessionRepositories.get(childSessionId);
const childSession =
this.agentSessionManager.getSession(childSessionId);
const repoId = childSession?.repositories[0]?.repositoryId;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This will need to change once multi-repo session support is added. handleResumeParentSession should not require the repo now that CYPACK-911 has been merged. I presume it's needed to build the allowed tools for the session, but the repo config itself should not be passed to this method.

Comment on lines +346 to +349
const repoId = this.sessionRepositories.get(sessionId);
const repoId = session.repositories[0]?.repositoryId;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same thing with this.

Comment on lines +366 to +369
const repoId = this.sessionRepositories.get(sessionId);
const repoId = session.repositories[0]?.repositoryId;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And this

@PaytonWebber
Copy link
Copy Markdown
Collaborator

@cyrusagent look at the comments left on this PR as review and address them

@cyrusagent-staging
Copy link
Copy Markdown

This repository is not configured with Cyrus. Please add it at https://app.atcyrus.com/repos to start using Cyrus here.

1 similar comment
@ghost
Copy link
Copy Markdown

ghost commented Mar 16, 2026

This repository is not configured with Cyrus. Please add it at https://app.atcyrus.com/repos to start using Cyrus here.

@ghost
Copy link
Copy Markdown

ghost commented Mar 16, 2026

I can't connect to your local environment because the Cyrus process isn't running. Run cyrus in your terminal and keep it running, then mention me again.

@ghost
Copy link
Copy Markdown

ghost commented Mar 16, 2026

This repository is not configured with Cyrus. Please add it at https://app.atcyrus.com/repos to start using Cyrus here.

1 similar comment
@ghost
Copy link
Copy Markdown

ghost commented Mar 16, 2026

This repository is not configured with Cyrus. Please add it at https://app.atcyrus.com/repos to start using Cyrus here.

…internally (CYPACK-968)

Move repo resolution out of event handler callbacks and into the handler
methods themselves via a new resolveSessionRepo helper. This removes the
requirement for sessions to be linked to repos at the call site, aligning
with CYPACK-911 where sessions can exist without repos (e.g. Slack surfaces).

Also adds multi-repo TODOs for removeDeletedRepositories and the helper.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@cyrusagent
Copy link
Copy Markdown

cyrusagent bot commented Mar 16, 2026

Pushed. Here's a summary of the changes addressing all 4 review comments:

  1. Removed repo parameter from handleResumeParentSession (line 325) — The _childRepo param was already unused; removed the repo lookup from the callback entirely.

  2. Moved repo resolution into handleSubroutineTransition (line 349) — The method now resolves the repo internally instead of receiving it as a parameter.

  3. Moved repo resolution into handleValidationLoopFixer (line 369) — Same treatment. Also applied to handleValidationLoopRerun for consistency.

  4. Added multi-repo TODO on removeDeletedRepositories filter (line 2025) — Documented that when multi-repo sessions exist, we should detach only the removed repo rather than stopping the entire session.

All changes use a new resolveSessionRepo helper (with its own multi-repo TODO) that centralizes the session.repositories[0] pattern. Typecheck passes, all 628 edge-worker tests pass.

Comment on lines 341 to -366
@@ -363,21 +346,12 @@ export class EdgeWorker extends EventEmitter {
this.agentSessionManager.on(
"validationLoopIteration",
async ({ sessionId, session, fixerPrompt, iteration, maxIterations }) => {
const repoId = this.sessionRepositories.get(sessionId);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And this.

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.

2 participants