Skip to content

fix: prevent spurious .github symlink probes in frontmatter hash GitHub API reader#45989

Merged
pelikhan merged 8 commits into
mainfrom
copilot/fix-issue-87613232204
Jul 16, 2026
Merged

fix: prevent spurious .github symlink probes in frontmatter hash GitHub API reader#45989
pelikhan merged 8 commits into
mainfrom
copilot/fix-issue-87613232204

Conversation

Copilot AI commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Two bugs in the JS text-based hash computation caused createGitHubFileReader to make invalid GitHub API requests to paths like .github and .github/workflows — paths that don't resolve to files and broke the invariant that all fetched paths must be under .github/workflows/.

Bug 1: extractImportsFromText emitting uses:/path: keys as import paths

YAML object-form imports like:

- uses: ./serena.md
  with:
    languages: ["go"]

…were extracted as the raw string uses: ./serena.md. In JS, path.join("a/b/c", "uses: ./serena.md") preserves the embedded ./ as a path separator, producing extra segments. Go normalizes this away, so the mangled path hits a non-existent file and is silently discarded. Fix: skip items where the extracted value starts with uses: or path: to match Go's effective behavior.

Bug 2: resolveRemoteSymlinks probing too-shallow path prefixes

Non-relative imports inside subdirectory workflows (e.g. shared/otlp.md from shared/reporting-otlp.md) produce doubled-directory paths (shared/shared/otlp.md) via path.join. The resulting 404 triggered resolveRemoteSymlinks starting at index 1, which probed .github alone — a directory, not a workflow file. Fix: use a path-aware minimum startIndex:

  • .github/workflows/… → start at 3 (skip .github + .github/workflows)
  • .github/… → start at 2 (skip .github, preserving e.g. .github/agents symlink resolution)
  • everything else → 1 (unchanged)

Copilot AI and others added 3 commits July 16, 2026 12:06
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… probes in hash computation

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title fix: skip YAML object-form imports and avoid spurious .github symlink probes in hash computation fix: prevent spurious .github symlink probes in frontmatter hash GitHub API reader Jul 16, 2026
Copilot AI requested a review from pelikhan July 16, 2026 12:39
@pelikhan
pelikhan marked this pull request as ready for review July 16, 2026 12:39
Copilot AI review requested due to automatic review settings July 16, 2026 12:39
@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Test Quality Sentinel completed test quality analysis.

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

PR Code Quality Reviewer completed the code quality review.

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #45989 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Prevents invalid GitHub API path probes during frontmatter hash computation.

Changes:

  • Filters object-form import entries.
  • Skips known non-symlink path prefixes.
  • Updates tests and regenerated workflows.
Show a summary per file
File Description
actions/setup/js/frontmatter_hash_pure.cjs Updates import extraction and symlink probing.
actions/setup/js/frontmatter_hash_pure.test.cjs Adds regression coverage.
.github/workflows/skillet.lock.yml Refreshes action version annotations.
.github/workflows/release.lock.yml Refreshes action version annotations.
.github/workflows/hourly-ci-cleaner.lock.yml Refreshes action version annotations.
.github/workflows/avenger.lock.yml Refreshes action version annotations.
.github/workflows/agentic-auto-upgrade.yml Changes the weekly schedule.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 7/7 changed files
  • Comments generated: 3
  • Review effort level: Medium

Comment on lines +275 to +280
// Skip YAML object-form imports (uses: <path> / path: <path>) — these require full
// YAML parsing and are not supported by the text-based hash parser. The Go implementation
// silently skips them at the filesystem level (file not found); we skip them here to
// match that behavior and avoid spurious GitHub API calls.
if (item.startsWith("uses:") || item.startsWith("path:")) {
continue;

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.

Fixed in 6e6a265. extractImportsFromText now extracts the path value from uses: <path> and path: <path> prefixes instead of skipping those items. The same fix was applied to the Go extractImportItem for JS/Go hash parity. All 257 workflow lock files were recompiled to reflect the updated hash values.

Comment on lines +614 to +617
if (filePath.startsWith(".github/workflows/")) {
startIndex = 3; // Skip ".github" and ".github/workflows" components
} else if (filePath.startsWith(".github/")) {
startIndex = 2; // Skip ".github" component; still check e.g. ".github/agents"

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.

Fixed in 6e6a265. Added "should never probe .github or .github/workflows for a .github/workflows/ path" which records all probed paths, asserts neither .github nor .github/workflows is ever requested, and confirms .github/workflows/shared is still probed for a path like .github/workflows/shared/otlp.md.

on:
schedule:
- cron: "21 3 * * 5" # Weekly (auto-upgrade)
- cron: "11 4 * * 6" # Weekly (auto-upgrade)

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.

Fixed in 6e6a265. The schedule drift ("11 4 * * 6" → Saturday) was reverted to the original "21 3 * * 5" (Friday). This was unintended regeneration noise unrelated to the PR's changes.

@github-actions

Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel Report

Test Quality Score: 100/100 — Excellent

Analyzed 3 test(s): 3 design, 0 implementation, 0 violation(s).

📊 Metrics (3 tests)
Metric Value
Analyzed 3 (Go: 0, JS: 3)
✅ Design 3 (100%)
⚠️ Implementation 0 (0%)
Edge/error coverage 3 (100%)
Duplicate clusters 0
Inflation No (1.35:1)
🚨 Violations 0
Test File Classification Issues
should extract path from uses: format frontmatter_hash_pure.test.cjs:99 design_test None
should extract path from path: format frontmatter_hash_pure.test.cjs:113 design_test None
memoize symlink lookups (modified assertion) frontmatter_hash_pure.test.cjs:959 design_test None

Verdict

Passed. 0% implementation tests (threshold: 30%).

Highlights:

  • Tests 1 & 2 cover new extractImportsFromText behavior for uses: and path: object-form imports, verifying plain string imports still parse and object-form entries are skipped.
  • The modified memoize test precisely validates the core bug fix: .github is no longer probed (assertion changed from toBe(1) to toBeUndefined()), directly enforcing the behavioral contract the fix introduced.
  • All 3 tests cover edge cases with clear behavioral assertions and no mocking violations.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

🧪 Test quality analysis by Test Quality Sentinel · 29.2 AIC · ⌖ 8.72 AIC · ⊞ 6.8K ·
Comment /review to run again

@github-actions github-actions Bot mentioned this pull request Jul 16, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Test Quality Sentinel: 100/100. 0% implementation tests (threshold: 30%).

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The two fixes are correct and well-tested.

Bug 1 — extractImportsFromText skipping uses:/path: keys: The filter correctly detects object-form YAML list items after the - prefix is stripped. New tests cover both forms and verify plain string imports in the same list are still extracted.

Bug 2 — resolveRemoteSymlinks startIndex optimization: The index math is correct for all path shapes:

  • .github/workflows/foo.md (3 parts, startIndex=3): loop never runs — no API calls for known non-symlink dirs.
  • .github/workflows/dir/foo.md (4 parts, startIndex=3): only dir is probed — correct.
  • .github/agents/... paths still check .github/agents (startIndex=2).

The updated test assertion (callCounts.get(".github")toBeUndefined) correctly reflects the new behaviour.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 27.4 AIC · ⌖ 4.33 AIC · ⊞ 5K

@github-actions

Copy link
Copy Markdown
Contributor

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd — the fixes are correct and well-targeted; commenting with minor improvements.

📋 Key Themes & Highlights

Key Themes

  • Test names mislead: Both new extractImportsFromText tests say "extract" but assert that items are skipped. This risks future contributors reading the test as confirming extraction and inverting the intent.
  • Silent skip gap: The uses: / path: skip stops spurious API calls correctly, but there is no diagnostic signal (debug log or documented limitation) that object-form imports are silently omitted from the hash. If a workflow changes only its uses:-form imports, the hash will not change and the setup action will not re-run.

Positive Highlights

  • ✅ Both bugs fixed at the right layer (text parser and symlink traversal)
  • ✅ Regression tests added for both bugs
  • startIndex logic handles .github/ vs .github/workflows/ hierarchy correctly with clear comments
  • ✅ Updated assertion for .github no longer being probed is exact and well-explained

@copilot please address the review comments above.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 23.4 AIC · ⌖ 4.63 AIC · ⊞ 6.7K ·
Comment /matt to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 23.4 AIC · ⌖ 4.63 AIC · ⊞ 6.7K
Comment /matt to run again

const result = extractImportsFromText(frontmatterText);
expect(result).toEqual(["shared/test.md"]);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] Misleading test name — the test verifies that uses:-form items are skipped, not extracted.

💡 Suggested rename
it("should skip object-form uses: imports (requires full YAML parser)", () => {

A test name should read as a specification. "should extract path from uses: format" implies the path is returned, but the assertion shows it is excluded. Renaming makes the intent clear and prevents a future contributor from "fixing" the test.

@copilot please address this.

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.

Fixed in 6e6a265. The test was renamed to "should extract path from object-form uses: import" and the assertion updated to expect(result).toEqual(["./serena.md", "shared/common.md"])uses: imports are now extracted (not skipped).

// Skip YAML object-form imports (uses: <path> / path: <path>) — these require full
// YAML parsing and are not supported by the text-based hash parser. The Go implementation
// silently skips them at the filesystem level (file not found); we skip them here to
// match that behavior and avoid spurious GitHub API calls.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/diagnosing-bugs] The uses: / path: skip silently drops object-form imports without surfacing the limitation anywhere. If a workflow heavily relies on uses:-form imports, the hash computation will silently under-count dependencies — potentially missing re-runs when those imports change.

💡 Suggestion

Emit a core.debug (or console.debug) log when an item is skipped so that diagnostics can confirm the skip is intentional rather than a silent data loss:

if (item.startsWith("uses:") || item.startsWith("path:")) {
  // debug only; callers cannot act on it at hash-compute time
  // console.debug(`[frontmatter_hash] skipping object-form import: ${item}`);
  continue;
}

Alternatively, document in a CONTEXT.md or inline comment that the hash may be conservative (always triggers a re-run) rather than exact when object-form imports are present, so future maintainers understand the known gap.

@copilot please address this.

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.

Addressed in 6e6a265 by fixing the root cause: extractImportsFromText now extracts the path from uses: and path: prefixes instead of skipping them. Object-form imports are fully tracked in the hash, so there is no longer a silent data loss to surface via a debug log.

expect(result).toEqual(["shared/common.md"]);
});

it("should extract path from path: format", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] Same misleading test name issue on the path: variant — the test name says "extract" but the behaviour is "skip".

Consider: "should skip object-form path: imports (requires full YAML parser)".

@copilot please address this.

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.

Fixed in 6e6a265. Renamed to "should extract path from object-form path: import" and assertion updated to expect(result).toEqual(["shared/tool.md"])path: imports are now extracted (not skipped).

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Requesting changes: Bug 1's fix is semantically incorrect and introduces a new hash divergence between the JS and Go implementations.

📋 Findings summary

Critical: JS/Go hash divergence for object-form imports

The fix to extractImportsFromText (line 279) silently skips uses: and path: object-form imports. But Go's import_topological.go (lines 227–234) extracts the path value and includes it in the hash. Any workflow containing - uses: ./foo.md will produce different hashes in JS vs Go, defeating the cross-language hash verification that this code exists to ensure.

The fix should extract the path value after the colon, not skip the entry entirely.

Medium: Test name is semantically wrong

The test at line 170 is named 'should extract path from uses: format' but asserts that the path is skipped. Minor but will confuse future readers and mask regressions.

No issues with Bug 2 fix

The resolveRemoteSymlinks startIndex logic (lines 152–157) is correct. The path-prefix conditions properly skip non-symlink .github prefix components.

🔎 Code quality review by PR Code Quality Reviewer · 68.1 AIC · ⌖ 4.67 AIC · ⊞ 5.6K
Comment /review to run again

Comments that could not be inline-anchored

actions/setup/js/frontmatter_hash_pure.cjs:279

Hash divergence: uses:/path: object-form imports are silently skipped in JS but extracted in Go, causing frontmatter hash mismatches for any workflow that uses object-form import syntax.

<details>
<summary>💡 Details and suggested fix</summary>

The Go parser in import_topological.go (lines 227–234) explicitly extracts the path value from object-form imports and appends it to the hash input:

case map[string]any:
    if pathValue, hasPath := importItem[&quot;path&quot;]; hasPath {
      …

</details>

<details><summary>actions/setup/js/frontmatter_hash_pure.test.cjs:170</summary>

**Misleading test name**: `&#39;should extract path from uses: format&#39;` describes extraction, but the assertion verifies that the `uses:` item is *skipped*, not extracted.

&lt;details&gt;
&lt;summary&gt;💡 Details&lt;/summary&gt;

The test asserts `expect(result).toEqual([&quot;shared/common.md&quot;])`i.e., the `uses: ./serena.md` entry is absent from the result. But the test name says &quot;extract path from uses: format&quot;, which implies the opposite behavior (that the path _is_ extracted and returned).

Beyond being confusin</details>

@github-actions

Copy link
Copy Markdown
Contributor

🤖 PR Triage

Field Value
Category bug
Risk 🟢 Low
Score 62 (impact 28 + urgency 20 + quality 14)
Action fast_track

Rationale: Targeted JS hash-reader fix (2 bugs, 7 files, +57/-15). Copilot bot reviewed, github-actions bot APPROVED. CI in-progress but passing. Prevents silent invalid API requests in hash computation.

Generated by 🔧 PR Triage Agent · 32.8 AIC · ⌖ 4.9 AIC · ⊞ 5.6K ·

@github-actions

Copy link
Copy Markdown
Contributor

Hey @copilot-swe-agent 👋 — great work tracking down these two bugs in extractImportsFromText and the path-stripping logic in frontmatter_hash_pure.cjs! The fix is focused, well-described, and comes with updated tests. This looks ready for review! 🎉

Generated by ✅ Contribution Check · 100.5 AIC · ⌖ 8.71 AIC · ⊞ 6.2K ·

@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot run pr-finisher skill

Copilot AI and others added 2 commits July 16, 2026 13:47
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ation

- JS extractImportsFromText: extract path value from `uses:` and `path:`
  prefixes instead of skipping, so object-form imports are included in
  the frontmatter and body hashes. Changes to imported files now
  correctly invalidate the cached hash.
- Go extractImportItem: same extraction fix using strings.CutPrefix for
  hash parity between JS and Go implementations.
- Update JS tests: assertions and names updated to reflect extraction
  (not skip) of object-form import paths.
- Add Go tests: TestExtractImportsFromText_ObjectFormUsesImport and
  TestExtractImportsFromText_ObjectFormPathImport.
- Add JS regression test: asserts .github and .github/workflows are
  never probed by resolveRemoteSymlinks for a .github/workflows/ path.
- Revert unintended schedule drift in agentic-auto-upgrade.yml (Friday
  → Saturday cron change was generated noise, not an intentional change).
- Recompile all 257 workflow lock files to reflect updated hash values.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

@copilot run pr-finisher skill

All review threads addressed in commit 6e6a265. Summary below.

@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot merge main and recompile

Copilot AI and others added 3 commits July 16, 2026 14:34
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…13232204

# Conflicts:
#	.github/workflows/aw-failure-investigator.lock.yml
#	.github/workflows/code-scanning-fixer.lock.yml
#	.github/workflows/daily-function-namer.lock.yml
#	.github/workflows/daily-malicious-code-scan.lock.yml
#	.github/workflows/glossary-maintainer.lock.yml
#	.github/workflows/pr-code-quality-reviewer.lock.yml
#	.github/workflows/refiner.lock.yml
#	.github/workflows/security-review.lock.yml
#	.github/workflows/skillet.lock.yml
#	.github/workflows/stale-repo-identifier.lock.yml
#	.github/workflows/unbloat-docs.lock.yml

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

@copilot merge main and recompile

Done. Merged origin/main (resolved .lock.yml conflicts by taking theirs) and recompiled all 257 workflows successfully.

@pelikhan
pelikhan merged commit 1542daf into main Jul 16, 2026
37 checks passed
@pelikhan
pelikhan deleted the copilot/fix-issue-87613232204 branch July 16, 2026 15:04
@github-actions

Copy link
Copy Markdown
Contributor

🎉 This pull request is included in a new release.

Release: v0.82.11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants