Conversation
…helper function Agent-Logs-Url: https://github.com/github/gh-aw/sessions/1f6c6c24-eb6e-484f-b08f-424a7ff9d79d Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot review cache-memory instructions and prompt to match implementation. |
|
Hey One thing that would help get this across the finish line:
If you'd like a hand, you can assign this prompt to your coding agent:
|
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/12fdafe4-4109-48d8-be02-4f0f1299063e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in e2ba5c6. Issues found and fixed: Prompt templates (
Documentation (
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
🧪 Test Quality Sentinel ReportTest Quality Score: 70/100
Test Classification Details
Also: two existing tests ( Flagged Tests — Requires ReviewNo tests flagged for guideline violations or implementation-test issues. 💡 Suggested Improvements (non-blocking)Both new tests cover only the happy path. The
Example table-driven addition: func TestBuildCacheMemoryPromptSection_AllowedExtensionsEdgeCases(t *testing.T) {
tests := []struct {
name string
caches []CacheMemoryEntry
expected string
}{
{name: "dedup across caches", caches: []CacheMemoryEntry{
{ID: "a", AllowedExtensions: []string{".json"}},
{ID: "b", AllowedExtensions: []string{".json", ".txt"}},
}, expected: "\n<allowed-extensions>.json, .txt</allowed-extensions>"},
{name: "empty slice is blank", caches: []CacheMemoryEntry\{\{ID: "a", AllowedExtensions: []string{}}}, expected: ""},
}
...
}Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §24936314670
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 70/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). Both new tests verify behavioral contracts (env var values for allowed-extensions feature). No coding-guideline violations. Minor suggestion: add edge-case tests for extension deduplication across caches.
There was a problem hiding this comment.
Pull request overview
Formalizes a single, canonical cache-memory directory naming convention and aligns prompt templates + reference docs with the actual cache-memory implementation.
Changes:
- Centralized cache-memory path derivation via
cacheMemoryDirFor()and replaced duplicated path logic across workflow compilation and tool-permission code. - Updated cache-memory prompt templates to include the
__GH_AW_ALLOWED_EXTENSIONS__placeholder and extended tests to cover the new substitution behavior. - Corrected cache-memory reference documentation around key defaults, run_id handling, and restore-keys behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/copilot_engine_execution.go | Uses cacheMemoryDirFor() to add cache directories to Copilot CLI args consistently. |
| pkg/workflow/claude_tools.go | Uses cacheMemoryDirFor() for per-cache filesystem tool allowlisting patterns. |
| pkg/workflow/cache_memory_prompt_test.go | Adds tests ensuring GH_AW_ALLOWED_EXTENSIONS is rendered (or empty) as expected. |
| pkg/workflow/cache.go | Introduces canonical cache-memory path constants + helper; updates prompt rendering to include allowed-extensions XML block. |
| docs/src/content/docs/reference/cache-memory.md | Fixes inaccuracies in cache-memory key/restore-keys documentation and examples. |
| actions/setup/md/cache_memory_prompt_multi.md | Adds __GH_AW_ALLOWED_EXTENSIONS__ placeholder to multi-cache prompt template. |
| actions/setup/md/cache_memory_prompt.md | Adds __GH_AW_ALLOWED_EXTENSIONS__ placeholder to single-cache prompt template. |
| .github/workflows/ai-moderator.lock.yml | Updates locked workflow prompt substitution env to match new allowed-extensions rendering format. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 1
| } | ||
| return cacheMemoryDirPrefix + cacheID | ||
| } | ||
|
|
||
| // validCacheMemoryScopes defines the allowed values for cache-memory scope | ||
| var validCacheMemoryScopes = []string{"workflow", "repo"} | ||
|
|
…lization Generated by Design Decision Gate [§24936314671]. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (103 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References: §24936314671
|
|
@copilot review all comments |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/e0da8bd5-c1bb-4b8c-ab0c-024734463273 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 3de96e9. The code-review finding about path traversal was the main actionable item: Added
Tests cover valid IDs, path-traversal patterns, slashes, dots, spaces, and the panic in |
The cache directory path computation was duplicated 10+ times across three files with no single authoritative definition, making the convention implicit and prone to drift. Additionally, the agent-facing prompt templates and reference documentation had several inaccuracies relative to the actual implementation. A path-traversal vulnerability in the cache ID → directory path mapping was also identified and fixed.
Changes
Cache directory naming convention
New constants in
cache.go:defaultCacheMemoryDir = "/tmp/gh-aw/cache-memory"— canonical path for the default cachecacheMemoryDirPrefix = "/tmp/gh-aw/cache-memory-"— prefix for named cachesNew helper
cacheMemoryDirFor(cacheID string) string— single source of truth for resolving a cache ID to its runtime path (no trailing slash; callers that need one for display context append it explicitly)Replaced all 10 scattered if/else blocks in
cache.go,claude_tools.go, andcopilot_engine_execution.goCache ID validation (security fix)
isValidCacheID— new validation function enforcing^[A-Za-z0-9_-]{1,64}$. Without this, a user-supplied ID such as../../etc/passwdwould be concatenated directly into a filesystem path, escaping/tmp/gh-aw.parseCacheMemoryEntry, returning a clear error for invalid IDs.cacheMemoryDirForpanics if an invalid ID somehow bypasses parse-time validation.Prompt template fixes
cache_memory_prompt.mdandcache_memory_prompt_multi.md: added the missing__GH_AW_ALLOWED_EXTENSIONS__placeholder. The Go code was computing and passingGH_AW_ALLOWED_EXTENSIONSbut neither template had a slot for it, so agents were never informed about file-type restrictions. Whenallowed-extensionsis configured the placeholder now renders as<allowed-extensions>.json, .txt</allowed-extensions>; it is empty when all extensions are allowed.Documentation fixes (
reference/cache-memory.md)${{ github.run_id }}from the Advanced Configuration key example — the compiler explicitly rejects keys containinggithub.run_idwith a validation errorGH_AW_WORKFLOW_ID_SANITIZED, notgithub.workflow)run_idsuffix to form a single restore-keys prefix;scope: repoadds a second key without the workflow ID