Skip to content

Conversation

@cte
Copy link
Collaborator

@cte cte commented Mar 11, 2025

Context

I ran into an annoying race condition in the evals in which we attempt to write to the secret state while it's already being asynchronously initialized, and the values I want to write get clobbered. This is caused by the ContextProxy's constructor kicking off an async function call w/o allowing us to await it.

Additionally, I don't see a reason to allow arbitrary key names in ContextProxy, so I added full type safety to it.

I also further simplified the RooCodeAPI which allowed me to clean up some of the e2e test code.

Implementation

Screenshots

before after

How to Test

Get in Touch


Important

Refactor ContextProxy to remove async constructor, enhance RooCodeAPI with new methods, and update tests and configurations accordingly.

  • ContextProxy:
    • Refactor ContextProxy to make constructor non-async and add initialize() method.
    • Add type safety for GlobalStateKey and SecretKey.
  • RooCodeAPI:
    • Add cancelTask() and setConfiguration() methods.
    • Remove setCustomInstructions() and getCustomInstructions().
  • Tests:
    • Update tests in contextProxy.test.ts and ClineProvider.test.ts to reflect changes in ContextProxy and RooCodeAPI.
    • Add subtasks.test.ts for testing subtask cancellation and resumption.
  • Misc:
    • Rename .env.integration to .env.local in code-qa.yml and related files.
    • Update knip.json to ignore e2e/** and other paths.

This description was created by Ellipsis for 4b6def5. It will automatically update as commits are pushed.

@changeset-bot
Copy link

changeset-bot bot commented Mar 11, 2025

🦋 Changeset detected

Latest commit: 80d1fae

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
roo-cline Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Mar 11, 2025
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rename this file for consistency.

@ellipsis-dev
Copy link
Contributor

ellipsis-dev bot commented Mar 11, 2025

The pull request involves changes across multiple areas, including CI updates, refactoring, and test updates. While these changes contribute to a common goal of improving the codebase, they span different areas of the project. Consider splitting the pull request into smaller, more focused pull requests if the changes are not tightly interconnected. For example, CI updates could be separated from refactoring tasks, and test updates could be handled in a separate pull request. This approach can help streamline the review process and ensure each set of changes receives the attention it deserves.

@dosubot dosubot bot added the bug Something isn't working label Mar 11, 2025
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We log this too much (it's really noisy in the evals). Let's only log an error if all attempts fail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a generated file? Or are we going to need to remember to keep it up to date?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There should be full type safety, so you will be forced to keep this up to date. This is intended to the be the source of truth, and is not generated. If you're noticing the redundancy between this type and the SECRET_KEYS constant, you can see how I handle the out-of-date problem with the following:

type CheckSecretKeysExhaustiveness = Exclude<SecretKey, (typeof SECRET_KEYS)[number]> extends never ? true : false

const _checkSecretKeysExhaustiveness: CheckSecretKeysExhaustiveness = true

Copy link
Collaborator Author

@cte cte Mar 11, 2025

Choose a reason for hiding this comment

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

(If you know of a better way to enumerate the possible keys without placing a constant in the .d.ts file LMK).

@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Mar 11, 2025
@cte cte force-pushed the cte/context-proxy-fixes branch from 00da3d4 to c10c85e Compare March 11, 2025 20:33
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Skipping this for now until we can fix it... CC @mrubens @cannuri

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good, it's been flaking for me

@cte cte force-pushed the cte/context-proxy-fixes branch from 0d46a91 to 4b6def5 Compare March 11, 2025 21:24
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Mar 12, 2025
@cte cte merged commit ae6903b into main Mar 12, 2025
11 checks passed
@cte cte deleted the cte/context-proxy-fixes branch March 12, 2025 15:16
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Mar 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants