Surface stalled Claude CLI failures and bound Design Decision Gate diff input#53677
Conversation
PR TriageCategory: chore (WIP) | Risk: low | Priority: low (score 20)
Recommended action: defer — recheck once agent finishes and diff is populated
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #53677 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). No action required.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No blocking correctness or maintainability regressions stand out in the changed lines.
Review notes
I checked the diff-capping path, the stall-annotation wiring, and the new tests/doc updates. The 200 KB cap is recorded in metadata, truncated diffs are explicitly signaled to downstream agents, and the harness change only alters log annotation severity without changing process control flow. I did not find a changed-line issue that is specific enough to block merge.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 4.84 AIC · ⌖ 6.8 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — requesting changes on two correctness issues and a test coverage gap.
📋 Key Themes & Highlights
Key Themes
- Missing negative test (
process_runner.test.cjs): the new test only asserts the::error::path; a complementary negative test is needed to prevent silent regressions. - Byte-boundary truncation (
design-decision-gate.md):head -ccan split UTF-8 characters or diff hunks mid-line; line-boundary truncation is safer. - Annotation format (
process_runner.cjs): the::error::prefix is concatenated inline without structuredtitle=/file=parameters, making the annotation harder to triage in the Actions UI.
Positive Highlights
- ✅ Clean separation between the stall-warning and stall-error paths via a single env var.
- ✅ Removing
.patchfrompr-files.jsonis a good defensive move to reduce token exposure. - ✅ Persisting
diff_size_bytesanddiff_truncatedinadr-prefetch-summary.jsongives the agent reliable guard-rails without extra API calls. - ✅ New test is well-structured and uses the real Node.js process, keeping it integration-level without external dependencies.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 33.2 AIC · ⌖ 10.2 AIC · ⊞ 7.8K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/process_runner.test.cjs:183
[/tdd] The test only covers the GH_AW_HARNESS_STALL_ERROR=true path — there's no assertion that the ::error:: prefix is absent when the env var is unset. Without a negative case the test wouldn't catch a regression that hard-codes the prefix unconditionally.
<details>
<summary>💡 Suggested negative test</summary>
it("does not emit an error annotation when stall error is not configured", async () => {
const logs = [];
await runProcess({
command: process.execPath,
…
</details>
<details><summary>.github/workflows/design-decision-gate.md:95</summary>
**[/diagnosing-bugs]** `head -c` truncates at a byte boundary, potentially splitting a UTF-8 character or cutting mid-diff-hunk. The AI agent then receives a malformed partial line before the truncation notice, which could mislead hunk-aware parsing.
<details>
<summary>💡 Truncate at a line boundary instead</summary>
Replace the `head -c` approach with `awk` to truncate cleanly at the last complete line within the byte budget:
```bash
awk -v max="$MAX_DIFF_BYTES" \
'BEGIN{n=0} {n+=length($…
</details>
<details><summary>actions/setup/js/process_runner.cjs:160</summary>
**[/diagnosing-bugs]** The `::error::` annotation is prepended inline to the stall message string, producing one long line. GitHub Actions supports structured annotation parameters (`title=`, `file=`) — without them the annotation appears titleless in the UI summary, making it harder to triage at a glance.
<details>
<summary>💡 Use the structured annotation format</summary>
```js
const stallPrefix = stallIsError
? "::error title=Agent CLI stall,file=process_runner.cjs::"
: "";
`${stallPre…
</details>
There was a problem hiding this comment.
The changes look correct and well-tested.
Stall surfacing (process_runner.cjs): The GH_AW_HARNESS_STALL_ERROR flag cleanly prepends ::error:: to the stall watchdog annotation without changing the process lifecycle — behaviour matches documentation.
Diff bounding (design-decision-gate.md): Switching from gh pr diff to jq-reconstructed diff from the PR files API is a solid approach for controlling token consumption. The 200KB cap + diff_truncated signal + updated agent instructions give consumers a clear contract.
One minor observation (non-blocking): when FILE_COUNT > 300, the new code sets DIFF_TRUNCATED=true. Semantically, diff_truncated implies "was generated but capped" rather than "generation was skipped entirely." Since diff_available: false already gates the agent away from reading pr.diff, this has no functional impact — but a value of false for diff_truncated (or a dedicated diff_skipped field) would be more precise for that branch.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 37.3 AIC · ⌖ 8.81 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Ponytail review: over-engineering only.
L143-144: native: hand-assembled diff text via jq template (diff --git/---/+++/patch) reinvents what gh pr diff already emits. gh pr diff "$PR_NUMBER" --repo "$EXPR_GITHUB_REPOSITORY" | head -c "$MAX_DIFF_BYTES", 0 hand-rolled diff formatting.
net: -12 lines possible.
Generated by ✂️ Ponytail Reviewer for #53677 · auto · 40.2 AIC · ⌖ 12.3 AIC · ⊞ 7.3K
Comment /ponytail to run again
| --repo "$EXPR_GITHUB_REPOSITORY" \ | ||
| > /tmp/gh-aw/agent/pr.diff | ||
| MAX_DIFF_BYTES=200000 | ||
| jq -r '.[] | "diff --git a/\(.filename) b/\(.filename)\n--- a/\(.filename)\n+++ b/\(.filename)\n\(.patch // "# Patch unavailable")\n"' \ |
There was a problem hiding this comment.
L143-144: native: hand-assembled diff text via jq template (diff --git/---/+++/patch) reinvents what gh pr diff already emits. gh pr diff "$PR_NUMBER" --repo "$EXPR_GITHUB_REPOSITORY" | head -c "$MAX_DIFF_BYTES", 0 hand-rolled diff formatting.
There was a problem hiding this comment.
Pull request overview
Adds stall diagnostics and bounds Design Decision Gate diff context.
Changes:
- Adds opt-in stall error annotations with tests and documentation.
- Caps prefetched PR diffs at 200 KB and records truncation metadata.
- Configures and recompiles the Design Decision Gate workflow.
Show a summary per file
| File | Description |
|---|---|
docs/src/content/docs/reference/environment-variables.md |
Documents stall error annotations. |
actions/setup/js/process_runner.test.cjs |
Tests configured stall errors. |
actions/setup/js/process_runner.cjs |
Emits stall error markers. |
.github/workflows/design-decision-gate.md |
Bounds diff context and enables stall reporting. |
.github/workflows/design-decision-gate.lock.yml |
Regenerates the compiled workflow. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
| jq -r '.[] | "diff --git a/\(.filename) b/\(.filename)\n--- a/\(.filename)\n+++ b/\(.filename)\n\(.patch // "# Patch unavailable")\n"' \ | ||
| /tmp/gh-aw/agent/pr-files-full.json > /tmp/gh-aw/agent/pr.diff.full | ||
| DIFF_SIZE_BYTES=$(wc -c < /tmp/gh-aw/agent/pr.diff.full) |
| stallWarnings++; | ||
| log( | ||
| `attempt ${attempt + 1}: stall watchdog: no output from '${command}' for ${formatDuration(idleMs)}` + | ||
| (stallIsError ? "::error::Agent CLI exceeded its no-output budget. " : "") + |
Design Decision Gate could silently stall while Claude read large PR diffs, ending only at the Actions timeout. This change makes stalls visible and limits diff content passed to the agent.
Stall visibility
Bounded PR context
pr.difffrom GitHub file patches and cap it at 200 KB.pr-files.json.