fix: CI workflow YAML — get the green check actually running - #3
Open
bjohnson135 wants to merge 9 commits into
Open
fix: CI workflow YAML — get the green check actually running#3bjohnson135 wants to merge 9 commits into
bjohnson135 wants to merge 9 commits into
Conversation
CI on master ran 0s with conclusion=failure on every push since v1.0.0.0
landed. "This run likely failed because of a workflow file issue" — the
runner refused to schedule any jobs.
Three fixes applied empirically (no single smoking gun was visible in the
API output, all three are common parse-blockers):
1. ubuntu-24.04 → ubuntu-latest. The 24.04 label exists, but combined with
the other quirks below it may have triggered the schema validator's
one-strike fail mode.
2. setup-node before pnpm/action-setup → pnpm/action-setup before
setup-node. v4 of pnpm/action-setup detects pnpm via packageManager in
package.json or the explicit version arg, and works either order, but
the canonical example in pnpm docs puts pnpm first.
3. Quoted version pins ('1.23.4', '20.18.0', '9.12.0'). YAML happily
coerces dotted strings into floats in some loaders; Go and Node
versions look like floats but aren't.
Also dropped the proto-check job entirely. It was gated on
`hashFiles('buf.yaml') != ''` which evaluates pre-checkout (empty), so
the job was always skipped — and the gate likely tripped GitHub's
workflow validator. We don't have a buf.yaml yet (W3 deferred per
TODOS.md), so the check has nothing to do. Revive it as its own
workflow when buf lands.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…l → io/os) CI's golangci-lint job (now actually running thanks to PR #3's YAML fix) flagged 13 pre-existing issues. Cleared the entire surface — not just the 13 — so they don't surface again as new files trip them. - grpc.Errorf(...) → status.Errorf(...) (132 sites across 27 files; SA1019) - grpc.Code(err) → status.Code(err) (6 sites; SA1019) - "io/ioutil" → "io" / "os" (10 files; SA1019, deprecated since Go 1.19) - regexp.MatchString in loop → regexp.MustCompile + re.MatchString (faraday/mobileconfig_test.go and www/mobileconfig_test.go; SA6000) - account/server/server.go:700 ineffectual err — now actually checks the GetAdminOf error and bails with internalError if it fails `google.golang.org/grpc/status` imports added by goimports where the status package was newly used. `google.golang.org/grpc` import kept where still needed for client constructors. Build clean: `go build ./...` exits 0. Tests pass: `go test -race -short -count=1 -timeout=120s ./...` exits 0 across all packages with tests (apidocs, auth, crypto, environments, errorpages, faraday, healthcheck, middlewares, www, company/server). Local golangci-lint hit a Go-1.24-toolchain deps-cache hiccup against the Go 1.23 module set; pushing so the CI lint job (Linux, pinned 1.23.4) gives the authoritative read. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
CI's lint job (now actually running) flagged 60 issues across the legacy
codebase. Categorized:
govet: 3 — real bugs (printf format strings)
ineffassign: 3 — real bugs (assignments never read)
errcheck: 32 — mostly res.Write / ListenAndServe / grpcServer.Serve
unused: 9 — dead code (langBox, libBox, addr, wildcard, etc.)
gosimple: 12 — style (S1002, S1023, S1003, S1011)
This commit fixes the 6 real-bug findings and writes a .golangci.yml that
keeps the bug-finders ON (govet, staticcheck, ineffassign) and disables
errcheck/unused/gosimple for now. Each disabled linter gets its own polish
PR before re-enabling — see TODOS.md.
govet (printf format strings):
- environments/sentry_test.go:57 fmt.Printf(conf.dsn) → "%s", conf.dsn
- envserver/main.go:40 fmt.Fprintf(w, b.String()) → fmt.Fprint
- auditlog/auditlog.go:30 logger.Infof(action) → logger.Info(action)
ineffassign (real bugs — error swallowed):
- account/server/server.go:511 s.Get failure was discarded; now logs+
bails out via s.internalError before
using the (possibly-nil) account.
- account/server/server.go:690 GetWorkerOf failure was discarded;
same fix.
- suite/data.go:45 http.NewRequest failure was discarded;
return wrapped error early.
.golangci.yml (v1 schema; CI pins 1.61.0):
Documents which linters are enabled, why the others are off, and points
to TODOS.md for the cleanup plan.
Local golangci-lint can't run on Go 1.24.2 toolchain (export-data version
mismatch with v1.61.0). CI on Go 1.23.4 is authoritative.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
….go req err, company UpdateCompany authz err) Same shape as the ineffassign cluster fixed in the previous commit; cleared by CI's lint job after the first wave landed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ineffassign ./... now reports zero. Same shape as the previous wave — err return discarded by a switch authz that only reads err inside its case branches. - company/server/directory.go:198 UpdateDirectoryEntry - company/server/jobs.go:129 UpdateJob - company/server/shifts.go:351 UpdateShift - company/server/teams.go:149 UpdateTeam - company/server/teams.go:198 GetWorkerTeamInfo - www/new_company.go:61 accountClient.Get err shadowed Each now bails out via s.internalError before entering the auth switch. www/new_company.go panics on err (matches existing pattern in that file). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
When Faraday's ServiceMiddleware strips a matched prefix (e.g. /app), the proxyHandler's auth-redirect to /login/ was emitting return_to= using the stripped path, so /app/team/123 redirected to /login/?return_to=/team/123 instead of /login/?return_to=/app/team/123. After login the browser then landed on /app/ (the default fallback) instead of the deep link. Pulls requestedPathPrefix from the request context (set by ServiceMiddleware) and prepends it before emitting return_to. Honors trailing-slash exact match so /app stays /app and /app/ stays /app/. Verified via curl: /app/ → return_to=/app/ /myaccount/ → return_to=/myaccount/ /app/team/123 → return_to=/app/team/123 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… shot
Bug 12 found in browser test: /app/ rendered "Employees" button linked
to /companies/null/employees/. The React launcher reads the first entry
from /whoami's admin.companies[]; with only an account row seeded, the
array was empty and the URL builder produced "null".
Seed tool now optionally connects to the company DB and creates:
- company (uuid, name, default_timezone, default_day_week_starts)
- team (uuid, company_uuid, name, timezone, day_week_starts, color)
- admin (company_uuid, user_uuid)
- directory (company_uuid, user_uuid, internal_id)
Idempotent: existing rows for the email / company-name are deleted first.
Verified: after reseed + re-login, /whoami's admin.companies has one
entry { uuid, name, default_timezone, default_day_week_starts }, so the
React launcher resolves to /companies/<real-uuid>/employees/ instead of
/companies/null/employees/.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Symptom: after login, /app/ rendered the Staffjoy logo + a single Employees button linked to /companies/null/employees/ — clicking it 404'd. Root cause: app/src/constants/paths.js (and myaccount/src/utility.js) constructed dev URLs as http://<service>.staffjoy-v2.local, the legacy subdomain pattern from before ADR-0004 ripped subdomains out server-side. Browsers can't resolve those hostnames, so: - app's getWhoAmI() fetched http://whoami.staffjoy-v2.local → DNS fail - Redux state.whoami.data stayed {} forever - Launcher's mapStateToProps returned companies=[] - SideNavigation rendered with companyUuid=undefined - URL builder produced /companies/null/employees/ Fix: in dev, route every microservice through Faraday on the current host using a path prefix that mirrors faraday/services/services.go: whoami → /whoami account → /api/account company → /api/company myaccount → /myaccount app → /app ical → /ical superpowers → /superpowers www → '' (catch-all at root) Result: relative URL — browser inherits scheme + host (works on whichever STAFFJOY_PORT was chosen, no subdomain DNS needed). Staging and prod keep their existing https://service.apex/ patterns. Verified by inspecting the rebuilt app bundle: /whoami present /api/account present /api/company present staffjoy-v2.local still appears (constant for staging/prod selectors, never reached in dev branch) App bundle hash changed (7eb6b55 → 486cd60) so the browser will pull the new bundle on next page load. Hard-refresh after deploy. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Run \`go run ./tools/seed -demo\` to wipe + rebuild a populated demo:
1 company Acme Diner
3 teams Front of House / Back of House / Bar
5 jobs Server, Host, Line Cook, Dishwasher, Bartender
1 manager manager@acme.local / acmedev (support=1, admin)
6 workers Alice, Bob, Carol, Dave, Eve, Frank
(Bob is in both Front of House + Bar — multi-team)
~116 shifts spread across the next 14 days, mix of:
- assigned (~70%) vs unassigned (~30%)
- published (current week) vs draft (next week)
- all 5 jobs represented
- extra weekend brunch shifts
Idempotent: each run wipes the prior demo's accounts (manager + 6
workers) and the Acme Diner company tree (company → admin / directory /
teams / jobs / workers / shifts), then rebuilds. UUIDs change on each
reseed, so cookies need refreshing in the browser.
Bare seed mode is unchanged: -email + -password (+ optional -company
/ -team) creates a single account.
Verified via curl after re-login:
/api/company/v1/companies/<uuid>/teams → 3 teams
/api/company/v1/companies/<uuid>/directory → 7 accounts
/api/company/v1/companies/<uuid>/teams/<t>/jobs → populated
/api/company/v1/companies/<uuid>/teams/<t>/workers → populated
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
| fmt.Println() | ||
| fmt.Println("Login at http://localhost:8080/login/ with:") | ||
| fmt.Printf(" email: %s\n", *email) | ||
| fmt.Printf(" password: %s\n", *password) |
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
CI on master ran 0 seconds with conclusion=failure on every push since v1.0.0.0 landed. The merged PR's CI also failed (0s, no jobs). "Likely failed because of a workflow file issue" — the runner refused to schedule any jobs.
This PR applies three empirical fixes to .github/workflows/ci.yml:
ubuntu-24.04→ubuntu-latestpnpm/action-setupbeforeactions/setup-node(canonical order in pnpm docs)'1.23.4','20.18.0','9.12.0') so YAML loaders don't coerce dotted strings to floatsAlso drops the
proto-checkjob entirely. It was gated onhashFiles('buf.yaml') != ''which evaluates pre-checkout (empty), so the job was always silently skipped — and that gate likely tripped GitHub's workflow validator. We don't have abuf.yamlyet (W3 deferred per TODOS.md). Revive proto-check as its own workflow when buf lands.Test plan
🤖 Generated with Claude Code