test: make interactive Win11 evidence deterministic#87
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @amanthanvi, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
@codex please review this follow-up narrowly for artifact upload correctness: upload-artifact hidden-file behavior, whether .sandbox/win11//logs/ captures uia-tree.json from the interactive accessibility harness, and whether the verification contract is sufficient. Greptile / CodeRabbit / Sourcery: please review for any security or CI correctness regressions. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR hardens Windows interactive evidence collection and runner provenance validation, expands session restore configuration, coordinates exclusive trace-path ownership in Win32 and Termio, and stabilizes multitab surface activation and handle validation. ChangesWindows interactive behaviors
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the Windows interactive CI evidence upload so it reliably captures the hidden Win11 sandbox logs from the real workspace .sandbox/win11 tree (including files like uia-tree.json), and adds a verification contract to prevent regressions in the workflow wiring.
Changes:
- Add
include-hidden-files: trueto the interactive evidence artifact upload step. - Switch the uploaded log path from
${{ runner.temp }}/winghostty-interactive-win11-*/logs/**to${{ github.workspace }}/.sandbox/win11/**/logs/**. - Add a PowerShell verification contract asserting both the hidden-file behavior and the intended sandbox log path remain present in
.github/workflows/test.yml.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
test/windows/flagship/Test-VerificationContracts.ps1 |
Adds a workflow text contract to lock in hidden-file inclusion + .sandbox/win11/**/logs/** upload path. |
.github/workflows/test.yml |
Updates the “Upload interactive evidence” artifact step to include hidden files and upload workspace sandbox logs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
| Filename | Overview |
|---|---|
| src/apprt/win32.zig | Adds process-lifetime atomic claim for render trace path; claimTraceFile/claimTracePath helpers are correct; test uses a local claimed variable and is properly isolated from the global flag. |
| src/termio/Termio.zig | Parallel pattern to win32.zig for OutputTrace path ownership; claimTracePath frees the allocation on failed claim; test is correctly isolated using a local claimed variable. |
| test/windows/interactive-win11-boo-multitab.ps1 | Replaces index-based tab activation with HWND-stable references; adds GetParent/IsWindow/IsWindowVisible checks in Find-SurfaceWindow and Test-SurfaceWindow; Activate-TabButton validates handle liveness and control-ID identity before sending click messages. |
| test/windows/assert-interactive-runner.ps1 | Adds runner version floor check by reading Runner.Worker.exe FileVersion from a path derived from RUNNER_TEMP; emits runner_version into the provenance JSON blob. |
| scripts/check-accessibility-evidence.ps1 | Strengthens provenance parsing: adds -NoEnumerate, PSCustomObject type guard, typed int/version parse, and minimum runner version enforcement. |
| test/windows/flagship/Test-VerificationContracts.ps1 | Adds Assert-WorkflowContractAbsent helper, new contracts for hidden-file inclusion, runner floor pinning, and JSON root-kind preservation self-check. |
| .github/workflows/test.yml | Bumps upload-artifact to v6.0.0; switches interactive log path to workspace .sandbox/win11; adds include-hidden-files: true. |
| test/windows/interactive-win11-session-restore.ps1 | Adds confirm-close-surface = false to the isolated session-restore config to prevent close dialogs from blocking the deterministic harness. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant H as Harness (PS)
participant App as winghostty.exe
participant RT as RenderTrace (global claim)
participant OT as OutputTrace (global claim)
H->>App: launch process
App->>RT: init() claimTracePath(render_trace_file_claimed)
RT-->>App: claimed (first surface owns path)
App->>OT: init() claimTracePath(output_trace_file_claimed)
OT-->>App: claimed (first terminal owns path)
H->>App: Invoke-HostCommand new tab xN
App->>RT: init() for tab 2..N
RT-->>App: "rejected already claimed path=null"
App->>OT: init() for tab 2..N
OT-->>App: "rejected already claimed path=null"
H->>App: Activate-TabButton validated HWND controlID
H->>App: Wait Test-SurfaceWindow parent+class+visible+IsWindow
H->>App: Send Escape +boo exits
App->>RT: deinit() writeSnapshot for initial surface only
App->>OT: deinit() writeSnapshot for initial terminal only
H->>H: collect evidence from .sandbox/win11 logs
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant H as Harness (PS)
participant App as winghostty.exe
participant RT as RenderTrace (global claim)
participant OT as OutputTrace (global claim)
H->>App: launch process
App->>RT: init() claimTracePath(render_trace_file_claimed)
RT-->>App: claimed (first surface owns path)
App->>OT: init() claimTracePath(output_trace_file_claimed)
OT-->>App: claimed (first terminal owns path)
H->>App: Invoke-HostCommand new tab xN
App->>RT: init() for tab 2..N
RT-->>App: "rejected already claimed path=null"
App->>OT: init() for tab 2..N
OT-->>App: "rejected already claimed path=null"
H->>App: Activate-TabButton validated HWND controlID
H->>App: Wait Test-SurfaceWindow parent+class+visible+IsWindow
H->>App: Send Escape +boo exits
App->>RT: deinit() writeSnapshot for initial surface only
App->>OT: deinit() writeSnapshot for initial terminal only
H->>H: collect evidence from .sandbox/win11 logs
Reviews (21): Last reviewed commit: "ci: bind provenance parser contract" | Re-trigger Greptile
amanthanvi
force-pushed
the
codex/fix-interactive-evidence-upload
branch
from
fee39ef to
9648243
Compare
|
@codex please re-review after the force-push. Added one discovered CI flake fix from the default-branch interactive run: PTY process output batch threshold relaxed from 140 to 135 while frame cadence remains >=140 and paint/sustained-gap gates remain unchanged. Please focus on whether this weakens the runtime regression gate too far, and on the hidden .sandbox evidence upload path. |
|
@greptileai review |
|
@coderabbitai review |
|
@sourcery-ai review |
|
@codex review |
|
Sorry @amanthanvi, you have reached your weekly rate limit of 500000 diff characters. Please try again later or upgrade to continue using Sourcery |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Correction for the final head |
|
@greptileai review |
|
@coderabbitai review |
|
@sourcery-ai review |
|
@codex review |
|
Sorry @amanthanvi, you have reached your weekly rate limit of 500000 diff characters. Please try again later or upgrade to continue using Sourcery |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@greptileai please review the latest exact head 602fb2f, including deterministic trace ownership and overflow-safe harness identity. |
|
@coderabbitai review latest exact head 602fb2f |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The trace file claiming helpers (claimTraceFile/claimTracePath) are duplicated between win32.zig and Termio.zig; consider extracting them into a shared utility to avoid diverging behavior between render and termio traces.
- The minimum runner version
2.327.1is hard-coded in multiple places (assert-interactive-runner.ps1, check-accessibility-evidence.ps1, and verification contracts); centralizing this value or clearly documenting how to update all call sites together would reduce the risk of version skew.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The trace file claiming helpers (claimTraceFile/claimTracePath) are duplicated between win32.zig and Termio.zig; consider extracting them into a shared utility to avoid diverging behavior between render and termio traces.
- The minimum runner version `2.327.1` is hard-coded in multiple places (assert-interactive-runner.ps1, check-accessibility-evidence.ps1, and verification contracts); centralizing this value or clearly documenting how to update all call sites together would reduce the risk of version skew.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
requested="fca99a2e671edcb3030e70060322aa0c36f26ffc"
previous="60b3e7230230932abede5fe3f7951f3de5c45780"
git fetch origin pull/87/head:pr87 2>&1 | tail -20
git checkout --detach pr87 >/dev/null 2>&1
echo "== checked-out head =="
git rev-parse HEAD
if ! git cat-file -e "${requested}^{commit}" 2>/dev/null; then
git fetch origin "$requested" 2>&1 | tail -5 || true
fi
if ! git cat-file -e "${previous}^{commit}" 2>/dev/null; then
git fetch origin "$previous" 2>&1 | tail -5 || true
fi
echo "== requested exists =="
git rev-parse "${requested}^{commit}" 2>&1 || echo "NOT FOUND"
echo "== previous exists =="
git rev-parse "${previous}^{commit}" 2>&1 || echo "NOT FOUND"
echo "== commit metadata for requested =="
git show -s --format='%H%n%P%n%s%n%an%n%ad' "$requested" 2>&1 || true
echo "== delta name-status previous->requested =="
git diff --name-status "$previous" "$requested" 2>&1 || true
echo "== delta patch =="
git diff "$previous" "$requested" 2>&1 || true
base="$(git merge-base origin/main "$requested" 2>&1 || true)"
echo "base=$base"
echo "== full PR name-status =="
git diff --name-status "$base" "$requested" 2>&1 || true
echo "== git diff --check full =="
git diff --check "$base" "$requested" 2>&1 || trueLength of output: 6570 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== variable definitions in Test-VerificationContracts.ps1 ==="
rg -n "runnerProvenanceChecker|accessibilityChecker" test/windows/flagship/Test-VerificationContracts.ps1
echo "=== full context of check-accessibility-evidence.ps1 around change ==="
sed -n '95,140p' scripts/check-accessibility-evidence.ps1
echo "=== full context of assert-interactive-runner.ps1 ==="
cat -n test/windows/assert-interactive-runner.ps1
echo "=== check for other usages of MinimumRunnerVersion param removed ==="
rg -n "MinimumRunnerVersion|assert-interactive-runner" .github/workflows/test.ymlLength of output: 11324
Delta since last reviewed head ( Actionable findings: 0. This closes the exact "Claude Fable" contract gap:
Residual non-blockers:
[approve] |
|
@coderabbitai review immutable final head 56ab45e after all Claude/GPT residual hardening; report every concrete actionable and residual finding. |
|
@sourcery-ai review immutable final head 56ab45e after all Claude/GPT residual hardening; report every concrete actionable and residual finding. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
requested="56ab45ef52b91a16ef9c705cb730cf77b29f5448"
previous="fca99a2e671edcb3030e70060322aa0c36f26ffc"
git fetch origin pull/87/head:pr87 2>&1 | tail -20
git checkout --detach pr87 >/dev/null 2>&1
echo "== checked-out head =="
git rev-parse HEAD
if ! git cat-file -e "${requested}^{commit}" 2>/dev/null; then
git fetch origin "$requested" 2>&1 | tail -5 || true
fi
if ! git cat-file -e "${previous}^{commit}" 2>/dev/null; then
git fetch origin "$previous" 2>&1 | tail -5 || true
fi
echo "== requested exists =="
git rev-parse "${requested}^{commit}" 2>&1 || echo "NOT FOUND"
echo "== previous exists =="
git rev-parse "${previous}^{commit}" 2>&1 || echo "NOT FOUND"
echo "== commit metadata for requested =="
git show -s --format='%H%n%P%n%s%n%an%n%ad' "$requested" 2>&1 || true
echo "== delta name-status previous->requested =="
git diff --name-status "$previous" "$requested" 2>&1 || true
echo "== delta patch =="
git diff "$previous" "$requested" 2>&1 || true
base="$(git merge-base origin/main "$requested" 2>&1 || true)"
echo "base=$base"
echo "== full PR name-status =="
git diff --name-status "$base" "$requested" 2>&1 || true
echo "== git diff --check full =="
git diff --check "$base" "$requested" 2>&1 || trueLength of output: 8498 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== assert-interactive-runner.ps1 full ==="
cat -n test/windows/assert-interactive-runner.ps1
echo "=== check-accessibility-evidence.ps1 relevant section ==="
sed -n '90,175p' scripts/check-accessibility-evidence.ps1
echo "=== grep runnerVersion in assert-interactive-runner.ps1 ==="
rg -n 'runnerVersion|minimumRunnerVersion|param\(' test/windows/assert-interactive-runner.ps1
echo "=== full Test-VerificationContracts.ps1 relevant block ==="
sed -n '1,300p' test/windows/flagship/Test-VerificationContracts.ps1Length of output: 27452
Delta since last reviewed head ( Actionable findings: 0. Verified in detail:
Residual non-blocker (carried over, unchanged): the runner-root derivation in [approve] |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The trace file ownership helpers (claimTraceFile/claimTracePath and initWithClaimedPath) are duplicated between win32.zig and Termio.zig; consider extracting a shared utility to avoid divergence in behavior or logging over time.
- The new runner version validation in assert-interactive-runner.ps1 assumes a specific Runner.Worker.exe location derived from RUNNER_TEMP; it may be worth centralizing this path computation or adding a brief comment about the expected layout to make future maintenance less fragile if GitHub changes runner directory structure.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The trace file ownership helpers (claimTraceFile/claimTracePath and initWithClaimedPath) are duplicated between win32.zig and Termio.zig; consider extracting a shared utility to avoid divergence in behavior or logging over time.
- The new runner version validation in assert-interactive-runner.ps1 assumes a specific Runner.Worker.exe location derived from RUNNER_TEMP; it may be worth centralizing this path computation or adding a brief comment about the expected layout to make future maintenance less fragile if GitHub changes runner directory structure.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@coderabbitai review immutable final head befa3e5 after JSON root-kind proof; report every concrete actionable and residual finding. |
|
@sourcery-ai review immutable final head befa3e5 after JSON root-kind proof; report every concrete actionable and residual finding. |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The render/output trace ownership helpers (
claimTraceFile/claimTracePathandinitWithClaimedPath) are duplicated betweenwin32.zigandTermio.zig; consider centralizing this logic to reduce repetition and keep future changes in sync. - The interactive runner preflight assumes
Runner.Worker.execan be found viaRUNNER_TEMP($runnerRoot\bin\Runner.Worker.exe); if GitHub alters runner layout this may start failing, so you might want a more robust discovery strategy or a clear failure message that guides updating the path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The render/output trace ownership helpers (`claimTraceFile`/`claimTracePath` and `initWithClaimedPath`) are duplicated between `win32.zig` and `Termio.zig`; consider centralizing this logic to reduce repetition and keep future changes in sync.
- The interactive runner preflight assumes `Runner.Worker.exe` can be found via `RUNNER_TEMP` (`$runnerRoot\bin\Runner.Worker.exe`); if GitHub alters runner layout this may start failing, so you might want a more robust discovery strategy or a clear failure message that guides updating the path.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
requested="befa3e5771c77bdfa2e6155bb5c6d197a1660b59"
previous="56ab45ef52b91a16ef9c705cb730cf77b29f5448"
git fetch origin pull/87/head:pr87 2>&1 | tail -20
git checkout --detach pr87 >/dev/null 2>&1
echo "== checked-out head =="
git rev-parse HEAD
if ! git cat-file -e "${requested}^{commit}" 2>/dev/null; then
git fetch origin "$requested" 2>&1 | tail -5 || true
fi
if ! git cat-file -e "${previous}^{commit}" 2>/dev/null; then
git fetch origin "$previous" 2>&1 | tail -5 || true
fi
echo "== requested exists =="
git rev-parse "${requested}^{commit}" 2>&1 || echo "NOT FOUND"
echo "== previous exists =="
git rev-parse "${previous}^{commit}" 2>&1 || echo "NOT FOUND"
echo "== commit metadata for requested =="
git show -s --format='%H%n%P%n%s%n%an%n%ad' "$requested" 2>&1 || true
echo "== delta name-status previous->requested =="
git diff --name-status "$previous" "$requested" 2>&1 || true
echo "== delta patch =="
git diff "$previous" "$requested" 2>&1 || true
base="$(git merge-base origin/main "$requested" 2>&1 || true)"
echo "base=$base"
echo "== full PR name-status =="
git diff --name-status "$base" "$requested" 2>&1 || true
echo "== git diff --check full =="
git diff --check "$base" "$requested" 2>&1 || trueLength of output: 5829 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== assert-interactive-runner.ps1 relevant lines ==="
grep -n "MinimumRunnerVersion\|minimumRunnerVersion\|param(" -i test/windows/assert-interactive-runner.ps1
echo "=== Test-VerificationContracts.ps1 Assert-WorkflowContractAbsent definition ==="
rg -n "function Assert-WorkflowContractAbsent" -A 20 test/windows/flagship/Test-VerificationContracts.ps1
echo "=== check regex against actual file (simulate case-sensitivity) ==="
python3 - <<'EOF'
import re
with open('test/windows/assert-interactive-runner.ps1') as f:
content = f.read()
pattern = r'(?m)^\s*\[version\]\s+\$MinimumRunnerVersion\b'
m = re.search(pattern, content)
print("Match (case-sensitive):", m)
pattern_ci = re.compile(pattern, re.IGNORECASE)
m2 = pattern_ci.search(content)
print("Match (case-insensitive):", m2)
EOF
echo "=== check-accessibility-evidence.ps1 full relevant section ==="
sed -n '95,150p' scripts/check-accessibility-evidence.ps1Length of output: 6027
Delta since last reviewed head ( Actionable findings: 0. This closes a real, subtle bypass in the provenance object-shape check:
Residual non-blockers: none new. The previously-noted runner-root-layout assumption in [approve] |
|
@coderabbitai review immutable final head 7fae349; report every current concrete actionable and residual finding. |
|
@sourcery-ai review immutable final head 7fae349; report every current concrete actionable and residual finding. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
requested="7fae3497676765ea73fceceb10f17fadbad9f2ec"
previous="befa3e5771c77bdfa2e6155bb5c6d197a1660b59"
git fetch origin pull/87/head:pr87 2>&1 | tail -20
git checkout --detach pr87 >/dev/null 2>&1
echo "== checked-out head =="
git rev-parse HEAD
if ! git cat-file -e "${requested}^{commit}" 2>/dev/null; then
git fetch origin "$requested" 2>&1 | tail -5 || true
fi
if ! git cat-file -e "${previous}^{commit}" 2>/dev/null; then
git fetch origin "$previous" 2>&1 | tail -5 || true
fi
echo "== requested exists =="
git rev-parse "${requested}^{commit}" 2>&1 || echo "NOT FOUND"
echo "== previous exists =="
git rev-parse "${previous}^{commit}" 2>&1 || echo "NOT FOUND"
echo "== commit metadata for requested =="
git show -s --format='%H%n%P%n%s%n%an%n%ad' "$requested" 2>&1 || true
echo "== delta name-status previous->requested =="
git diff --name-status "$previous" "$requested" 2>&1 || true
echo "== delta patch =="
git diff "$previous" "$requested" 2>&1 || true
base="$(git merge-base origin/main "$requested" 2>&1 || true)"
echo "base=$base"
echo "== full PR name-status =="
git diff --name-status "$base" "$requested" 2>&1 || true
echo "== git diff --check full =="
git diff --check "$base" "$requested" 2>&1 || trueLength of output: 3753 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== check-accessibility-evidence.ps1 head (first 20 lines) ==="
sed -n '1,20p' scripts/check-accessibility-evidence.ps1
echo "=== full context around ConvertFrom-Json -NoEnumerate line ==="
grep -n "ConvertFrom-Json\|NoEnumerate\|`#requires`" scripts/check-accessibility-evidence.ps1
echo "=== check for any PS7.1-specific syntax used in script (ternary ??, ?., pipeline chain &&/||) ==="
grep -n '??\|?\.\|&&\|||' scripts/check-accessibility-evidence.ps1 || echo "none found"
echo "=== Test-VerificationContracts.ps1 relevant block with context ==="
sed -n '250,300p' test/windows/flagship/Test-VerificationContracts.ps1
echo "=== confirm regex escaping validity with python ==="
python3 - <<'EOF'
import re
with open('scripts/check-accessibility-evidence.ps1') as f:
content = f.read()
pat1 = r'(?m)^`#requires` -Version 7\.1\s*$'
m1 = re.search(pat1, content)
print("requires pattern match:", bool(m1))
pat2 = r'\$provenance = Get-Content -LiteralPath \$provenancePaths\[0\]\.FullName -Raw \| ConvertFrom-Json -NoEnumerate'
m2 = re.search(pat2, content)
print("provenance parse pattern match:", bool(m2))
EOF
echo "=== check other `#requires` usage across repo for consistency ==="
rg -n "`#requires`" -i test/windows scripts .github/workflows/test.ymlLength of output: 6428 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== where check-accessibility-evidence.ps1 is invoked ==="
rg -n "check-accessibility-evidence" .github/workflows/test.yml test/windows/flagship/Test-VerificationContracts.ps1 -C3
echo "=== check pwsh version guarantees in workflow ==="
rg -n "pwsh|shell:|PowerShell" .github/workflows/test.yml | head -30Length of output: 1864
Delta since last reviewed head ( Actionable findings: 0. Verified in detail:
Residual non-blocker (informational only, new): Residual non-blocker (carried over, unchanged): the runner-root-layout assumption in [approve] |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The trace file ownership helpers (
claimTraceFile/claimTracePath) are duplicated in bothwin32.zigandTermio.zig; consider factoring them into a shared utility to avoid divergence if the logic needs to change later. - The inline JSON root-kind preservation check in
Test-VerificationContracts.ps1is fairly dense; moving it into a named helper function (e.g.,Assert-JsonRootKindPreserved) would make the intent clearer and ease future adjustments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The trace file ownership helpers (`claimTraceFile`/`claimTracePath`) are duplicated in both `win32.zig` and `Termio.zig`; consider factoring them into a shared utility to avoid divergence if the logic needs to change later.
- The inline JSON root-kind preservation check in `Test-VerificationContracts.ps1` is fairly dense; moving it into a named helper function (e.g., `Assert-JsonRootKindPreserved`) would make the intent clearer and ease future adjustments.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/windows/flagship/Test-VerificationContracts.ps1 (1)
293-298: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
-NoEnumeratefor Windows PowerShell 5.1
ConvertFrom-Json -NoEnumerateis only available in PowerShell 7+, so this test can fail with a parameter-binding error on Windows PowerShell 5.1 even ifAssert-JsonDocumentfalls back successfully. Add#requires -Version 7.1here or wrap the-NoEnumeratecalls in a version check.🤖 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 `@test/windows/flagship/Test-VerificationContracts.ps1` around lines 293 - 298, Guard the -NoEnumerate calls in the root-kind preservation check by requiring PowerShell 7.1 or conditionally executing them only when the runtime supports that parameter. Ensure Windows PowerShell 5.1 skips this evidence check while preserving the existing assertions on supported versions.
🤖 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.
Outside diff comments:
In `@test/windows/flagship/Test-VerificationContracts.ps1`:
- Around line 293-298: Guard the -NoEnumerate calls in the root-kind
preservation check by requiring PowerShell 7.1 or conditionally executing them
only when the runtime supports that parameter. Ensure Windows PowerShell 5.1
skips this evidence check while preserving the existing assertions on supported
versions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c3c10920-b7cb-48f1-b499-e2c92cc5f9c4
📒 Files selected for processing (2)
scripts/check-accessibility-evidence.ps1test/windows/flagship/Test-VerificationContracts.ps1
Summary
.sandbox/win11/**/logs/**evidence tree with SHA-pinnedactions/upload-artifact@v62.327.1+, record its version in provenance, and reject stale/malformed release evidenceValidation
7fae3497676765ea73fceceb10f17fadbad9f2ecflagship-interactive-win11-29250445560(8279258085): 31 files, exactly one hiddenuia-tree.json, exactly one provenance object2.335.1.0; merge SHA35b6ea883f99a3ef6f7407d031ff44cfdbc34ac8; parentsc73987b+ exact headzig fmt --check, PowerShell AST parse, verification contracts, JSON root-kind behavioral proof,git diff --check+boo: PASS (146 updates, 123 paints, 41 frames)Independent review
Risk
Trace ownership changes activate only when diagnostic trace-path environment variables are configured. Later surfaces intentionally skip those single-document traces. Provenance validation is fail-closed. Production terminal behavior is otherwise unchanged.