[typist] Optimize prompt: tighten bash allowlist, fix multi-op rejection, trim Phase 4 examples#43826
Conversation
…ove serena --help probe, trim Phase 4 examples Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43826 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Optimizes the [typist] agentic workflow prompt and tool allowlist to reduce token usage and avoid avoidable firewall rejections, while keeping the workflow’s analysis guidance focused.
Changes:
- Tightens/reshapes the restricted bash allowlist to better match expected runtime command prefixes.
- Updates Phase 0 setup instructions (activation + “one logical operation per bash call” guidance) and deduplicates the Phase 0 header.
- Replaces verbose Phase 4 Go code block examples with shorter bullet guidance to reduce prompt size.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/typist.md | Refines bash allowlist entries, adjusts Phase 0 activation guidance, and trims verbose examples to reduce prompt tokens. |
| .github/workflows/typist.lock.yml | Regenerates compiled workflow to reflect the updated prompt and allowlisted bash prefixes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
|
|
||
| 1. **Activate Serena Project**: | ||
| Use Serena's `activate_project` tool with the workspace path to enable semantic analysis. | ||
| > **Bash constraint:** Run one logical operation per Bash call. Chain with `&&` only when both parts are on the configured allowlist. |
There was a problem hiding this comment.
Fixed in f5dbad9: aligned the constraint wording with the example by covering pipes as well as && chaining.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Allowlist Tightening + Prompt Compression
Overall the changes are correct, targeted, and consistent between source and lock file. The token savings analysis is sound.
Verified:
- Lock file comment block and
--allowed-toolsstring match the new source allowlist exactly. - Phase 0 header dedup is logically correct — both original branches emitted the identical header, so making it unconditional is sound.
- Removing
Bash(cat pkg/**/*.go)is safe because the existingBash(cat)prefix entry already covers allcat <args>invocations. Bash(wc -l)correctly generalises the removedBash(wc -l pkg/**/*.go)without narrowing coverage.- The
&&-only constraint wording is intentional — it addresses the chained-tool-call rejection, not in-shell pipes (which are single-call constructs and handled differently by the firewall).
Non-blocking observations:
Bash(python3)is a broad prefix that permits arbitrary Python execution. This is justified by runtime evidence cited in the PR description, but worth documenting in the workflow if the specific usage pattern solidifies.Bash(serena)andBash(serena:*)coexist — the former covers CLI invocations (serena activate_project ...) while the latter covers MCP tool namespacing. The distinction is correct and intentional.
No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 39.5 AIC · ⌖ 5.09 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — commenting (no blocking issues, but three actionable concerns worth addressing).
📋 Key Themes & Highlights
Key Themes
- Allowlist breadth vs. specificity: Two new entries (
python3,mkdir -p) expand the agent's writable and executable surface without documented rationale; tightening them or adding comments would make future audits easier. - Constraint/example inconsistency: The new Phase 0 multi-op constraint mentions
&&but the very next code block uses a pipe (2>&1 | tail -20) — a silent exception that could confuse the agent or trigger the same firewall rejections the constraint is meant to prevent.
Positive Highlights
- ✅ The targeted grep-prefix decomposition (
grep -rn,grep -rhnP,grep -rhoP,grep -rcP) is a significant improvement over the previous blob-style glob patterns — much easier to reason about and maintain. - ✅ Collapsing the duplicated
### Phase 0header and replacing the verbose Serena tool-call instruction with a direct bash invocation is a clean simplification. - ✅ The Phase 4 template reduction (~500 tokens of boilerplate → terse bullet references) is well-executed: the meaning is fully preserved and the diff reads cleanly.
- ✅ Adding an explicit multi-op constraint to Phase 0 directly addresses the observed firewall-rejection loop — good causal reasoning from the run data.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 73.3 AIC · ⌖ 5.34 AIC · ⊞ 6.7K
Comment /matt to run again
| - grep -rcP | ||
| - wc -l | ||
| - awk | ||
| - python3 |
There was a problem hiding this comment.
[/codebase-design] python3 as a bare prefix is unusually broad for a read-mostly Go analysis workflow — any Python invocation is permitted, including script file execution.
The PR body says it was observed at runtime but doesn't document the use case. This leaves the allowlist scope unclear for future maintainers.
💡 Suggested tightening
If Python is only used for inline one-liners (e.g. JSON formatting or arithmetic), restrict to:
- python3 -cThis covers inline evaluation without allowing python3 script.py or other file-based invocations. If the broader prefix is genuinely needed, add a comment in the YAML explaining the use case.
@copilot please address this.
There was a problem hiding this comment.
Fixed in f5dbad9: tightened the allowlist entry from python3 to python3 -c so only inline one-liners are permitted.
|
|
||
| 1. **Activate Serena Project**: | ||
| Use Serena's `activate_project` tool with the workspace path to enable semantic analysis. | ||
| > **Bash constraint:** Run one logical operation per Bash call. Chain with `&&` only when both parts are on the configured allowlist. |
There was a problem hiding this comment.
[/codebase-design] The Bash constraint on line 109 says "Chain with && only when both parts are on the configured allowlist", but the serena example on the very next line uses a pipe (2>&1 | tail -20), which is also a compound operation the firewall may reject.
This is an inconsistency in the stated constraint: the rule mentions && chains but silently permits pipes in the example.
💡 Suggested fix
Either extend the constraint to name pipes explicitly:
Run one logical operation per Bash call. Chain with
&&or pipe (|) only when both parts are on the configured allowlist.
...or simplify the serena invocation to avoid the pipe altogether:
serena activate_project --project ${{ github.workspace }}The 2>&1 | tail -20 is primarily cosmetic (suppresses verbose output); if Serena fails, the agent needs the full error anyway.
@copilot please address this.
There was a problem hiding this comment.
Fixed in f5dbad9: updated the Bash constraint text to explicitly allow both && and | only when both sides are allowlisted.
| - serena | ||
| - sed -n | ||
| - head | ||
| - mkdir -p |
There was a problem hiding this comment.
[/codebase-design] mkdir -p is a write-side command that was not present before; the workflow is described as a read-only analysis task that ends with creating a discussion.
Adding mkdir -p broadens the writable surface of the sandboxed agent without any explanation in the PR body or a comment in the allowlist.
💡 Suggested action
If this is only needed to create a temp working directory (e.g. /tmp/gh-aw/agent/), consider narrowing it to that specific path:
- mkdir -p /tmp/gh-aw/agentOtherwise add a brief comment explaining what the agent is expected to create and why.
@copilot please address this.
There was a problem hiding this comment.
Fixed in f5dbad9: narrowed mkdir -p to mkdir -p /tmp/gh-aw/cache-memory/serena to constrain write scope.
🤖 PR Triage
Summary: Workflow optimization for
|
|
@copilot please run the
|
…-optimizer-optimization-typist-go-typ Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done in f5dbad9. I addressed the three |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot please run the
|
Completed a fresh pr-finisher pass on current HEAD
I can’t trigger GitHub Actions reruns from this agent context; a maintainer needs to re-trigger CI on the current HEAD. |
The typist workflow averaged 261.8 AIC/run with a firewall-rejection retry loop, 6 unused allowlist entries, a redundant
serena --helpprobe, and ~500 tokens of boilerplate Go code examples read on every run.Changes
grep -rn,grep -rhnP,grep -rhoP,grep -rcP,wc -l,awk,python3,serena,sed -n,head,mkdir -p) that match actual command patterns used at runtimeRun one logical operation per Bash call. Chain with && only when both parts are on the configured allowlist.Eliminates the chained-command firewall rejection + 3–5 recovery turns observed in the Jul 6 runactivate_projecttool" instruction with a direct bash invocationserena activate_project --project ${{ github.workspace }} 2>&1 | tail -20annotated with(skip any help-check), removing the unnecessary exploratory turnbefore/afterGo code blocks (~500 tokens) with terse bullet references; reduces prompt size for initial cache write on every run{{#if experiments.tone_style}}blocks both emitting the same### Phase 0: Setup and Activationheader into a single unconditional headerEstimated savings: 35–50 AIC/run (~13–19% reduction)