Check nightly that a new user can still reach a first cloud build - #5843
Open
shai-almog wants to merge 10 commits into
Open
shai-almog wants to merge 10 commits into
shai-almog wants to merge 10 commits into
Conversation
The onboarding starter is generated and personalised server-side, then resolves its dependencies over the network on the user's own machine. Neither half is visible to a check that reads the template out of a repository or replaces Maven with a stub, and both have broken in ways that made a first build impossible while every offline check stayed green: a generated pom that declared no dependency repository, so a pinned release resolved nothing once Codename One moved off Maven Central; launchers shipped without the execute bit, so the first command the README documents failed with "permission denied"; and a batch launcher that fell through to producing a local jar when given no target, so the user got no cloud build and no error either. Each survived for weeks because nothing exercised the artefact a user actually receives. So this signs in, downloads the personalised starter from the console, runs the shipped launcher with a real Maven, and requires the cloud build to reach success, on Linux and Windows, nightly. On failure it opens a single issue, assigns it, re-comments at most daily, and closes it on recovery -- the same pattern the syndication watchdog uses. Two details worth keeping. Success is read from the console's own build list rather than launcher stdout, because the upload client ships as a binary dependency whose wording is not in this repository and could change without notice. And a build is identified by an id that was absent before launch, since an account may retain only its most recent build, so counting rows would not do -- which is also what detects the local-jar case precisely: the launcher exits clean and no build ever appears. Apple targets are refused outright; they cost several times a normal build and a nightly run would exhaust the account. The assertions have offline tests, run on any PR that touches them, each pinned to the breakage it was written for. A canary that decays into a green check verifying nothing is the failure mode this is meant to end. Until the three secrets exist the scheduled run fails and opens the tracking issue, which is correct: an unconfigured canary is not a passing one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90d0613330
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ther Four corrections from review. The matrix legs were racing. The workflow-level concurrency group serialises separate runs, not the legs inside one run, and both legs share a single build account that allows one build in flight. Beyond a rejected submission, each leg identifies its own build by diffing the account's build list, so one could adopt the other's build and hide a launcher failure specific to its platform. They now run one at a time. On windows-latest the default shell is PowerShell, where a trailing backslash is not a line continuation, so python was handed a stray argument and the following two lines ran as separate commands -- the Windows leg would have failed before it ever touched the downloaded launcher. That step is explicitly bash now. Target names turn out not to be portable between launchers: the project archetype maps `javascript` to a local build and keeps a separate `javascript_cloud`, while the starter the console serves maps `javascript` straight to the cloud target. Rather than hard-code either convention, the canary now reads the launcher it was actually handed and fails immediately if the chosen target would build locally. Without that it would have spent the whole build poll waiting for a submission that was never coming and then blamed the starter. The check is exercised against both conventions in the tests. The Apple guard was matching server-side target strings, not the names the launchers accept, so a dispatch with `ios` sailed past it. It is an allowlist of cheap non-Apple targets now, so `ios`, `ios_release`, `ios_source`, `xcode`, `mac_native` and `mac_catalyst` are all refused, and one added later cannot slip through by not being on a blocklist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db90f052d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ptions Three more from review. The Windows leg would have died on its first line. Running under Git Bash there is no python3 shim, while setup-python puts `python` on PATH everywhere, so the step exited command-not-found and every Windows run would have reported the starter broken without once touching build.bat. android_source was in the allowlist, and it does not submit anything: its buildTarget is android-source, which generates an Android Studio project on the user's machine. The guard only looked for a `local-` prefix, so it let that through and the canary would have waited out the full build poll before raising a false outage. It now reads the resolved buildTarget and rejects the whole class -- `local-*` and `*-source` alike -- and android_source is off the allowlist beside ios_source and xcode. Authentication was pinned with the wrong property. The starter declares its Maven plugin through cn1.plugin.version, separately from cn1.version; they are equal today but they are separate knobs, and seeding the token with the framework version could resolve a plugin coordinate that was never published. The plugin property is read on its own now and falls back only if it is absent. Both launcher conventions stay covered, and the guards are checked against the real launchers: the served starter accepts javascript and linux_device and refuses android_source, while the archetype refuses javascript and accepts javascript_cloud. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 510d6bb5ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…e read Review caught that build.bat escapes the separator for cmd, spelling it buildTarget^=, while the regex accepted only a bare `=`. So on Windows nothing ever matched, and because an unreadable target was treated as acceptable the check quietly approved everything it was handed -- including a local target, which would then have cost the full build poll and produced a false outage. The caret is optional now, and an unreadable target is refused rather than assumed innocent: not being able to tell is not evidence that it submits. Verifying that against the real launchers turned up two more faults of the same kind, both mine. The label was matched as a plain substring, so ":ios" matched inside ":ios_source" and "function ios" inside "function ios_source". The check read a neighbouring target's buildTarget and ruled on the wrong one entirely -- `ios` came back as `android-source`. Labels are anchored to a whole line now. And the body was a fixed 400-character window, which runs off the end of a short function into the next one. ios_source only delegates to xcode, so it was being judged on android_source's buildTarget further down the file. The body now stops at the closing brace, or at the next label on Windows, so a delegating target reports that its own target cannot be read instead of borrowing a verdict. Both launchers are now exercised directly in the tests rather than fixtures alone: every allowlisted target resolves as cloud on build.sh and build.bat, android_source is refused as local, and ios_source is refused as unreadable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29c19ae0f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…report Drops the setup README. Operational runbooks -- how the account is provisioned, what has to be configured where -- do not belong in a public repository. The workflow still names its environment variables because a GitHub Actions workflow cannot reference a secret without naming it, and the script has to read them, but the procedure around them is not published here. The dispatch target was interpolated straight into the step body, where the shell evaluates it before python ever starts. A value like $(...) would have run as a command with all three credentials in the environment, and the target allowlist is enforced far too late to matter. Both inputs now arrive through env and are read as quoted shell variables. A rejected sign-in named the account in the failure text. That text is written to canary.json and copied verbatim into a public tracking issue, and Actions log masking reaches neither artifacts nor issue bodies -- so the first time sign-in broke, which is exactly when this runs, it would have published a configured credential. The message now reports the status and path only. The address is also passed to set-user-token as an argument, so captured output is redacted for it as well as the token, in case a tool echoes its arguments back in an error. The job timeout was 45 minutes while the canary allowed 60 for the launcher and another 30 for the poll, so a slow but healthy build would have been killed before it could write a report -- turning a delay into a reported outage. The two budgets are named constants now, they sum to 50, the job allows 70, and a test asserts the relationship so the next person to raise one finds out here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90331dc2fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Bounding the launcher and the poll left a third blocking phase uncounted: set-user-token went through run()'s 3600-second default, so a Maven that stalled resolving it could spend an hour before the thirty-minute launcher and twenty-minute poll had even started, and the runner would kill the job before python could write the report that says what went wrong. That phase is SEED_TIMEOUT now, ten minutes, and the three budgets sum to sixty against a seventy-minute job. run() no longer has a default timeout at all. An unbounded default is what let a phase escape the arithmetic in the first place, so every call site has to name a number that the budget test can add up, and the test asserts there is no default to fall back to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c5013a9ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The build client does not authenticate with the account's app token. It stores the JWT that /appsec/7.0/set-user mints and /poll-user hands back, so seeding it with the app token left it unauthenticated -- and what the client does then is worth recording: it printed a browser login URL, said it would wait five minutes, did not wait, reported "your build was submitted", and exited BUILD SUCCESSFUL after six seconds. No build existed on the server. Proven against production: the account's build list was empty afterwards. That is the local-jar failure again in a different component, and it is exactly why success is read from the console's build list rather than from launcher output. The canary called it correctly with no change: no new build id, so no build. The token is now minted per run from the session the canary already holds, the way the tooling itself obtains one. There is no long-lived build credential in repository secrets, nothing to rotate when it expires, and one fewer secret to configure -- email and password are enough. An unexpected exception also wrote no report at all, so a crash reached the alert job as "no report" rather than as what broke. Every exception is captured now, with the type and message in the report. Verified end to end against production: sign in, download the personalised starter, mint a token, run the shipped ./build.sh javascript, and wait for the cloud build to finish -- build 02ad7a37 succeeded in 282 seconds, comfortably inside the ten/thirty/twenty minute phase budgets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f08ec13c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…th it A timeout on the token-seeding step did not go through the redaction at all. subprocess raises TimeoutExpired before any output is returned, and that exception stringifies the entire command line -- which carries the minted token and the account address -- straight into the report the alert job copies into a public issue. The exception is caught now and never surfaced; the message says which phase timed out and shows redacted output. Killing a timed-out launcher only killed the launcher. It is a shell that execs mvnw, which execs a JVM, and that JVM can go on to submit a build minutes after its own leg has already failed -- which the next serialised leg would then see as a build id it did not create, recreating exactly the cross-leg confusion max-parallel exists to prevent. Each phase runs in its own process group and a timeout takes the group down, with the drain afterwards bounded so a survivor holding the pipe cannot hang the phase we just gave up on. An artefact-only dispatch could also close an outage. With skip_build the run reports ok, and the alert job read only that flag, so a diagnostic run would have closed an open issue and announced that both platforms completed a cloud build without one having been submitted. Recovery now requires stage "build"; an artefact-only run is inconclusive and leaves the issue exactly as it found it. Verified end to end against production after the rewrite, because this replaced the function every external command goes through: build bf1cfcf1 succeeded in 283 seconds, against 282 before it. The timeout tests are real rather than mocked -- one asserts a credential cannot appear in a timeout message, the other spawns a grandchild and asserts it does not outlive the kill. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4fee2f034
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Every subprocess inherited the whole environment, and one of the things in it was the account password. Maven resolves artifacts over the network and runs plugin code out of them, so a compromised dependency anywhere in the starter's tree could read a long-lived credential that sign-in had already finished with. Children get a sanitised environment now, without the canary credentials or the Actions tokens. The token still reaches set-user-token as an argument, so nothing has to travel in the environment at all. The Apple guard only ever checked the name the user asked for. If the served launcher regressed so javascript pointed at ios-device, the allowlist would wave it through and both legs would submit an Apple build every night at eight credits each -- roughly half the monthly allowance a week. The resolved buildTarget is checked against the same policy now, independently of the name that reached it. That change caught one of the existing tests asserting `ios` passes as a cloud target. It does resolve to one, but the test was written to prove `function ios` does not match inside `function ios_source`, and used Apple names only incidentally. It now makes the same point with android and android_source, so it still tests the anchoring without depending on a target the policy refuses. Loosening the policy to keep the assertion green would have had it backwards. Verified against production again, because this changed the environment every child runs in and no unit test proves Maven still works without those variables: build 119f38ca succeeded in 283 seconds, matching the previous two runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e32723a04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…e work The redaction added last round would have crashed instead of redacting. TimeoutExpired.output is bytes even when Popen ran with text=True -- its chunks are buffered before the newline translation that would have decoded them -- while communicate() hands back str, so concatenating the two raises TypeError. The generic handler would then have caught it and written "the canary crashed", throwing away the redacted diagnostics the fix existed to produce. A fix that only worked when it was not needed. Reproduced directly, and the regression test asserts the message says the phase timed out and not that the canary crashed. The budget also counted only the work, never the waiting. Sign-in, the starter download, both token-minting calls and the build-list polling all sit outside the three phases, and at a two-minute-per-request ceiling a slow but living endpoint could add twelve minutes to a sixty-minute budget under a seventy- minute job -- killed before writing a report, so latency would have been reported as an outage. Requests are bounded at a minute each now, the ten minutes they can collectively spend is named and included in the sum, and the job allows eighty-five. The budget test counts the HTTP allowance too, so the next phase added outside the arithmetic fails here rather than in the dark. Verified against production again, since this changed the timeout on every HTTP call the canary makes: build 85f52df6 succeeded in 299 seconds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Signs in, downloads the personalised onboarding starter from the console, runs
the shipped launcher with a real Maven, and requires the cloud build to reach
success — on Linux and Windows, nightly.
Why against the live service rather than a unit test
The starter is generated and personalised server-side, then resolves its
dependencies over the network on the user's own machine. Neither half is visible
to a check that reads the template out of a repository or replaces Maven with a
stub — and both have broken in ways that made a first build impossible while
every offline check stayed green:
resolved nothing once Codename One moved off Maven Central
permission deniedon the firstcommand the README documents
target: no cloud build, no error
Each survived for weeks because nothing exercised the artefact a user actually
receives.
Design notes
the upload client is a binary dependency whose wording isn't in this repo.
retain only its most recent build. That's also what detects the local-jar case
precisely: the launcher exits clean and no build ever appears.
pinned to the breakage it was written for.
pull_request, so forks spend no secretsand no build credits. Only the self-test runs here.
Before this is useful — needs a human
A dedicated canary account and three repository secrets
(
CN1_CANARY_EMAIL,CN1_CANARY_PASSWORD,CN1_CANARY_TOKEN).Setup in
scripts/ci/starter-canary/README.md. Until they exist the scheduledrun fails and opens the tracking issue — an unconfigured canary is not a passing
one.
🤖 Generated with Claude Code