Skip to content

fix(ci): reject unconfirmed Glama builds - #860

Draft
ajitpratap0 wants to merge 1 commit into
mainfrom
fix/glama-build-trigger
Draft

ajitpratap0 wants to merge 1 commit into
mainfrom
fix/glama-build-trigger

Conversation

@ajitpratap0

Copy link
Copy Markdown
Owner

Summary

Glama returned an HTTP 200 validation page without creating a build, but the sync workflow printed “Glama build triggered” and completed successfully. Reject unconfirmed responses and let trigger failures fail the job. Success now requires a 302/303 redirect to a canonical Glama build-test URL; raw redirect targets and response bodies are not logged.

Send the current form's baseImage field and use a 20-second request timeout. Keep the optional missing-cookie warning/skip and the version-bump job's existing best-effort behavior.

Test Plan

  • task check (format, vet, lint, full race suite).
  • YAML syntax parsing, embedded Python compilation, and git diff --check.
  • Thirteen local mocked response/payload checks, including the observed HTTP 200 false success, auth and foreign redirects, malformed/non-build URLs, valid relative/absolute build URLs, pinned SHA, and build-only intent.
  • Independent targeted security review, with 19 additional URL edge cases and synthetic transport checks confirming redirects do not forward the cookie.
  • Verify a successful live build response. Glama's normal Build form rejects the valid v1.16.0 commit e9c5db91a8eda4f98d697a53dbd997ebd5255c87 with “Commit not found. The commit must exist in the repository,” even after Sync Server refreshes to descendant 326fdd15. The defensive redirect contract is inferred from existing build-test URLs and remains unconfirmed on a successful POST. Vendor support has the diagnostic evidence; no main/unpinned build was substituted.

The original workflow runs 39855149972 and 39855773623 both returned HTTP 200 with an empty Location and created no build.

Breaking Changes

None to the public API. A configured Glama trigger that returns an unconfirmed response now fails visibly instead of reporting success.

Stop treating HTTP 200 validation pages as successful build triggers.
Require a canonical Glama build-test redirect, expose trigger failures,
and avoid logging raw redirect targets or response bodies.

Send the current form's base image and bound the request timeout.
Preserve the documented missing-cookie warning and skip behavior.

Co-Authored-By: Codex <noreply@openai.com>
Codex-Session: 01a164e5-e59a-7281-b5b1-efc545a2f622
@vercel

vercel Bot commented Oct 22, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
gosqlx-website Ignored Ignored Oct 22, 2026 9:21pm UTC

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Claude finished @ajitpratap0's task in 21s —— View job


Review of PR #860: reject unconfirmed Glama builds

I reviewed the diff of .github/workflows/glama-sync.yml by reading it. I did not run the workflow or any tests. Overall this is a sound, defensive change.

What looks good

  • Strict success check. Requiring a 302/303 to a canonical /admin/dockerfile/tests/<uuid> URL fixes the false-success bug. The URL check is tight: exact prefix, canonical lowercase UUID, and the location must equal the bare path or https://glama.ai plus that path. That rejects query strings, fragments, foreign hosts and path tricks.
  • Cookie and logging. NoRedirect stops the session cookie from being forwarded. The script logs only the status code, never the raw Location or response body, so nothing sensitive leaks into the logs.
  • Failure handling. HTTPError and URLError/TimeoutError are caught and exit 1 with a clean message. The 20s timeout stops the job hanging. A 302 isn't raised as HTTPError because the NoRedirect handler returns it as a normal response, so the flow is correct.
  • Auth check. It now looks only at target.path, which avoids matching "auth" in a hostname or query string.

Concerns and suggestions

  1. Unverified success contract (main risk). As the PR says, no successful live POST has been observed. If Glama's real success response differs, for example a 200 with an HX-Redirect header, JSON, or a different URL shape, the job will now fail on every release. That is arguably the right failure mode, but the first real run will tell you. Consider noting the assumption in a comment above the check, with a pointer to this PR, so a future failure is easy to diagnose.
  2. continue-on-error removal. Now any Glama outage or cookie expiry turns the whole workflow red. That matches the stated intent. Please confirm no downstream job needs: this one. The version-bump job is described as staying best-effort, so check it doesn't depend on trigger-glama-build in a way that would now skip it.
  3. Hardcoded values. baseImage: debian:bookworm-slim, nodeVersion: 25 and the /mcp/servers/ajitpratap0/GoSQLX/... prefix are all literals. They are fine now, but the prefix will break if the repo moves or is renamed. Deriving it from github.repository would be more robust. This is minor.
  4. Redundant checks (nit). The auth keyword check is largely covered by the later strict prefix check; it only gives a more helpful message for the expired-cookie case. That's reasonable to keep.
  5. Test coverage. There is no automated test for the embedded Python. The 13 mocked checks and 19 URL edge cases from your plan are valuable. If you can, preserve them as a small script or fixture under scripts/ so the redirect validator doesn't regress. This is optional for a CI-only change.

Verdict: Looks good to merge, with the caveat that the success path is unconfirmed until Glama resolves the "Commit not found" issue. I'd re-verify with the first live run afterwards.
· branch fix/glama-build-trigger

@github-actions github-actions Bot added the ci-cd label Oct 22, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant