packages/workflows/src/dag-executor.test.ts (1)

1385-1425: ⚡ Quick win

Harden this test against cascading failures by restoring the spy in finally and asserting the call exists first.

If an assertion fails before Line 1424, execFileAsync remains mocked and can pollute later tests. Also, dereferencing mock.calls[0] before asserting invocation can throw a less helpful error.

Proposed patch

   it('passes user message through env vars, not string substitution, preventing shell injection', async () => {
     const execSpy = spyOn(git, 'execFileAsync').mockResolvedValue({ stdout: 'ok\n', stderr: '' });
-    const mockDeps = createMockDeps();
-    const platform = createMockPlatform();
-    const workflowRun = makeWorkflowRun('bash-shell-safe-run-id', {
-      workflow_name: 'bash-shell-safe',
-      conversation_id: 'conv-shell-safe',
-      user_message: '$(rm -rf /)',
-    });
-
-    const bashNode: BashNode = {
-      id: 'safe',
-      bash: 'echo $USER_MESSAGE',
-    };
-
-    await executeDagWorkflow(
-      mockDeps,
-      platform,
-      'conv-shell-safe',
-      testDir,
-      { name: 'bash-shell-safe-test', nodes: [bashNode] },
-      workflowRun,
-      'claude',
-      undefined,
-      join(testDir, 'artifacts'),
-      join(testDir, 'logs'),
-      'main',
-      'docs/',
-      minimalConfig
-    );
-
-    // The script passed to bash -c must contain literal $USER_MESSAGE (not substituted)
-    const bashArgs = execSpy.mock.calls[0]?.[1] as string[];
-    expect(bashArgs[1]).toBe('echo $USER_MESSAGE');
-
-    // The env must contain the user message
-    const envArg = (execSpy.mock.calls[0]?.[2] as { env: NodeJS.ProcessEnv }).env;
-    expect(envArg?.USER_MESSAGE).toBe('$(rm -rf /)');
-    expect(envArg?.ARGUMENTS).toBe('$(rm -rf /)');
-
-    execSpy.mockRestore();
+    try {
+      const mockDeps = createMockDeps();
+      const platform = createMockPlatform();
+      const workflowRun = makeWorkflowRun('bash-shell-safe-run-id', {
+        workflow_name: 'bash-shell-safe',
+        conversation_id: 'conv-shell-safe',
+        user_message: '$(rm -rf /)',
+      });
+
+      const bashNode: BashNode = {
+        id: 'safe',
+        bash: 'echo $USER_MESSAGE',
+      };
+
+      await executeDagWorkflow(
+        mockDeps,
+        platform,
+        'conv-shell-safe',
+        testDir,
+        { name: 'bash-shell-safe-test', nodes: [bashNode] },
+        workflowRun,
+        'claude',
+        undefined,
+        join(testDir, 'artifacts'),
+        join(testDir, 'logs'),
+        'main',
+        'docs/',
+        minimalConfig
+      );
+
+      expect(execSpy).toHaveBeenCalledTimes(1);
+      const firstCall = execSpy.mock.calls[0];
+
+      // The script passed to bash -c must contain literal $USER_MESSAGE (not substituted)
+      const bashArgs = firstCall?.[1] as string[] | undefined;
+      expect(bashArgs?.[1]).toBe('echo $USER_MESSAGE');
+
+      // The env must contain the user message
+      const envArg = (firstCall?.[2] as { env?: NodeJS.ProcessEnv } | undefined)?.env;
+      expect(envArg?.USER_MESSAGE).toBe('$(rm -rf /)');
+      expect(envArg?.ARGUMENTS).toBe('$(rm -rf /)');
+    } finally {
+      execSpy.mockRestore();
+    }
   });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/workflows/src/dag-executor.test.ts` around lines 1385 - 1425, The
test currently mocks git.execFileAsync as execSpy and then dereferences
execSpy.mock.calls[0] without first asserting the spy was called and it restores
the mock only at the end; modify the test to (1) wrap the assertions and
call-site dereference in a try/finally so execSpy.mockRestore() always runs, and
(2) assert execSpy.mock.calls.length > 0 (or expect(execSpy).toHaveBeenCalled())
before reading execSpy.mock.calls[0]; update references to the captured call to
use a local variable only after that assertion. Ensure this change targets the
test block that calls executeDagWorkflow and references execSpy,
git.execFileAsync, and bashArgs/envArg.

packages/core/src/handlers/clone.ts (4)

59-70: 💤 Low value

JSDoc for resolveForgeAuth is detached from the function.

Lines 59–62 document resolveForgeAuth, but SELF_HOSTED_FORGE is declared between the comment and the function, so tooling/readers will associate the JSDoc with the wrong symbol. Move the function-level doc directly above export function resolveForgeAuth(...).

📝 Suggested reorder

-/**
- * Resolve forge-specific authentication token and URL scheme for a repository URL.
- * Returns the token and auth scheme prefix, or empty values if no token is available.
- */
-/** Well-known self-hosted hostname label patterns → env var + scheme. */
+/** Well-known self-hosted hostname label patterns → env var + scheme. */
 const SELF_HOSTED_FORGE: { label: string; envVar: string; scheme: string }[] = [
   { label: 'gitlab', envVar: 'GITLAB_TOKEN', scheme: 'oauth2:' },
   { label: 'gitea', envVar: 'GITEA_TOKEN', scheme: '' },
   { label: 'forgejo', envVar: 'GITEA_TOKEN', scheme: '' },
 ];

+/**
+ * Resolve forge-specific authentication token and URL scheme for a repository URL.
+ * Returns the token and auth scheme prefix, or empty values if no token is available.
+ */
 export function resolveForgeAuth(url: string): { token: string | undefined; scheme: string } {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/handlers/clone.ts` around lines 59 - 70, The JSDoc for
resolveForgeAuth is separated from the function by the SELF_HOSTED_FORGE
constant, so tooling will attach the comment to the wrong symbol; move the
function-level comment block that starts "/** Resolve forge-specific
authentication..." so it sits immediately above "export function
resolveForgeAuth(url: string): { token: string | undefined; scheme: string }"
(leave the SELF_HOSTED_FORGE declaration in place), ensuring the doc comment
directly precedes the resolveForgeAuth declaration.

---

47-57: ⚡ Quick win

Misleading JSDoc on FORGE_AUTH — references a non-existent exact field.

The comment describes Entries with exact: true vs Entries with exact: false semantics, but ForgeAuthEntry has no exact property; matching is split between this array (always exact equality on hostPattern) and the separate SELF_HOSTED_FORGE table (label-based). Either align the comment with the actual implementation or add the exact flag.

📝 Suggested doc fix

-/**
- * Known forge authentication mappings.
- * Entries with `exact: true` match the full hostname.
- * Entries with `exact: false` match if the hostname contains the pattern as a label.
- * Order matters: exact matches are checked first.
- */
+/**
+ * Known forge authentication mappings.
+ * Each entry matches when the URL's hostname equals `hostPattern` exactly.
+ * Self-hosted/label-based matching is handled separately by `SELF_HOSTED_FORGE`.
+ */

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/handlers/clone.ts` around lines 47 - 57, The JSDoc for
FORGE_AUTH is incorrect: it mentions an `exact` field that doesn't exist on
ForgeAuthEntry and misstates matching behavior; update the comment to state that
entries in FORGE_AUTH are matched by exact hostname equality (hostPattern must
equal the request host) while label/substring matching is handled by the
separate SELF_HOSTED_FORGE table, or alternatively add and wire an `exact`
boolean to ForgeAuthEntry and matching logic — reference FORGE_AUTH,
SELF_HOSTED_FORGE, and ForgeAuthEntry to locate the code and choose one of those
two fixes to make the documentation and implementation consistent.

---

92-103: 💤 Low value

Label-match semantics: confirm intent for ambiguous multi-forge hostnames.

The self-hosted matcher uses labels.includes(entry.label) and iterates SELF_HOSTED_FORGE in declaration order. A hostname like gitlab.gitea.example.com will resolve to GITLAB_TOKEN (first match wins) even though gitea is also a label; conversely, a host that legitimately uses both products in DNS labels can never resolve to the second one. Two follow-up considerations:

• Document/confirm that the first-listed forge always wins on ambiguity (current order: gitlab → gitea → forgejo).
• Consider returning early only when the env var is set, so a missing GITLAB_TOKEN on gitlab.gitea.example.com falls through to try GITEA_TOKEN instead of giving up:

📝 Optional: fall through on missing token

   for (const entry of SELF_HOSTED_FORGE) {
     if (labels.includes(entry.label)) {
       const token = process.env[entry.envVar];
       if (token) {
         return { token, scheme: entry.scheme };
       }
-      return { token: undefined, scheme: '' };
+      // Try the next label-forge mapping if this token isn't set.
+      continue;
     }
   }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/handlers/clone.ts` around lines 92 - 103, The current
self-hosted matcher iterates SELF_HOSTED_FORGE and uses
labels.includes(entry.label) to pick the first matching entry, but it returns
immediately even when the matched entry's env var (process.env[entry.envVar]) is
unset, preventing later matches (e.g., gitlab.gitea.example.com always choosing
gitlab). Change the logic in the loop over SELF_HOSTED_FORGE so that you only
return when the env var is defined (token present) and otherwise continue to the
next entry; also add a short comment documenting that resolution is
first-match-by-order and that order in SELF_HOSTED_FORGE determines priority
(gitlab → gitea → forgejo) so maintainers know why ordering matters.

---

326-339: ⚡ Quick win

Silent http:// → https:// upgrade and dropped URL components when injecting auth.

When safeParseUrl succeeds, the rebuilt cloneUrl always uses https:// and only re-emits parsed.host + parsed.pathname. Two effects worth confirming are intentional:

1. An http:// clone URL is silently upgraded to https:// once a token is present. This is usually desirable security-wise but can break tests/internal forges that intentionally use plaintext; consider either documenting this as deliberate or preserving parsed.protocol.
2. parsed.search and parsed.hash are dropped. For git clone these are rare, but if a user passes e.g. ?ref=... it silently disappears once auth is injected and not otherwise.

If both are intentional, a one-line comment near Line 333 documenting the upgrade/component-drop would meet the “document fallback behavior with a comment when intentional and safe” guideline.

📝 Optional preservation

-    if (parsed) {
-      cloneUrl = `https://${authScheme}${forgeToken}@${parsed.host}${parsed.pathname}`;
+    if (parsed) {
+      // Intentional: force https when embedding a token; preserve path/search/hash.
+      cloneUrl = `https://${authScheme}${forgeToken}@${parsed.host}${parsed.pathname}${parsed.search}${parsed.hash}`;
     } else if (!workingUrl.startsWith('http')) {

As per coding guidelines: "Document fallback behavior with a comment when intentional and safe; otherwise throw."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/handlers/clone.ts` around lines 326 - 339, The current
auth-injection in the clone URL (inside the block using resolveForgeAuth and
safeParseUrl) silently upgrades any http:// to https:// and drops parsed.search
and parsed.hash; update the logic so that when safeParseUrl(parsed) returns a
URL you reconstruct cloneUrl preserving parsed.protocol (fall back to https:
only for bare host/path case), and append parsed.search and parsed.hash after
parsed.pathname while still injecting `${authScheme}${forgeToken}@` between the
protocol and host; if instead the upgrade/drop is intentional, add a one-line
comment next to the cloneUrl rebuild explaining that protocol is intentionally
upgraded to https and query/hash are discarded when injecting auth.

packages/core/src/handlers/clone.test.ts (2)

311-348: ⚡ Quick win

Consider adding an http-only-forge case to lock down the https upgrade behavior.

The new tests cover token injection with https:// URLs and the path-only false-positive, but not the implicit http:// → https:// upgrade in clone.ts Line 333. Given the security-sensitive nature of this PR, an explicit test (e.g., await cloneRepository('http://gitlab.mycompany.com/owner/repo') with GITLAB_TOKEN set, asserting the final clone URL starts with https://oauth2:...) would pin the current behavior and catch any future regression that silently leaks tokens over plaintext.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/handlers/clone.test.ts` around lines 311 - 348, Add a new
test case in clone.test.ts that mirrors the existing GitLab token tests but
passes an http:// repository URL to cloneRepository to assert the code upgrades
to HTTPS and injects the token; specifically, create a test named like "upgrades
http to https and injects GITLAB_TOKEN" that sets process.env.GITLAB_TOKEN,
clears GH_TOKEN, mockCreateCodebase to return a codebase whose repository_url is
"http://gitlab.mycompany.com/owner/repo", calls await
cloneRepository('http://gitlab.mycompany.com/owner/repo'), finds the git clone
call via spyExecFileAsync.mock.calls (same search used in other tests), asserts
the clone URL equals "https://oauth2:<token>@gitlab.mycompany.com/owner/repo",
and finally deletes process.env.GITLAB_TOKEN to clean up.

---

429-463: ⚡ Quick win

Move resolveForgeAuth import to top-level for consistency and type safety.

Line 430 uses const { resolveForgeAuth } = require('./clone') while the rest of the file imports from the same module via ES imports (line 70). This creates a style inconsistency and results in an untyped reference—require() returns any, losing type checking on the function call.

Since resolveForgeAuth is exported from clone.ts with a proper type signature, add it to the existing top-level import:

📝 Suggested fix

-import { cloneRepository, registerRepository } from './clone';
+import { cloneRepository, registerRepository, resolveForgeAuth } from './clone';

 describe('resolveForgeAuth', () => {
-  const { resolveForgeAuth } = require('./clone');
-
   test('returns GH_TOKEN for github.com', () => {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/handlers/clone.test.ts` around lines 429 - 463, The test
currently uses a runtime require for resolveForgeAuth inside the describe block
which bypasses TypeScript typing; remove the inline const { resolveForgeAuth } =
require('./clone') and instead add resolveForgeAuth to the existing top-level ES
import list (e.g., import { ..., resolveForgeAuth } from './clone') so the test
uses the typed export; ensure you delete the require line and run the tests to
confirm types and imports are correct.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes