Conversation
🦋 Changeset detectedLatest commit: 93f9225 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # then restore every manifest selected by main's workspace configuration so its | ||
| # lockfile and dependency graph remain internally consistent. | ||
| git ls-files | | ||
| grep -E '^(packages|examples|tools)/[^/]+/package[.]json$' | |
There was a problem hiding this comment.
The cleanup and restoration passes hard-code the current one-level workspace layout instead of deriving it from the restored pnpm-workspace.yaml. When main adds a nested importer, another workspace root, or a broader glob, its manifest will not be restored consistently, increasing maintenance cost and allowing the frozen install to reject the main lockfile.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/major-release-signoff.yml
Line: 220
Comment:
**Workspace pattern duplicates configuration**
The cleanup and restoration passes hard-code the current one-level workspace layout instead of deriving it from the restored `pnpm-workspace.yaml`. When main adds a nested importer, another workspace root, or a broader glob, its manifest will not be restored consistently, increasing maintenance cost and allowing the frozen install to reject the main lockfile.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
Review
Summary
Approve. Standards: 0 must-fix. Spec: 0 must-fix. Primary concern: none.
Review evidence
- Scope: reviewed HEAD, both commits, Jira and comments, repository guidance, and existing review threads.
- Method: separate sequential Standards, Spec, and Correctness passes; traced workspace restoration, release classification, provider direction, and the two controlled-reader workflows. No material simplification warranted expanding this repair.
| Behavior or check | Method / command | Result | Evidence source |
|---|---|---|---|
| Workflow regression coverage | bash .github/scripts/major-release-signoff.test.sh |
33 passed, 0 failed | Reviewer-run |
| Controlled reader | pnpm --filter @youversion/platform-react-ui exec vitest run --project unit src/components/bible-reader-controlled.test.tsx |
33 passed | Reviewer-run |
| Provider RTL inside hostile LTR host | pnpm --filter @youversion/platform-react-ui exec vitest run --project storybook src/components/bible-version-picker.shadow-isolation.stories.tsx |
8 passed in Chromium | Reviewer-run, synthetic Storybook app key |
| Stacked release classification | Executed scripts/preview-release.mjs with each merge base |
Main comparison reports introduced major; actual PR-base comparison correctly reports none | Reviewer-run; CI preview agrees |
- Limits: the first local browser attempt could not start because its Chromium executable was missing; installing the expected browser resolved it and the rerun passed. Historical PR #414/#415 heads were not independently rerun. The broader security redesign is explicitly out of scope.
- CI and bot review: build, lint, type checks, and release preview succeeded; UI CI passed 645 assertions. The existing workspace-pattern concern describes future maintenance; the patterns match current main. No duplicate comment added.
- Event: APPROVE.
Written by Code Reviewer bot on behalf of Cam.
|
|
||
| export const DirectionOnlyInheritanceRejectsHostVisualValues: Story = { | ||
| export const ProviderDirectionRejectsHostVisualValues: Story = { | ||
| globals: { interfaceDirection: 'rtl' }, |
There was a problem hiding this comment.
praise: Setting the provider to RTL inside an LTR host makes this test prove which direction the SDK follows.
For Agents: independent direction inputs
The explicit
interfaceDirection: 'rtl'and hostdir="ltr"distinguish provider-owned direction from accidental host inheritance. The story checks an RTL trigger and heading while the wrapper stays LTR, including after hostile styles are applied. All eight stories in this file passed in the local Chromium run.
Written by Code Reviewer bot on behalf of Cam.
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
Pre-emptively approved but thinking...
Dismissing at Cam’s request: not ready for approval yet.
|
|
||
| if grep -Fq 'BASE_SHA: ${{ needs.context.outputs.base_sha }}' "$WORKFLOW" && | ||
| grep -Fq 'git fetch --no-tags --force origin "$BASE_SHA"' "$WORKFLOW" && | ||
| grep -Fq 'BASE=$(git merge-base "$BASE_SHA" "$HEAD_SHA")' "$WORKFLOW"; then |
There was a problem hiding this comment.
suggestion: Replace this text check with one test that proves inherited breaking changes do not require signoff, but a breaking change added by this PR still does.
For Agents: exercise both release-preview outcomes
This block only checks that three strings appear somewhere in the workflow. It does not execute the comparison or verify the resulting decision. The current implementation behaved correctly in my manual comparison; this is a non-blocking regression-test improvement, not a reported defect.
Reuse this suite's temporary Git repository pattern and
extract_stephelper for one two-stage scenario:
- Create
main, then a target branch containing a major changeset, then a PR branch containing only a non-major change. Run the workflow's actualCompute release previewstep with the captured target/PR commit IDs and the real preview script. Assertintroduced_major: falseinpreview.jsonandis_major=0inGITHUB_OUTPUT.- Add a separate major changeset to that same PR branch. Keep the target commit fixed, update
HEAD_SHA, and rerun with a fresh output file. Assertintroduced_major: trueandis_major=1.Keep the remote local to the fixture and use minimal valid Changesets/package configuration. Do not hard-code the expected comparison base into a replacement preview implementation: the test should exercise the workflow's choice of base. Reverting to the old
maincomparison should fail stage 1; always reporting no introduced major should fail stage 2.This adds fixture setup compared with
grep, but protects the behavior described in YPE-5931 rather than the spelling of the implementation.
Written by Code Reviewer bot on behalf of Cam.
2 participants
Summary
This intentionally does not include the broader major-release-signoff security redesign from closed PR #416. That repository-wide work should be evaluated separately from a branch based on main.
YPE-5931
PR 414 and 415 failure coverage
Both PRs failed for the same two reasons:
The preview preparation and frozen-lockfile validation now pass against the immutable heads of both PRs:
Validation
The PR appears safe to merge.
Fix with agent prompt
Summary
This PR repairs major-release preview preparation and stabilizes the affected Shadow DOM and controlled-reader checks.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Resolve immutable PR context] --> B[Fetch trusted main] B --> C[Restore release tooling and dependency inputs] C --> D[Install with frozen lockfile] D --> E[Find merge base of PR base and head] E --> F[Compute release preview] F --> G{PR introduces a major release?} G -->|No| H[No signoff required] G -->|Yes| I[Enforce major-release signoff]Reviews (3) · Last reviewed commit: "test(ci): exercise stacked release previ..."