Repository navigation
fix: run the test recipe under MAKE_TIMED instead of skipping it - #29849
Merged
nickvigilante merged 1 commit intoSep 25, 2026
Conversation
With .ONESHELL, GNU make 4.4 mangles the shell arguments when the first word of .SHELLFLAGS is a shell builtin. MAKE_TIMED set .SHELLFLAGS to "$@ -ceu", so for the test target timed-shell.sh received "/bin/sh -c 'test -ceu' <recipe>". It then ran `test -ceu`, which succeeds without doing anything, and reported "/bin/sh" as the target. The pre-push hook's Go tests therefore never ran. In some checkouts the same argument handling corrupts make's memory and it segfaults, which blocks every push from that checkout. Prefix the target name with "target:" so the first word is never a builtin, and treat an empty target name as a parse-time $(shell) call, as the old "-ceu" check did.
nickvigilante
marked this pull request as ready for review
September 24, 2026 14:52
nickvigilante
added a commit
that referenced
this pull request
Sep 24, 2026
…29851) doc-check creates one `doc-check-pr-<N>` workspace per PR and never stops or deletes it. On docfood.cdr.dev, 54 of them ran at once, and the host's load average reached about 2900, mostly from processes blocked on I/O. This stops the review workspace once the chat reaches a terminal status, starts it again before the next review, and deletes it when the PR merges. A PR closed without merging keeps its workspace, because stale PRs get closed and reopened and the review needs its context back. Runs for the same PR share a concurrency group, so one run cannot stop or delete the workspace while another is reviewing in it. The workspace lookup also now matches the name exactly. The `name:` filter matches substrings (`coderd/database/queries/workspaces.sql`), so `doc-check-pr-12` also matched `doc-check-pr-123`. Depends on #29849, which fixes the pre-push hook crash that blocked pushing this branch. Retarget to `main` after it merges. Refs [DOCS-1007](https://linear.app/codercom/issue/DOCS-1007/doc-check-stop-review-workspace-after-each-run-and-delete-it-on-pr), CODAGT-1025 <details> <summary>Decision log</summary> ### Choices - **Stop only on a terminal chat status** (`waiting`, `completed`, `error`). When the 40-minute wait times out, the chat is still working in the workspace, so the step leaves it running. - **Start in the workflow, not the model.** The next run finds the workspace stopped. The workflow starts it through the REST API and waits up to 3 minutes, rather than relying on the agent to call `start_workspace`. The job timeout goes from 45 to 50 minutes to make room. A failed start only warns, so the agent's existing recovery path still applies. - **Delete on merge only.** Per review feedback, `closed` without `merged` keeps the workspace. Workspaces for PRs that close and never reopen will pile up (stopped, then dormant) until a sweep removes them. - **Serialize per PR** with `cancel-in-progress: false`. Canceling a job does not cancel the Coder chat, so cancellation would not stop the review it interrupts. ### Follow-up Replace per-PR workspaces with one persistent doc-check workspace and a git worktree per PR, pinned through the action's `workspace-id` input. ### Verification - `actionlint` and `zizmor --persona=regular` pass. - The exact-name jq filter picks the right workspace from sample responses, including an empty or null `workspaces` field. - Not exercised end to end against docfood. </details> --- 🤖 Generated by Coder Agents on behalf of @nickvigilante.
ethanndickson
approved these changes
Sep 25, 2026
nickvigilante
deleted the
vigilante/dog-55-pre-push-hook-never-runs-go-tests-and-segfaults-make-in-some
branch
September 25, 2026 12:16
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The pre-push hook's Go tests never run, and in some checkouts
makesegfaults (exit 139) and blocks every push.Environment: found and reproduced in a Write Coder on Coder workspace (
codertemplate, active version0ad7018) on dogfood: Ubuntu 26.04.1, GNU Make 4.4.1, 12 cores. That template is the standard coder/coder dev environment, so anyone who runs the pre-push hook there is affected.With
MAKE_TIMED=1, the Makefile sets.SHELLFLAGS = $@ -ceuunder.ONESHELL. GNU make 4.4.1 mangles the shell arguments when the first word of.SHELLFLAGSis a shell builtin such astest,exec, orcd: for thetesttarget,scripts/lib/timed-shell.shreceives/bin/sh -c 'test -ceu' <recipe>. The wrapper then runstest -ceu, which succeeds without doing anything, and reports✓ /bin/sh (0s). In some checkouts, freshly created worktrees among them, the same argument handling corrupts make's memory and it crashes.This prefixes the target name (
target:$@) so the first word is never a builtin, and strips the prefix in the wrapper. An empty target name still means a parse-time$(shell ...)call and goes straight to bash, as the old-ceucheck did.Note
For developers who run the pre-push hook (the allowlist in
scripts/githooks/pre-push, orgit config coder.pre-push true), pushes with Go or TypeScript changes now run the full Go suite, in parallel withtest-js. An uncached run took about 7 minutes in that workspace. Under that load, frontend tests hit their 5-second timeouts in 3 of 4 runs; a different handful each time, and the ones rerun alone passed.Closes DOG-55
Investigation and verification
Minimal reproduction
The wrapper receives
[/bin/sh] [-c] [test -ceu] [...]. Target namestest,exec, andcdtrigger it;echo,true,ls,build,lint, andgendo not. Dropping.ONESHELL, or using a single-line recipe, avoids it.testis the only target in this Makefile with a builtin name.Alternatives tried
.SHELLFLAGS = --target=$@ -ceu: also mangled..SHELLFLAGS = -ceu $@: breaks argument order..SHELLFLAGS = target:$@ -ceu: works for every name tested (test,exec,cd,set,export,foo).Verification
make MAKE_TIMED=1 test TEST_PACKAGES=./coderd/util/slice/...shows✓ testand runs 20 tests. Before the fix, it showed✓ /bin/sh (0s)or segfaulted.✗ test, so failures still propagate.make pre-push, four runs: 32258 Go tests passed in 410s on the first. Frontend timeouts under load across runs:ModelForm.test.tsx,DashboardLayout.test.tsx,OAuth2AppForm.test.tsx,DateRangePicker.test.tsx,useChatDraftAttachments.test.ts, andTemplateSchedulePage.test.tsx. The ones rerun alone passed 3/3. One run also hung inTestPubsub_Disconnect(the test's Postgres never came back up) until the 25-minute timeout. That test passed in the other runs.🤖 Generated by Coder Agents on behalf of @nickvigilante.