Skip to content

[jsweep] Clean workflow_metadata_helpers.cjs #42415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 1 commit into from
Jun 30, 2026
Merged

[jsweep] Clean workflow_metadata_helpers.cjs #42415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 7 additions & 12 deletions actions/setup/js/workflow_metadata_helpers.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,10 @@
function getWorkflowMetadata(owner, repo) {
const workflowName = process.env.GH_AW_WORKFLOW_NAME || "Workflow";
const workflowId = process.env.GH_AW_WORKFLOW_ID || "";
const runId = context.runId || 0;
const runId = context.runId ?? 0;
const runUrl = buildWorkflowRunUrl({ runId, serverUrl: context.serverUrl }, { owner, repo });

return {
workflowName,
workflowId,
runId,
runUrl,
};
return { workflowName, workflowId, runId, runUrl };
}

/**
Expand All @@ -38,14 +33,14 @@ function getWorkflowMetadata(owner, repo) {
*/
function buildWorkflowRunUrl(ctx, workflowRepo) {

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[/codebase-design] The serverUrl fallback keeps || while the rest of the PR moves to ??. This is likely intentional — an empty string serverUrl should be treated as absent — but a brief inline comment would make the inconsistency self-documenting.

💡 Suggested comment 
// Use || intentionally: empty string serverUrl is invalid and should fall back
const server = ctx.serverUrl || process.env.GITHUB_SERVER_URL || 'https://github.com';

Without this note, future readers may wonder why this line was left unchanged when everything else was migrated to ??.

@copilot please address this.

const server = ctx.serverUrl || process.env.GITHUB_SERVER_URL || "https://github.com";
let { owner, repo } = workflowRepo || {};
let { owner = "", repo = "" } = workflowRepo ?? {};
if (!owner || !repo) {
// When context is spread (e.g. `{...context}`), prototype getters like context.repo
// are not included. Fall back to GITHUB_REPOSITORY for the workflow repo.
const parts = (process.env.GITHUB_REPOSITORY || "").split("/");
if (parts.length === 2 && parts[0] && parts[1]) {
owner = owner || parts[0];
repo = repo || parts[1];
const [envOwner = "", envRepo = ""] = (process.env.GITHUB_REPOSITORY || "").split("/");

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[/codebase-design] The new array destructuring removes the parts.length === 2 guard that was present in the original code, creating a subtle behaviour difference for malformed GITHUB_REPOSITORY values.

💡 Details

Original:

const parts = (process.env.GITHUB_REPOSITORY || '').split('/');
if (parts.length === 2 && parts[0] && parts[1]) { ... }

New:

const [envOwner = '', envRepo = ''] = (process.env.GITHUB_REPOSITORY || '').split('/');
if (envOwner && envRepo) { ... }

If GITHUB_REPOSITORY were ever "org/repo/extra", the old code would skip the fallback (length !== 2), while the new code silently uses org/repo. This is almost certainly harmless in practice, but the removal of the explicit length guard is an undocumented behaviour change. Consider adding a brief comment, or re-introducing the guard:

const parts = (process.env.GITHUB_REPOSITORY || '').split('/');
const [envOwner = '', envRepo = ''] = parts;
if (parts.length === 2 && envOwner && envRepo) { ... }

@copilot please address this.

if (envOwner && envRepo) {
owner = owner || envOwner;
repo = repo || envRepo;
}
Comment on lines +40 to 44
}
return `${server}/${owner}/${repo}/actions/runs/${ctx.runId}`;
Expand Down
61 changes: 61 additions & 0 deletions actions/setup/js/workflow_metadata_helpers.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,15 @@ describe("getWorkflowMetadata", () => {

expect(metadata.runUrl).toBe("https://github.com/my-owner/my-repo/actions/runs/7");
});

it("should use 0 as runId when context.runId is explicitly 0", () => {
global.context = { runId: 0, serverUrl: "https://github.com" };

const metadata = getWorkflowMetadata("owner", "repo");

expect(metadata.runId).toBe(0);
expect(metadata.runUrl).toBe("https://github.com/owner/repo/actions/runs/0");
});

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The runId = 0 test does not actually exercise the ?? vs || behavioral difference — 0 || 0 and 0 ?? 0 both evaluate to 0, so this test would pass with the old code too.

💡 Suggested fix

To prove ?? 0 semantics matter, add a case where context.runId is a falsy but non-null/undefined value — e.g. false or "" (empty string), which || would replace with 0 but ?? would preserve:

it('should preserve empty-string runId unchanged (proving ?? not || semantics)', () => {
  global.context = { runId: '', serverUrl: 'https://github.com' };
  const metadata = getWorkflowMetadata('owner', 'repo');
  // ?? 0 leaves '' as-is; || 0 would have returned 0
  expect(metadata.runId).toBe('');
});

Without a differentiating test, the PR description's claim that this case "validates ?? 0 semantics" is not actually verified by the test suite.

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[/tdd] The new test covers runId: 0 (the key || 0 vs ?? 0 difference), but the suite doesn't include tests for runId: null or runId: undefined — the two values ?? is specifically designed to handle.

💡 Suggested additions 
it('should use 0 as runId when context.runId is null', () => {
  global.context = { runId: null, serverUrl: 'https://github.com' };
  const metadata = getWorkflowMetadata('owner', 'repo');
  expect(metadata.runId).toBe(0);
});

it('should use 0 as runId when context.runId is undefined', () => {
  global.context = { runId: undefined, serverUrl: 'https://github.com' };
  const metadata = getWorkflowMetadata('owner', 'repo');
  expect(metadata.runId).toBe(0);
});

These cases are the primary motivation for choosing ?? over ||; testing them anchors the semantic intent for future readers.

@copilot please address this.

});

describe("buildWorkflowRunUrl", () => {
Expand Down Expand Up @@ -200,4 +209,56 @@ describe("buildWorkflowRunUrl", () => {
}
}
});

@github-actions github-actions Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[/tdd] The GITHUB_REPOSITORY save-and-restore pattern is repeated verbatim in three new tests. Extracting it to beforeEach/afterEach would reduce noise and eliminate the risk of a test accidentally leaking state.

💡 Suggested refactor 
describe('buildWorkflowRunUrl — env fallback', () => {
  let originalEnv;

  beforeEach(() => {
    originalEnv = process.env.GITHUB_REPOSITORY;
    process.env.GITHUB_REPOSITORY = 'env-owner/env-repo';
  });

  afterEach(() => {
    if (originalEnv === undefined) {
      delete process.env.GITHUB_REPOSITORY;
    } else {
      process.env.GITHUB_REPOSITORY = originalEnv;
    }
  });

  it('should use explicit owner but fall back to GITHUB_REPOSITORY for missing repo', () => {
    const url = buildWorkflowRunUrl(...);
    expect(url).toBe(...);
  });

  // ... other env-dependent tests
});

This also makes the test boundary explicit — readers immediately know this describe block relies on GITHUB_REPOSITORY.

@copilot please address this.

it("should use explicit owner but fall back GITHUB_REPOSITORY for missing repo", () => {
const originalEnv = process.env.GITHUB_REPOSITORY;
process.env.GITHUB_REPOSITORY = "env-owner/env-repo";
try {
const url = buildWorkflowRunUrl({ serverUrl: "https://github.com", runId: 5 }, { owner: "explicit-owner", repo: "" });
expect(url).toBe("https://github.com/explicit-owner/env-repo/actions/runs/5");
} finally {
if (originalEnv === undefined) {
delete process.env.GITHUB_REPOSITORY;
} else {
process.env.GITHUB_REPOSITORY = originalEnv;
}
}
});

it("should use explicit repo but fall back GITHUB_REPOSITORY for missing owner", () => {
const originalEnv = process.env.GITHUB_REPOSITORY;
process.env.GITHUB_REPOSITORY = "env-owner/env-repo";
try {
const url = buildWorkflowRunUrl({ serverUrl: "https://github.com", runId: 6 }, { owner: "", repo: "explicit-repo" });
expect(url).toBe("https://github.com/env-owner/explicit-repo/actions/runs/6");
} finally {
if (originalEnv === undefined) {
delete process.env.GITHUB_REPOSITORY;
} else {
process.env.GITHUB_REPOSITORY = originalEnv;
}
}
});

it("should not fall back to GITHUB_REPOSITORY when both owner and repo are set", () => {
const originalEnv = process.env.GITHUB_REPOSITORY;
process.env.GITHUB_REPOSITORY = "env-owner/env-repo";
try {
const url = buildWorkflowRunUrl({ serverUrl: "https://github.com", runId: 8 }, { owner: "wf-owner", repo: "wf-repo" });
expect(url).toBe("https://github.com/wf-owner/wf-repo/actions/runs/8");
expect(url).not.toContain("env-owner");
expect(url).not.toContain("env-repo");
} finally {
if (originalEnv === undefined) {
delete process.env.GITHUB_REPOSITORY;
} else {
process.env.GITHUB_REPOSITORY = originalEnv;
}
}
});

it("should use string runId in URL", () => {
const url = buildWorkflowRunUrl({ serverUrl: "https://github.com", runId: "run-abc123" }, { owner: "owner", repo: "repo" });
expect(url).toBe("https://github.com/owner/repo/actions/runs/run-abc123");
});
});
Loading