refactor: split agent bindings per review on #7 - #9
Merged
Conversation
One function per module, underscore prefix for internal modules, one spec per module. index.ts still decides the public surface, so nothing exported changes name and there is no behaviour change. claude/ - _resolveCredentials.ts (+ spec): resolveClaudeCredentials and the ClaudeCredentials type. The spec keeps its temp home on the context and cleans it up through withAspect(a_temp_home, remove_temp_home) instead of a module-level tempRoots array and afterEach. - _buildDockerArgs.ts (+ spec): buildClaudeDockerArgs. - _credentialsEnv.ts: credentialsEnv, now shared by _buildDockerArgs and run (was private to run.ts). - run.ts / run.spec.ts: runClaudeInDocker and its error helpers only. cursor/ - _buildDockerArgs.ts (+ spec): buildDockerArgs. - run.ts / run.spec.ts: runCursorInDocker only. Spec bodies were moved, not rewritten. Verified with the unit suite (52 tests, 16 files), the repo e2e, and a consumer project running the packed tarball against both agents. Refs: #7 (comment) #7 (comment) #7 (comment)
devzeebo
approved these changes
Aug 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the review on #7 (merged with changes requested).
What the review asked for
run.spec.ts:40— temp homes on the context, cleaned up throughwithAspectinstead of a module-leveltempRootsarray +afterEach. Done aswithAspect(a_temp_home, remove_temp_home): the before-aspect creates the temp home, eachgivendecides whether to put a credentials file in it. (withAspect's first argument is typed as required, so a no-opundefinedbefore-aspect would not type-check; creating the home there is the idiomatic version anyway.)run.ts:51—resolveClaudeCredentialsmoved toclaude/_resolveCredentials.tswith its own_resolveCredentials.spec.ts.run.ts:80—buildClaudeDockerArgsmoved toclaude/_buildDockerArgs.tswith its own spec, and the same for the Cursor agent:cursor/_buildDockerArgs.ts+ spec.One forced extra split:
credentialsEnvwas private torun.tsand is now needed by both_buildDockerArgs.tsandrun.ts, so it lives inclaude/_credentialsEnv.ts(covered through both callers' specs, as before).Layout after
Checks
pnpm run lint && pnpm run test && pnpm run build: clean; 52 tests, now across 16 files.buildClaudeDockerArgs,resolveClaudeCredentials,runClaudeInDocker,buildDockerArgs,runCursorInDocker, types); thee2e/suite and an external consumer importing only from the package barrel need no changes.Two questions, not changed here
_-prefixed modules are re-exported byindex.ts(resolveClaudeCredentials,buildClaudeDockerArgs,buildDockerArgs). If underscore is meant as "sibling-private" (the way gwt-runner uses it), the twobuild*DockerArgsexports are the odd ones out: neither is in the README exports table. Dropping them would be an API change, so it is left for a later minor if you agree.accesscheck + error message) is still inline incursor/run.ts, while the Claude equivalent now lives in_resolveCredentials.ts. Acursor/_resolveAuthFile.ts+ spec would make bothrun.tsfiles pure orchestration and give the Cursor error message its first unit test. Left out because the review comment on the Cursor side was aboutbuildDockerArgs.