fix(release): resume drafts and normalize resolve-release outputs (FDE-714) - #2
Conversation
…E-714) The public distribution repository was running a stale export of the release workflow, and release artifacts were never attached to the v0.1.0 draft. Two defects were responsible. First, the resolve-release job derived its job outputs through `steps.passthrough.outputs.* || steps.derive.outputs.*`. On the public repository the passthrough step is skipped, so `release_created` never propagated, and the build-candidate and promote-release jobs were skipped on every dispatch. A dedicated normalization step now selects the derived or passthrough identity in shell based on the repository, so the job outputs are reliable. Second, the release lookup used the by-tag endpoint, which excludes draft releases, so each dispatch created a new empty draft instead of resuming the existing one. The new scripts/release_resolution.py lists all releases, including drafts, and selects a single safe action: create the tag and draft, create the draft on an existing tag, resume the one unpublished draft, or treat a published release as nothing to do. The release policy adds uploads.github.com to the fetch allowlist so the promotion job can upload artifacts. This ports the internal fix from FDE-714 (PR #106) verbatim for the release workflow, resolver, and its test. The release-policy change is scoped to the single allowlist entry so it does not touch unrelated operator-app assets.
📝 WalkthroughWalkthroughThe release pipeline now resolves immutable tags and release IDs, validates and reconciles assets, and publishes only after successful checks. The operator frontend now persists view generations and retirement state, supports recovery retries, rotates contested identities, and blocks terminal conflicts. ChangesRelease integrity pipeline
Operator view retirement and recovery
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant ReleaseWorkflow
participant GitHubAPI
participant PyPI
ReleaseWorkflow->>GitHubAPI: resolve draft by release ID
ReleaseWorkflow->>GitHubAPI: reconcile assets
ReleaseWorkflow->>GitHubAPI: publish validated release
ReleaseWorkflow->>PyPI: publish after promotion
sequenceDiagram
participant OperatorFrontend
participant SessionStorage
participant Backend
OperatorFrontend->>SessionStorage: restore retirement state
OperatorFrontend->>Backend: acknowledge retirement
Backend-->>OperatorFrontend: accept successor generation
OperatorFrontend->>SessionStorage: persist accepted generation
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Guard the RELEASING.md assertion behind an existence check so the exported test passes on the public repository, where RELEASING.md is intentionally absent. Ports the follow-up from internal PR #110 on top of the FDE-714 release workflow fix.
Clears the CodeQL incomplete-url-substring-sanitization alert on the hostname allowlist check. Carries the follow-up from internal PR #110.
kazazes
left a comment
kazazes
left a comment
There was a problem hiding this comment.
Approved for byte-preserving promotion. Candidate 3f2b266 was re-verified against private candidate 003004948923745a1905f7f962e1f4e54445c9fd and export tree SHA-256 c5588144ffe8f1dd0ce89a30603c6e3b839fbdd6ff1d4634a71d62eb957b8273 (514 exported, 26 withheld).
|
@codex review |
![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: 3f2b266aa0
ℹ️ 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".
| try { | ||
| await retireBrowserView(activeViewId, retiringGeneration); | ||
| acknowledgeViewRetirement(retiringGeneration); | ||
| } catch { | ||
| if (!pageActive) return; |
There was a problem hiding this comment.
If the backend restarts after pagehide stores a retirement marker but before the next page load, its in-memory _active_views and _retired_views no longer contain this generation, so /api/view/retire returns 409. This catch leaves retirementRequired set, returns before opening a successor socket, and every Reconnect click repeats the same failing retirement; the operator view remains permanently unusable until session storage is manually cleared. Handle the backend's “retirement is not proven” response by safely abandoning or rotating the persisted identity rather than retrying it forever.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Around line 554-567: Update the asset-upload curl invocation in the release
workflow to add a bounded connection/operation timeout and retries for transient
failures, including 5xx responses. Preserve the existing authenticated POST,
output handling, and reconciliation behavior, using curl options that avoid
unbounded stalls and limit retry attempts.
- Around line 150-159: Enable Bash pipefail for both release API pipeline steps
in the workflow, including the pipelines invoking gh api with jq or sort in the
release identity and asset reconciliation flows. Add set -o pipefail or
configure shell: bash with pipefail while preserving the existing pipeline
behavior.
In @.github/workflows/verify.yml:
- Around line 140-142: Update PLAYWRIGHT_WORKERS in .github/workflows/verify.yml
lines 140-142 to use the same gated expression as VERIFY_RELEASE_JOBS, yielding
4 workers only on main with vars.HEAVY_RUNNER and 1 otherwise; update the fixed
PLAYWRIGHT_WORKERS value in .github/workflows/documentation.yml line 19 to the
same expression.
In `@operator-app/frontend/src/main.ts`:
- Line 1146: Normalize generation values before the activeViewGeneration guard
and the restored-retirement marker comparison, matching the lowercase
canonicalization used by initialViewIdentity and the marker rewrite. Preserve
the existing acknowledgement and marker-clearing behavior while ensuring
mixed-case producer values compare consistently.
- Around line 1145-1158: Update the catch block in acknowledgeViewRetirement to
call forgetPersistedViewIdentity() instead of only setting
viewIdentityPersistent to false, ensuring the matching retirement marker and
persisted identity are cleared on storage-removal failure.
In `@operator-app/tests/operator.spec.ts`:
- Around line 1210-1241: The test should explicitly verify that the popup
initially inherits the opener’s sessionStorage identity before asserting
rotation. In the clone setup around window.open and the existing
picogrid-ecn-operator-view-id checks, capture the clone’s inherited identity and
assert it matches originalViewId, then retain the subsequent assertion that the
final identity differs and retirementGenerations remains empty.
- Around line 434-439: Update the seed requests in the Promise.all block to use
relative /api/config and /api/state URLs instead of hardcoded host and port
values, allowing page.request to resolve them against the configured baseURL.
- Around line 523-524: Strengthen the assertion after dispatching the click on
the disabled recover control by adding a bounded wait before checking
socketAttempts. Ensure the test allows time for any delayed connection attempt,
then verifies the count remains exactly 3 rather than passing immediately on its
initial evaluation.
- Around line 447-464: Update every routeWebSocket call site in operator.spec.ts
that currently matches '**/ws/state?view_id=*' to use the
query-order-independent /\/ws\/state\?/ pattern, including the shown WebSocket
route, while preserving the existing handlers and behavior.
In `@tests/release/test_release_workflow.py`:
- Around line 73-104: Add tests in the release-resolution suite for the
existing-tag path and malformed records: use a draft release with a non-current
tag to assert `_resolve` returns the `create-draft` action and expected payload,
and use a malformed release record to assert a nonzero return code with an “is
malformed” error. Preserve the existing test style and fixtures.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 67b5f74e-df9b-4b41-afa7-f5088ba46275
📒 Files selected for processing (8)
.github/workflows/documentation.yml.github/workflows/release.yml.github/workflows/verify.ymloperator-app/frontend/src/main.tsoperator-app/tests/operator.spec.tsscripts/release-policy.jsonscripts/release_resolution.pytests/release/test_release_workflow.py
| gh api \ | ||
| --paginate \ | ||
| --slurp \ | ||
| "repos/${GITHUB_REPOSITORY}/releases?per_page=100" | | ||
| jq 'add' > releases.json | ||
| python3 scripts/release_resolution.py \ | ||
| --tag "$tag" \ | ||
| --current-sha "$GITHUB_SHA" \ | ||
| --tag-sha "$tag_sha" \ | ||
| --releases-json releases.json > resolution.json |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
GitHub Actions default shell for run steps bash -e pipefail
💡 Result:
In GitHub Actions, the default behavior for run steps depends on how the shell is specified. When no shell is explicitly defined for a run step on Linux or macOS, GitHub Actions uses the internal command bash -e {0} [1][2]. While this includes the set -e flag (fail-fast on errors), it does not include pipefail [3][4]. When you explicitly specify shell: bash, GitHub Actions uses the command bash --noprofile --norc -eo pipefail {0} [1][2]. This configuration includes both set -e (exit on error) and set -o pipefail [3][4]. With pipefail enabled, a pipeline will return a non-zero exit status if any command in the pipeline fails, rather than only returning the exit status of the last command [3]. To ensure consistent behavior across your workflow, it is recommended to explicitly set the shell to bash in your workflow or job defaults [5]: defaults: run: shell: bash This ensures that all run steps use the bash --noprofile --norc -eo pipefail {0} command, providing both fail-fast and pipefail protections [3]. Note that for run steps inside a container, the default shell is sh instead of bash [6]. This can also be overridden using the same defaults configuration [6].
Citations:
- 1: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax
- 2: https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions
- 3: https://github.com/github/docs/blob/main/content/actions/reference/workflows-and-actions/workflow-syntax.md
- 4: https://stackoverflow.com/questions/75419587/does-a-github-action-step-use-set-e-semantics-by-default
- 5: https://docs.github.com/actions/writing-workflows/choosing-what-your-workflow-does/setting-a-default-shell-and-working-directory
- 6: https://docs.github.com/en/actions/how-tos/write-workflows/choose-where-workflows-run/run-jobs-in-a-container
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow structure and relevant sections ---'
sed -n '1,220p' .github/workflows/release.yml
printf '%s\n' '--- asset reconciliation area ---'
sed -n '520,610p' .github/workflows/release.yml
printf '%s\n' '--- shell/default declarations ---'
rg -n '(^|[[:space:]])(shell:|defaults:|run:|pipefail|gh api|releases\.json|sort)' .github/workflows/release.yml
printf '%s\n' '--- resolver implementation and references ---'
sed -n '1,260p' scripts/release_resolution.py
rg -n 'release_resolution|releases\.json|draft|duplicate|tag_sha|current_sha' .github/workflows/release.yml scriptsRepository: picogrid/ecn-sdk-python
Length of output: 25720
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- complete asset reconciliation step ---'
sed -n '470,585p' .github/workflows/release.yml
printf '%s\n' '--- available tools ---'
command -v bash || true
command -v jq || true
printf '%s\n' '--- pipeline behavior probe ---'
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/gh" <<'EOF'
#!/bin/sh
printf '%s\n' '[{"tag_name":"v1.2.3","draft":true}]'
exit 17
EOF
cat >"$tmpdir/jq" <<'EOF'
#!/bin/sh
cat
exit 0
EOF
chmod +x "$tmpdir/gh" "$tmpdir/jq"
set +e
PATH="$tmpdir:$PATH" bash -e -c 'gh api --paginate --slurp releases | jq add > releases.json'
without_pipefail=$?
PATH="$tmpdir:$PATH" bash -eo pipefail -c 'gh api --paginate --slurp releases | jq add > releases.json'
with_pipefail=$?
set -e
printf 'without_pipefail=%s with_pipefail=%s\n' "$without_pipefail" "$with_pipefail"
printf 'result_file='
cat releases.jsonRepository: picogrid/ecn-sdk-python
Length of output: 5595
Enable pipefail in both release API pipeline steps.
The default bash -e {0} masks a failed gh api when jq or sort exits successfully. Set shell: bash or add set -o pipefail to the release identity and asset reconciliation steps. Otherwise, incomplete release data can cause duplicate drafts and block later runs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 150 - 159, Enable Bash pipefail
for both release API pipeline steps in the workflow, including the pipelines
invoking gh api with jq or sort in the release identity and asset reconciliation
flows. Add set -o pipefail or configure shell: bash with pipefail while
preserving the existing pipeline behavior.
| curl \ | ||
| --fail-with-body \ | ||
| --silent \ | ||
| --show-error \ | ||
| --request POST \ | ||
| --header "Accept: application/vnd.github+json" \ | ||
| --header "Authorization: Bearer ${GH_TOKEN}" \ | ||
| --header "X-GitHub-Api-Version: 2022-11-28" \ | ||
| --header "Content-Type: application/octet-stream" \ | ||
| --data-binary "@${path}" \ | ||
| --output uploaded.json \ | ||
| "https://uploads.github.com/repos/${GITHUB_REPOSITORY}/releases/${RELEASE_ID}/assets?name=${encoded_name}" | ||
| test "$(jq -r '.name' uploaded.json)" = "$name" | ||
| done |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Add retry and a timeout to the asset upload.
This curl call has no retry and no timeout. One transient 5xx or a stalled connection from uploads.github.com fails the promotion after the step already deleted the matching assets. Retry is safe here, because the reconciliation loop deletes an existing asset with the same name before the upload.
♻️ Proposed fix
curl \
--fail-with-body \
--silent \
--show-error \
+ --retry 5 \
+ --retry-all-errors \
+ --connect-timeout 15 \
+ --max-time 600 \
--request POST \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| curl \ | |
| --fail-with-body \ | |
| --silent \ | |
| --show-error \ | |
| --request POST \ | |
| --header "Accept: application/vnd.github+json" \ | |
| --header "Authorization: Bearer ${GH_TOKEN}" \ | |
| --header "X-GitHub-Api-Version: 2022-11-28" \ | |
| --header "Content-Type: application/octet-stream" \ | |
| --data-binary "@${path}" \ | |
| --output uploaded.json \ | |
| "https://uploads.github.com/repos/${GITHUB_REPOSITORY}/releases/${RELEASE_ID}/assets?name=${encoded_name}" | |
| test "$(jq -r '.name' uploaded.json)" = "$name" | |
| done | |
| curl \ | |
| --fail-with-body \ | |
| --silent \ | |
| --show-error \ | |
| --retry 5 \ | |
| --retry-all-errors \ | |
| --connect-timeout 15 \ | |
| --max-time 600 \ | |
| --request POST \ | |
| --header "Accept: application/vnd.github+json" \ | |
| --header "Authorization: Bearer ${GH_TOKEN}" \ | |
| --header "X-GitHub-Api-Version: 2022-11-28" \ | |
| --header "Content-Type: application/octet-stream" \ | |
| --data-binary "@${path}" \ | |
| --output uploaded.json \ | |
| "https://uploads.github.com/repos/${GITHUB_REPOSITORY}/releases/${RELEASE_ID}/assets?name=${encoded_name}" | |
| test "$(jq -r '.name' uploaded.json)" = "$name" | |
| done |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 554 - 567, Update the
asset-upload curl invocation in the release workflow to add a bounded
connection/operation timeout and retries for transient failures, including 5xx
responses. Preserve the existing authenticated POST, output handling, and
reconciliation behavior, using curl options that avoid unbounded stalls and
limit retry attempts.
| env: | ||
| PLAYWRIGHT_WORKERS: "4" | ||
| VERIFY_RELEASE_JOBS: ${{ vars.HEAVY_RUNNER && '4' || '1' }} | ||
| VERIFY_RELEASE_JOBS: ${{ github.ref == 'refs/heads/main' && vars.HEAVY_RUNNER && '4' || '1' }} |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Playwright parallelism no longer matches the selected runner class. Both workflows now select vars.HEAVY_RUNNER only for refs/heads/main and fall back to ubuntu-latest elsewhere, but each still starts 4 Playwright workers. Pull request runs oversubscribe a 2-vCPU runner and produce timeouts and flaky browser tests. .github/workflows/release.yml already ties the worker count to the same condition.
.github/workflows/verify.yml#L140-L142: setPLAYWRIGHT_WORKERSto${{ github.ref == 'refs/heads/main' && vars.HEAVY_RUNNER && '4' || '1' }}, matching the adjacentVERIFY_RELEASE_JOBSexpression..github/workflows/documentation.yml#L19-L19: replace the fixedPLAYWRIGHT_WORKERS: "4"value at Line 24 with the same gated expression.
📍 Affects 2 files
.github/workflows/verify.yml#L140-L142(this comment).github/workflows/documentation.yml#L19-L19
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/verify.yml around lines 140 - 142, Update
PLAYWRIGHT_WORKERS in .github/workflows/verify.yml lines 140-142 to use the same
gated expression as VERIFY_RELEASE_JOBS, yielding 4 workers only on main with
vars.HEAVY_RUNNER and 1 otherwise; update the fixed PLAYWRIGHT_WORKERS value in
.github/workflows/documentation.yml line 19 to the same expression.
| function acknowledgeViewRetirement(generation: string): void { | ||
| if (activeViewGeneration !== generation) return; | ||
| acknowledgedRetirementGeneration = generation; | ||
| activeViewAcceptedByDocument = false; | ||
| retirementRequired = false; | ||
| if (!viewIdentityPersistent) return; | ||
| try { | ||
| if (window.sessionStorage.getItem(viewRetirementStorageKey) === generation) { | ||
| window.sessionStorage.removeItem(viewRetirementStorageKey); | ||
| } | ||
| } catch { | ||
| viewIdentityPersistent = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clear the persisted identity when the retirement marker cannot be removed.
Every other storage-failure handler in this change calls forgetPersistedViewIdentity(). This handler only sets viewIdentityPersistent = false and leaves the retirement marker in sessionStorage. The marker still matches the persisted generation, so initialViewIdentity takes the restored-retirement branch on the next load and returns retirementPending: true. The successor document then retires a generation that the backend already retired. retireBrowserView rejects that request, and the successor lands in the mandatory-reconnect state with no socket.
Use the same fail-closed helper here.
🛠️ Proposed fix
if (!viewIdentityPersistent) return;
try {
if (window.sessionStorage.getItem(viewRetirementStorageKey) === generation) {
window.sessionStorage.removeItem(viewRetirementStorageKey);
}
} catch {
- viewIdentityPersistent = false;
+ forgetPersistedViewIdentity();
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator-app/frontend/src/main.ts` around lines 1145 - 1158, Update the catch
block in acknowledgeViewRetirement to call forgetPersistedViewIdentity() instead
of only setting viewIdentityPersistent to false, ensuring the matching
retirement marker and persisted identity are cleared on storage-removal failure.
| } | ||
|
|
||
| function acknowledgeViewRetirement(generation: string): void { | ||
| if (activeViewGeneration !== generation) return; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Align the generation comparison with the canonicalization performed at load.
initialViewIdentity lowercases the restored generation, and the restored-retirement branch rewrites the marker in lowercase. This guard and the marker check at Line 1152 both use exact case-sensitive equality. The comparisons agree only because every producer now emits lowercase values. If any future producer stores a mixed-case generation, the acknowledgement is silently skipped and the marker is never cleared.
Consider comparing normalized values, or add a short comment that all generations are canonical lowercase.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator-app/frontend/src/main.ts` at line 1146, Normalize generation values
before the activeViewGeneration guard and the restored-retirement marker
comparison, matching the lowercase canonicalization used by initialViewIdentity
and the marker rewrite. Preserve the existing acknowledgement and
marker-clearing behavior while ensuring mixed-case producer values compare
consistently.
| const [configurationResponse, stateResponse] = await Promise.all([ | ||
| page.request.get('http://127.0.0.1:8080/api/config'), | ||
| page.request.get('http://127.0.0.1:8080/api/state'), | ||
| ]); | ||
| const configuration = await configurationResponse.json(); | ||
| const state = await stateResponse.json(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use relative URLs for the seed requests.
These two requests hardcode http://127.0.0.1:8080. Other tests in this file request /api/state relatively, for example Line 1044 and Line 1093. page.request resolves relative URLs against the configured baseURL. The hardcoded host makes this test fail if the Playwright webServer port changes.
♻️ Proposed change
const [configurationResponse, stateResponse] = await Promise.all([
- page.request.get('http://127.0.0.1:8080/api/config'),
- page.request.get('http://127.0.0.1:8080/api/state'),
+ page.request.get('/api/config'),
+ page.request.get('/api/state'),
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [configurationResponse, stateResponse] = await Promise.all([ | |
| page.request.get('http://127.0.0.1:8080/api/config'), | |
| page.request.get('http://127.0.0.1:8080/api/state'), | |
| ]); | |
| const configuration = await configurationResponse.json(); | |
| const state = await stateResponse.json(); | |
| const [configurationResponse, stateResponse] = await Promise.all([ | |
| page.request.get('/api/config'), | |
| page.request.get('/api/state'), | |
| ]); | |
| const configuration = await configurationResponse.json(); | |
| const state = await stateResponse.json(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator-app/tests/operator.spec.ts` around lines 434 - 439, Update the seed
requests in the Promise.all block to use relative /api/config and /api/state
URLs instead of hardcoded host and port values, allowing page.request to resolve
them against the configured baseURL.
| await page.routeWebSocket('**/ws/state?view_id=*', (webSocket) => { | ||
| socketAttempts += 1; | ||
| if (socketAttempts === 1) { | ||
| setTimeout(() => webSocket.send(JSON.stringify(state)), 50); | ||
| return; | ||
| } | ||
| if (socketAttempts === 2) { | ||
| void webSocket.close({ | ||
| code: 1011, | ||
| reason: 'synthetic successor failure', | ||
| }); | ||
| return; | ||
| } | ||
| void webSocket.close({ | ||
| code: 1013, | ||
| reason: 'operator view identity is already in use', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Playwright routeWebSocket URL glob pattern question mark query string matching
💡 Result:
In Playwright, the question mark (?) character in a glob pattern is specifically defined to match a literal question mark (?) only [1]. It does not act as a wildcard for "any single character." If you need to match a URL containing a query string (for example, /path?query=1), you should handle the question mark explicitly. To match a literal question mark in your glob pattern, you must escape it with a backslash (\?) [1][2]. Note that in many programming languages (like JavaScript/TypeScript), you will need to double-escape the backslash (\\?) to pass a literal backslash to the pattern matcher [3][4]. Key points regarding URL glob patterns in Playwright: - Literal Match: The ? symbol matches only the ? character [1]. - Escaping: Use \? to match a literal question mark in your pattern [1][2]. - Wildcard: If you intend to match any character (as a wildcard), use * instead of ? [1]. - Complexity: Playwright's glob implementation is simplified and can behave unexpectedly with complex URL structures because it is not fully URL-aware (for instance, it may not handle query string separation as expected by standard URL parsers) [3]. If your matching requirements involve complex query string logic or specific parameter matching, Playwright documentation and community practice recommend using a Regular Expression (RegExp) instead of a glob pattern [1]. This provides more precise control and avoids the limitations and potential ambiguity of the glob implementation [1][5]. This guidance applies to all network interception methods in Playwright, including page.route() and page.routeWebSocket() / browserContext.routeWebSocket() [1][6].
Citations:
- 1: https://playwright.dev/docs/network
- 2: https://github.com/microsoft/playwright-dotnet/blob/main/src/Playwright.Tests/InterceptionTests.cs
- 3: [Bug]: Incorrect behavior of '?' in the url glob pattern microsoft/playwright#34915
- 4: https://github.com/microsoft/playwright/blob/5790370e/tests/page/page-route.spec.ts
- 5: [Bug]: WebSocket route does not handle full URLs in Playwright microsoft/playwright#33085
- 6: https://playwright.dev/docs/api/class-browsercontext
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant files ---'
git ls-files | rg '(^|/)(operator\.spec\.ts|api\.ts|main\.ts)$'
printf '%s\n' '--- routeWebSocket occurrences ---'
rg -n -C 4 "routeWebSocket|ws/state\?view_id" operator-app/tests/operator.spec.ts operator-app/frontend/src/api.ts operator-app/frontend/src/main.ts 2>/dev/null || true
printf '%s\n' '--- URL construction and route setup ---'
rg -n -C 6 "stateWebSocket|view_generation|ws/state|stateSocketAcceptanceTimeoutMilliseconds" operator-app/frontend operator-app/tests 2>/dev/null || trueRepository: picogrid/ecn-sdk-python
Length of output: 27964
🏁 Script executed:
#!/bin/bash
set -eu
rg -n -C 8 "routeWebSocket|ws/state|view_generation|stateWebSocket" operator-app/tests/operator.spec.ts operator-app/frontend/src 2>/dev/null || trueRepository: picogrid/ecn-sdk-python
Length of output: 21527
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from urllib.parse import urlparse
pattern = '**/ws/state?view_id=*'
urls = [
'ws://127.0.0.1:8080/ws/state?view_id=view-1&view_generation=gen-1',
'ws://127.0.0.1:8080/ws/state?view_generation=gen-1&view_id=view-1',
'ws://127.0.0.1:8080/ws/state?view_id=view-1',
]
# Playwright URL-glob rules relevant here:
# ** matches any sequence, including '/'
# * matches any sequence except '/'
# ? is literal
# This direct matcher avoids executing repository code or requiring Playwright.
def matches(url: str) -> bool:
text = url
i = j = 0
while i < len(pattern):
if pattern.startswith('**', i):
if j == len(text):
return pattern[i + 2:] == ''
for k in range(len(text), j - 1, -1):
if matches_suffix(text, k, i + 2):
return True
return False
ch = pattern[i]
if ch == '*':
for k in range(j, len(text) + 1):
if '/' not in text[j:k] and matches_suffix(text, k, i + 1):
return True
return False
if j >= len(text) or text[j] != ch:
return False
i += 1
j += 1
return j == len(text)
def matches_suffix(text: str, start: int, pi: int) -> bool:
old = globals()['pattern']
rest = old[pi:]
# The only suffixes used below contain '*' and literals.
if rest == '':
return start == len(text)
if rest == '*':
return '/' not in text[start:]
if rest.endswith('*'):
prefix = rest[:-1]
return text.startswith(prefix, start) and '/' not in text[start + len(prefix):]
return text[start:] == rest
for url in urls:
print(f'{url} -> {matches(url)}')
PYRepository: picogrid/ecn-sdk-python
Length of output: 362
Use a query-order-independent routeWebSocket pattern
'**/ws/state?view_id=*' matches the current URL, but it fails if view_generation moves before view_id. Use /\/ws\/state\?/ at all matching call sites.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator-app/tests/operator.spec.ts` around lines 447 - 464, Update every
routeWebSocket call site in operator.spec.ts that currently matches
'**/ws/state?view_id=*' to use the query-order-independent /\/ws\/state\?/
pattern, including the shown WebSocket route, while preserving the existing
handlers and behavior.
| await recover.dispatchEvent('click'); | ||
| await expect.poll(() => socketAttempts).toBe(3); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Strengthen the negative assertion after the click on the disabled control.
socketAttempts already equals 3 at this point, so expect.poll(...).toBe(3) passes on its first evaluation. The assertion cannot detect a fourth socket that opens shortly after the dispatched click. Add a bounded wait before the check so the terminal guard is actually exercised.
💚 Proposed change
await recover.dispatchEvent('click');
- await expect.poll(() => socketAttempts).toBe(3);
+ await page.waitForTimeout(500);
+ expect(socketAttempts).toBe(3);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await recover.dispatchEvent('click'); | |
| await expect.poll(() => socketAttempts).toBe(3); | |
| await recover.dispatchEvent('click'); | |
| await page.waitForTimeout(500); | |
| expect(socketAttempts).toBe(3); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator-app/tests/operator.spec.ts` around lines 523 - 524, Strengthen the
assertion after dispatching the click on the disabled recover control by adding
a bounded wait before checking socketAttempts. Ensure the test allows time for
any delayed connection attempt, then verifies the count remains exactly 3 rather
than passing immediately on its initial evaluation.
| test('rotates a cloned identity before its initial socket opens', async ({ | ||
| context, | ||
| page, | ||
| }) => { | ||
| const retirementGenerations: string[] = []; | ||
| await context.route('**/api/view/retire', async (route) => { | ||
| const generation = route.request().headers()['x-operator-view-generation']; | ||
| if (generation) retirementGenerations.push(generation); | ||
| await route.fulfill({ json: { retired: true } }); | ||
| }); | ||
|
|
||
| await page.goto('/'); | ||
| await expect(page.getByTestId('connection-state')).toContainText('ready', { | ||
| timeout: 10_000, | ||
| }); | ||
| const originalViewId = await page.evaluate(() => | ||
| window.sessionStorage.getItem('picogrid-ecn-operator-view-id'), | ||
| ); | ||
| const popup = page.waitForEvent('popup'); | ||
| await page.evaluate(() => { | ||
| void window.open(window.location.href, '_blank'); | ||
| }); | ||
| const clone = await popup; | ||
| await expect(clone.getByTestId('connection-state')).toContainText('ready', { | ||
| timeout: 10_000, | ||
| }); | ||
| expect( | ||
| await clone.evaluate(() => | ||
| window.sessionStorage.getItem('picogrid-ecn-operator-view-id'), | ||
| ), | ||
| ).not.toBe(originalViewId); | ||
| expect(retirementGenerations).toEqual([]); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Make the cloned-identity precondition explicit.
This test relies on the popup inheriting the opener sessionStorage, which is what creates the contested identity. That inheritance is browser dependent. If a configured project does not copy sessionStorage into the new window, the clone starts with empty storage, generates an unrelated identity, and the assertion at Line 1236 passes without exercising the rotation path.
Assert the inherited state in the clone before checking the rotation, or record the assumption in a comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator-app/tests/operator.spec.ts` around lines 1210 - 1241, The test
should explicitly verify that the popup initially inherits the opener’s
sessionStorage identity before asserting rotation. In the clone setup around
window.open and the existing picogrid-ecn-operator-view-id checks, capture the
clone’s inherited identity and assert it matches originalViewId, then retain the
subsequent assertion that the final identity differs and retirementGenerations
remains empty.
| def test_release_resolution_uses_current_commit_only_before_tag_creation(tmp_path: Path) -> None: | ||
| result = _resolve(tmp_path, [], tag_sha="") | ||
|
|
||
| assert result.returncode == 0, result.stderr | ||
| assert json.loads(result.stdout) == { | ||
| "action": "create-tag-and-draft", | ||
| "html_url": "", | ||
| "release_id": None, | ||
| "release_created": True, | ||
| "release_sha": "b" * 40, | ||
| } | ||
|
|
||
|
|
||
| def test_release_resolution_stops_after_published_release(tmp_path: Path) -> None: | ||
| published = { | ||
| "id": 202, | ||
| "draft": False, | ||
| "html_url": "https://github.com/picogrid/ecn-sdk-python/releases/tag/v0.1.0", | ||
| "tag_name": "v0.1.0", | ||
| } | ||
|
|
||
| result = _resolve(tmp_path, [published]) | ||
|
|
||
| assert result.returncode == 0, result.stderr | ||
| assert json.loads(result.stdout) == { | ||
| "action": "published", | ||
| "release_id": 202, | ||
| "html_url": published["html_url"], | ||
| "release_created": False, | ||
| "release_sha": "a" * 40, | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add coverage for the create-draft and malformed-record paths.
The suite covers resume, create-tag-and-draft, published, and duplicates. It does not cover an existing tag with no release, which is the path that prevents a second draft on retry. It also does not cover a malformed release record.
♻️ Proposed additional tests
def test_release_resolution_creates_draft_for_existing_tag(tmp_path: Path) -> None:
result = _resolve(tmp_path, [{"id": 1, "draft": True, "html_url": "x", "tag_name": "v9.9.9"}])
assert result.returncode == 0, result.stderr
assert json.loads(result.stdout) == {
"action": "create-draft",
"html_url": "",
"release_id": None,
"release_created": True,
"release_sha": "a" * 40,
}
def test_release_resolution_refuses_malformed_release_record(tmp_path: Path) -> None:
result = _resolve(tmp_path, [{"id": 0, "draft": True, "html_url": "", "tag_name": "v0.1.0"}])
assert result.returncode != 0
assert "is malformed" in result.stderr🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/release/test_release_workflow.py` around lines 73 - 104, Add tests in
the release-resolution suite for the existing-tag path and malformed records:
use a draft release with a non-current tag to assert `_resolve` returns the
`create-draft` action and expected payload, and use a malformed release record
to assert a nonzero return code with an “is malformed” error. Preserve the
existing test style and fixtures.
3 participants
Why
The public repository is running a stale release workflow and has never produced release artifacts. The private upstream fixes are now promoted as one byte-preserving export.
resolve-releasenormalizes the private passthrough and public derived identities.!cancelled()plus explicit successful-needs checks, defeating the public repository's transitive skipped-release-pleasecondition without allowing failed prerequisites through.uploads.github.comendpoint.Promotion record
003004948923745a1905f7f962e1f4e54445c9fd3f2b266aa08cab84ee91a385114271c2abb946d1c5588144ffe8f1dd0ce89a30603c6e3b839fbdd6ff1d4634a71d62eb957b8273Acceptance
Releasedispatch builds once, promotes the exact verified artifacts, publishes the GitHub Release, and—when the separately controlled PyPI gate is approved—publishes only the client wheel and source distribution to PyPI.Summary by CodeRabbit
Bug Fixes
Chores