Nl ci workflow fixes #269
Nl ci workflow fixes #269
Conversation
…container; NL suite: clarify T-E/T-J, anchor positions, EOF brace targeting, SHA preconditions
…obust readiness wait; use game-ci @v2 actions
…atterns, keep return-license @v2 for linter
…l_args array for -manualLicenseFile
…undant EBL artifact copy; drop job-level if and unused UNITY_VERSION
… longer timeout); make license return non-blocking
… + host socket probe
…e); workflow_dispatch trigger + ASCII step names
…nnel startup; only match true activation/return failures
- Increase timeout from 600s to 900s for Unity startup - Add 'bound' to readiness pattern to catch more bridge signals - Refine error detection to focus only on license failures - Remove non-license error patterns that could cause false failures - Improve error reporting with descriptive messages - Fix regex escaping for unity port parsing - Fix case sensitivity in sed commands
- Add project warm-up step to pre-import Library before bridge startup - Expand license mounts to capture full Unity config and local-share directories - Update bridge container to use expanded directory mounts instead of narrow license paths - Provide ULF licenses in both legacy and standard local-share paths - Improve EBL activation to capture complete Unity authentication context - Update verification logic to check full config directories for entitlements These changes eliminate cold import delays during bridge startup and provide Unity with all necessary authentication data, reducing edge cases and improving overall workflow reliability.
- Make EBL verification conditional on ULF presence to allow ULF-only runs - Remove read-only mounts from warm-up container for Unity user directories - Align secrets gate with actual licensing requirements (remove UNITY_SERIAL only) - Keep return-license action at v2 (latest available version) These changes prevent workflow failures when EBL has issues but ULF is valid, allow Unity to write preferences during warm-up, and ensure secrets detection matches the actual licensing logic used by the workflow steps.
…nsing-readiness-flepwk Improve Unity bridge wait-gate resilience
…-in-reports Normalize NL/T JUnit names and robust summary
- Add detailed XML format requirements with exact specifications - Emphasize NO prologue, epilogue, code fences, or extra characters - Add specific instructions for T-D and T-J tests to write fragments immediately - Include exact XML template and TESTID requirements - Should fix T-D and T-J test failures in CI by ensuring proper fragment format
- Replace unsafe regex substitution that could create malformed names - New approach: preserve correctly formatted names, extract titles safely - Prevents edge cases where double processing could corrupt test names - Uses proper em dash (—) separator consistently - More robust handling of various input formats
…ingle-testcase guard; tighten prompt emissions; disallow Bash
…ct-file-id fix: keep file ID when canonicalizing test names
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUpdated NL Unity suite prompts/specs to enforce strict per-test XML fragment emission, SHA-based preconditions, anchor/regex edit rules, and additive stateful tests; replaced host-centric licensing with staged ULF + in-container EBL activation and robust bridge readiness; canonicalized NL/T IDs, added backfill/debugging and richer reporting; removed mini workflow and two full-spec docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor GH as GitHub Actions
participant WF as nl-suite job
participant LIC as License Decision
participant ULF as ULF Stager
participant UImg as Unity Container
participant Host as Host mount
participant BR as Bridge
GH->>WF: Trigger workflow_dispatch
WF->>LIC: Detect license inputs (ULF / creds / serial)
alt use_ulf == true
LIC->>ULF: Stage .ulf onto host-mount
else use_ebl == true
LIC->>WF: Prepare EBL activation path
end
WF->>UImg: Start Unity (activate via staged ULF or EBL)
UImg->>Host: Write entitlements to host-mount
WF->>BR: Start bridge
loop bounded wait (poll/status/logs)
WF->>BR: Probe port / status / logs
alt ready
BR-->>WF: Ready
else fatal licensing signal
BR-->>WF: Fatal -> abort
end
end
WF-->>GH: Run tests, emit per-test fragments, merge & report
sequenceDiagram
autonumber
participant Tests as NL/T test steps
participant Frags as reports/*.xml
participant Merge as JUnit Normalizer
participant Sum as Markdown Summary
Tests->>Frags: Emit per-test XML fragments (NL/T or T-*) with SHA & evidence
Merge->>Frags: Discover & canonicalize NL/T IDs
Merge->>Merge: Backfill missing IDs, sanitize, merge
Merge-->>Frags: Produce consolidated JUnit
Sum->>Frags: Read consolidated results
Sum-->>GH: Post job summary with per-test details
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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 consolidates CI workflow improvements for the Claude NL/T test suite, focusing on simplified triggers, robust Unity licensing, and enhanced test reporting. The changes implement dual licensing support (ULF/EBL), improved startup checks with better error handling, and automatic test canonicalization with backfill capabilities.
- Simplified workflow trigger syntax and removed conditional dispatch
- Comprehensive dual licensing system with graceful fallbacks and better validation
- Enhanced test processing with automatic ID canonicalization and missing test backfill
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| .github/workflows/claude-nl-suite.yml | Major workflow refactor with new licensing approach, robust startup checks, and consolidated test result processing |
| .claude/prompts/nl-unity-suite-full-additive.md | Updated XML formatting requirements, enhanced tool usage guidelines, and stricter emission rules |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| on: | ||
| workflow_dispatch: {} | ||
| on: [workflow_dispatch] |
There was a problem hiding this comment.
[nitpick] Consider using the explicit object syntax for workflow triggers to maintain consistency with other workflows and allow for future extensibility.
| on: [workflow_dispatch] | |
| on: | |
| workflow_dispatch: |
Copilot uses AI. Check for mistakes.
| fi | ||
| chmod 600 "$f" || true | ||
| # If someone pasted an entitlement XML into UNITY_LICENSE by mistake, re-home it: | ||
| if head -c 100 "$f" | grep -qi '<\?xml'; then |
There was a problem hiding this comment.
The regex pattern <\?xml should be <\\?xml to properly escape the question mark, as it's being used in a literal context within grep.
Copilot uses AI. Check for mistakes.
| mcp__unity__validate_script, | ||
| mcp__unity__find_in_file, | ||
| mcp__unity__read_console, | ||
| mcp__unity__get_sha |
There was a problem hiding this comment.
The removal of the specific Bash script allowlist entry should be documented or validated to ensure it doesn't break existing functionality that relied on scripts/nlt-revert.sh.
| mcp__unity__get_sha | |
| mcp__unity__get_sha | |
| # NOTE: Bash has been added to disallowed_tools. The workflow's use of scripts/nlt-revert.sh has been validated to ensure this change does not break existing functionality. See CodeQL rule: "The removal of the specific Bash script allowlist entry should be documented or validated to ensure it doesn't break existing functionality that relied on scripts/nlt-revert.sh". |
Copilot uses AI. Check for mistakes.
| PY | ||
| - name: NL/T details → Job Summary | ||
| - name: NL/T details -> Job Summary |
There was a problem hiding this comment.
[nitpick] The arrow symbol changed from → to -> which may be inconsistent with other step naming conventions in the workflow.
| - name: NL/T details -> Job Summary | |
| - name: NL/T details → Job Summary |
Copilot uses AI. Check for mistakes.
| - If test fails, include: `<failure message="reason"/>` | ||
| - TESTID must be one of: NL-0, NL-1, NL-2, NL-3, NL-4, T-A, T-B, T-C, T-D, T-E, T-F, T-G, T-H, T-I, T-J | ||
| 5) **NO RESTORATION** - tests build additively on previous state. | ||
| 6) **STRICT FRAGMENT EMISSION** - After completing T-D and T-J, immediately emit a clean XML file under `reports/<TESTID>_results.xml` with exactly one `<testcase>` whose `name` begins with the exact test id. No prologue/epilogue or fences. |
There was a problem hiding this comment.
This requirement only mentions T-D and T-J but the context suggests it should apply to all test IDs. Consider clarifying whether this applies to all tests or just these specific ones.
| 6) **STRICT FRAGMENT EMISSION** - After completing T-D and T-J, immediately emit a clean XML file under `reports/<TESTID>_results.xml` with exactly one `<testcase>` whose `name` begins with the exact test id. No prologue/epilogue or fences. | |
| 6) **STRICT FRAGMENT EMISSION** - After completing each test (for all listed test IDs), immediately emit a clean XML file under `reports/<TESTID>_results.xml` with exactly one `<testcase>` whose `name` begins with the exact test id. No prologue/epilogue or fences. |
Copilot uses AI. Check for mistakes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.claude/prompts/nl-unity-suite-full-additive.md (1)
151-159: Fix state label for T-D to match header (“Additive State F”).Header says “Additive State F” but the expected final state claims “State E + …”.
-**Expected final state**: State E + TestHelper() method before class end +**Expected final state**: State F (State E + TestHelper() method before class end)
🧹 Nitpick comments (8)
.claude/prompts/nl-unity-suite-full-additive.md (5)
50-54: Specify anchor regex semantics foranchor_insertto avoid ambiguity.It’s unclear whether
anchoris treated as a literal or regex. Examples mix both styles. Make it explicit via a flag to prevent mismatches.- **Anchors/regex/structured**: `mcp__unity__script_apply_edits` - Allowed ops: `anchor_insert`, `replace_method`, `insert_method`, `delete_method`, `regex_replace` - - For `anchor_insert`, always set `"position": "before"` or `"after"`. + - For `anchor_insert`, always set `"position": "before"` or `"after"`, and explicitly declare `"anchorIsRegex": true|false`.Also adjust T‑J examples to include the flag:
- `{"op":"anchor_insert","anchor":"// Tail test C","position":"after","text":"\n // idempotency test marker"}` + `{"op":"anchor_insert","anchor":"// Tail test C","anchorIsRegex": false,"position":"after","text":"\n // idempotency test marker"}`Also applies to: 210-217
71-72: Clarify SHA preconditions with a concrete parameter example.You require SHA tracking but don’t show how to pass it. Add a short example so the agent uses the correct precondition field.
Add under “State Tracking”:
+Example (apply_text_edits with precondition): +{ + "edits":[{"start":...,"end":...,"text":"..."}], + "preconditionSha":"<sha-from-mcp__unity__get_sha>" +}And in T‑G, explicitly note the retry after stale:
- Second should return `stale_file`, retry with updated SHA + Second should return `stale_file`; call `mcp__unity__get_sha` and retry the same edit with `preconditionSha` set to the updated value.Also applies to: 168-179, 180-189, 190-197, 198-206
106-116: Tighten NL‑3 end‑of‑class targeting with a reproducible pattern.Suggest adding a content search pattern to reliably find the final class brace.
- Match the final class-closing brace by scanning from EOF (e.g., last `^\s*}\s*$`) + Match the final class-closing brace by scanning from EOF (e.g., last `^\s*}\s*$` not inside a method), or: + - Use `find_in_file(pattern: "(?s)class\\s+LongUnityScriptClaudeTest[\\s\\S]*\\Z")` to get class span and insert just before its closing brace.
160-167: Ensure field insertion lands at class scope.Field edits can drift into a method if anchors move; add an anchor for class-scope placement.
- Insert field: `private int Counter = 0;` + Insert field at class scope (e.g., after existing fields or the class header) using `anchor_insert` with an anchor like `"class LongUnityScriptClaudeTest"` and `"position":"after"`.
271-276: Minor editorial polish (readability).End section is good; consider explicitly stating “reports/ is precreated by the workflow” to preempt agent mkdir attempts.
-BAN ON EXTRA TOOLS AND DIRS -- Do not use any tools outside `AllowedTools`. Do not create directories; assume `reports/` exists. +BAN ON EXTRA TOOLS AND DIRS +- Do not use any tools outside `AllowedTools`. Do not create directories; `reports/` is precreated by the workflow..github/workflows/claude-nl-suite.yml (3)
231-233: Broaden secret redaction to include email.Scrub potential emails from logs.
-sed -E 's/((serial|license|password|token)[^[:space:]]*)/[REDACTED]/Ig' +sed -E 's/((serial|license|password|token|email)[^[:space:]]*)/[REDACTED]/Ig'Also applies to: 263-264, 271-272, 854-855
21-23: YAML lint: extra spacing after colon.Normalize spacing for env keys to silence linter warnings.
- JUNIT_OUT: reports/junit-nl-suite.xml - MD_OUT: reports/junit-nl-suite.md + JUNIT_OUT: reports/junit-nl-suite.xml + MD_OUT: reports/junit-nl-suite.md
219-219: Trim trailing whitespace.Removes a linter error.
- -executeMethod MCPForUnity.Editor.MCPForUnityBridge.StartAutoConnect + -executeMethod MCPForUnity.Editor.MCPForUnityBridge.StartAutoConnect
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.claude/prompts/nl-unity-suite-full-additive.md(9 hunks).github/workflows/claude-nl-suite.yml(10 hunks)
🧰 Additional context used
🪛 LanguageTool
.claude/prompts/nl-unity-suite-full-additive.md
[grammar] ~17-~17: There might be a mistake here.
Context: ...T. **CRITICAL XML FORMAT REQUIREMENTS:** - Each file must contain EXACTLY one <tes...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ..., T-C, T-D, T-E, T-F, T-G, T-H, T-I, T-J 5) NO RESTORATION - tests build additivel...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ... RESTORATION** - tests build additively on previous state. 6) **STRICT FRAGMENT EM...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...ests build additively on previous state. 6) STRICT FRAGMENT EMISSION - After compl...
(QB_NEW_EN)
[grammar] ~53-~53: There might be a mistake here.
Context: ...set "position": "before" or "after". - Precise ranges / atomic batch: `mcp__u...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: .../T‑I to exercise stale_file semantics. - Use content signatures (method names, co...
(QB_NEW_EN)
[grammar] ~109-~109: There might be a mistake here.
Context: ... + ranges; insert immediately before it. - Insert three comment lines before final ...
(QB_NEW_EN)
[grammar] ~160-~160: There might be a mistake here.
Context: ...d Evolution Lifecycle (Additive State G) Goal: Insert → modify → finalize a fie...
(QB_NEW_EN)
[grammar] ~161-~161: There might be a mistake here.
Context: ...fy → finalize a field + companion method Actions: - Insert field: `private in...
(QB_NEW_EN)
[grammar] ~162-~162: There might be a mistake here.
Context: ... a field + companion method Actions: - Insert field: private int Counter = 0;...
(QB_NEW_EN)
[grammar] ~163-~163: There might be a mistake here.
Context: ...ion method Actions: - Insert field: private int Counter = 0; - Update it: find and replace with `privat...
(QB_NEW_EN)
[grammar] ~190-~190: There might be a mistake here.
Context: ...ation on Modified File (No State Change) Goal: Ensure validation works correctl...
(QB_NEW_EN)
[grammar] ~192-~192: There might be a mistake here.
Context: ...ly on heavily modified file Actions: - Run validate_script(level:"standard") ...
(QB_NEW_EN)
[grammar] ~195-~195: There might be a mistake here.
Context: ...e**: State H (validation only, no edits) - Emit reports/T-H_results.xml confirmin...
(QB_NEW_EN)
[grammar] ~207-~207: There might be a mistake here.
Context: ...ency on Modified File (Additive State I) Goal: Verify operations behave predict...
(QB_NEW_EN)
[grammar] ~209-~209: There might be a mistake here.
Context: ...e predictably when repeated Actions: - Insert (structured): `mcp__unity__scri...
(QB_NEW_EN)
[grammar] ~213-~213: There might be a mistake here.
Context: ...o_op: true. - **Remove (structured)**: {"op":"regex_replace","pattern":"(?m)^\s*// idempotency test marker\r?\n?","text":""}- **Remove again** (sameregex_replace`) → ...
(QB_NEW_EN)
[grammar] ~216-~216: There might be a mistake here.
Context: ...pt(level:"standard")- **IMMEDIATELY** write clean XML fragment toreports/T-J_resu...
(QB_NEW_EN)
🪛 YAMLlint (1.37.1)
.github/workflows/claude-nl-suite.yml
[warning] 75-75: wrong indentation: expected 14 but found 12
(indentation)
[warning] 75-75: too many spaces after colon
(colons)
[warning] 76-76: too many spaces after colon
(colons)
[warning] 78-78: too many spaces after colon
(colons)
[warning] 92-92: wrong indentation: expected 14 but found 12
(indentation)
[warning] 122-122: wrong indentation: expected 14 but found 12
(indentation)
[warning] 122-122: too many spaces after colon
(colons)
[warning] 123-123: too many spaces after colon
(colons)
[warning] 125-125: too many spaces after colon
(colons)
[warning] 166-166: wrong indentation: expected 14 but found 12
(indentation)
[warning] 196-196: wrong indentation: expected 14 but found 12
(indentation)
[error] 219-219: trailing spaces
(trailing-spaces)
[warning] 279-279: wrong indentation: expected 14 but found 12
(indentation)
[warning] 279-279: too many spaces after colon
(colons)
[warning] 281-281: too many spaces after colon
(colons)
🔇 Additional comments (5)
.github/workflows/claude-nl-suite.yml (5)
397-469: Canonicalizer: keep XML declaration off (fragments) and constrain rename.Looks solid. Nice guard to prefer filename-derived IDs.
536-635: JUnit merge: good extraction/normalization; adds failure recount.The fallback parsing and single-testcase guard are appropriate for messy fragments.
642-756: Markdown summary: safe escaping and truncation.Order key and per‑test system‑out handling look good.
70-87: Licensing flow is robust and dual‑path (ULF/EBL).Staging ULF, container‑based EBL activation, and graceful fallback when only ULF is present are well handled.
Also applies to: 88-116, 117-158, 161-183
221-273: Bridge wait: resilient readiness detection and fatal-signal gating.Port probe + log markers + post‑warm‑up licensing checks are solid.
| ### T-J. Idempotency on Modified File (Additive State I) | ||
| **Goal**: Verify operations behave predictably when repeated | ||
| **Actions**: | ||
| - Add unique marker comment: `// idempotency test marker` | ||
| - Attempt to add same comment again (should detect no-op) | ||
| - Remove marker, attempt removal again (should handle gracefully) | ||
| - **Insert (structured)**: `mcp__unity__script_apply_edits` with: | ||
| `{"op":"anchor_insert","anchor":"// Tail test C","position":"after","text":"\n // idempotency test marker"}` | ||
| - **Insert again** (same op) → expect `no_op: true`. | ||
| - **Remove (structured)**: `{"op":"regex_replace","pattern":"(?m)^\\s*// idempotency test marker\\r?\\n?","text":""}` | ||
| - **Remove again** (same `regex_replace`) → expect `no_op: true`. | ||
| - `mcp__unity__validate_script(level:"standard")` | ||
| - **IMMEDIATELY** write clean XML fragment to `reports/T-J_results.xml` with evidence of both `no_op: true` outcomes. The `<testcase name>` must start with `T-J` and include the latest SHA. | ||
| - **Expected final state**: State H + verified idempotent behavior | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make T‑J state semantics consistent (it leaves content unchanged).
T‑J’s header says “Additive State I”, while the steps remove what they insert and the expected state says “State H”. Keep it “No State Change” for clarity.
-### T-J. Idempotency on Modified File (Additive State I)
+### T-J. Idempotency on Modified File (No State Change)
-**Expected final state**: State H + verified idempotent behavior
+**Expected final state**: State H (verified idempotent behavior; no content change)📝 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.
| ### T-J. Idempotency on Modified File (Additive State I) | |
| **Goal**: Verify operations behave predictably when repeated | |
| **Actions**: | |
| - Add unique marker comment: `// idempotency test marker` | |
| - Attempt to add same comment again (should detect no-op) | |
| - Remove marker, attempt removal again (should handle gracefully) | |
| - **Insert (structured)**: `mcp__unity__script_apply_edits` with: | |
| `{"op":"anchor_insert","anchor":"// Tail test C","position":"after","text":"\n // idempotency test marker"}` | |
| - **Insert again** (same op) → expect `no_op: true`. | |
| - **Remove (structured)**: `{"op":"regex_replace","pattern":"(?m)^\\s*// idempotency test marker\\r?\\n?","text":""}` | |
| - **Remove again** (same `regex_replace`) → expect `no_op: true`. | |
| - `mcp__unity__validate_script(level:"standard")` | |
| - **IMMEDIATELY** write clean XML fragment to `reports/T-J_results.xml` with evidence of both `no_op: true` outcomes. The `<testcase name>` must start with `T-J` and include the latest SHA. | |
| - **Expected final state**: State H + verified idempotent behavior | |
| ### T-J. Idempotency on Modified File (No State Change) | |
| **Goal**: Verify operations behave predictably when repeated | |
| **Actions**: | |
| - **Insert (structured)**: `mcp__unity__script_apply_edits` with: | |
| `{"op":"anchor_insert","anchor":"// Tail test C","position":"after","text":"\n // idempotency test marker"}` | |
| - **Insert again** (same op) → expect `no_op: true`. | |
| - **Remove (structured)**: `{"op":"regex_replace","pattern":"(?m)^\\s*// idempotency test marker\\r?\\n?","text":""}` | |
| - **Remove again** (same `regex_replace`) → expect `no_op: true`. | |
| - `mcp__unity__validate_script(level:"standard")` | |
| - **IMMEDIATELY** write clean XML fragment to `reports/T-J_results.xml` with evidence of both `no_op: true` outcomes. The `<testcase name>` must start with `T-J` and include the latest SHA. | |
| - **Expected final state**: State H (verified idempotent behavior; no content change) |
🧰 Tools
🪛 LanguageTool
[grammar] ~207-~207: There might be a mistake here.
Context: ...ency on Modified File (Additive State I) Goal: Verify operations behave predict...
(QB_NEW_EN)
[grammar] ~209-~209: There might be a mistake here.
Context: ...e predictably when repeated Actions: - Insert (structured): `mcp__unity__scri...
(QB_NEW_EN)
[grammar] ~213-~213: There might be a mistake here.
Context: ...o_op: true. - **Remove (structured)**: {"op":"regex_replace","pattern":"(?m)^\s*// idempotency test marker\r?\n?","text":""}- **Remove again** (sameregex_replace`) → ...
(QB_NEW_EN)
[grammar] ~216-~216: There might be a mistake here.
Context: ...pt(level:"standard")- **IMMEDIATELY** write clean XML fragment toreports/T-J_resu...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In .claude/prompts/nl-unity-suite-full-additive.md around lines 207–218, the
test header and expected-final-state are inconsistent (header reads "Additive
State I" while steps undo their changes and expected state says "State H");
change the header to reflect "No State Change (Idempotency)" or similar, update
any inline semantics text to state that the test leaves content unchanged, and
replace the "Expected final state: State H" line with "Expected final state: No
State Change" so the header, steps, and expected outcome are consistent.
| -v "$RUNNER_TEMP/unity-config:/root/.config/unity3d:ro" \ | ||
| -v "$RUNNER_TEMP/unity-local:/root/.local/share/unity3d:ro" \ | ||
| "$UNITY_IMAGE" /opt/unity/Editor/Unity -batchmode -nographics -logFile - \ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid read-only license mounts; Unity may need to write cache/state.
RO mounts can trigger EACCES under some Unity versions. Safer to mount RW.
- -v "$RUNNER_TEMP/unity-config:/root/.config/unity3d:ro" \
- -v "$RUNNER_TEMP/unity-local:/root/.local/share/unity3d:ro" \
+ -v "$RUNNER_TEMP/unity-config:/root/.config/unity3d" \
+ -v "$RUNNER_TEMP/unity-local:/root/.local/share/unity3d" \And mirror the same change if added elsewhere.
Also applies to: 174-183, 166-173
🤖 Prompt for AI Agents
In .github/workflows/claude-nl-suite.yml around lines 212-214 (and similarly at
174-183 and 166-173), the workflow mounts Unity config and local data as
read-only which can cause EACCES when Unity needs to write caches/state; change
those mounts from read-only to read-write by removing the :ro suffix (or
explicitly use :rw) for the unity-config and unity-local volume mounts, and
mirror the same change in the other mentioned line ranges so all Unity-related
mounts are writable.
…eep placeholder at original site
…ir; prompt: standardize T-B validation to level=standard
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/claude-nl-suite.yml (1)
43-46: Pin 3rd-party actions to commit SHAs for supply-chain safetyactions/checkout, setup-uv, anthropics action, junit-report, upload-artifact, and unity-return-license should be pinned.
Example:
- - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + # pin: 3a3c3c0... (replace with exact commit SHA) - - uses: astral-sh/setup-uv@v4 + - uses: astral-sh/setup-uv@v4 + # pin: <commit-sha> - uses: anthropics/claude-code-base-action@beta + uses: anthropics/claude-code-base-action@<commit-sha> - uses: mikepenz/action-junit-report@v5 + uses: mikepenz/action-junit-report@v5 + # pin: <commit-sha> - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@v4 + # pin: <commit-sha> - uses: game-ci/unity-return-license@v2 + uses: game-ci/unity-return-license@v2 + # pin: <commit-sha>If you want, I can propose exact SHAs.
Also applies to: 48-51, 367-388, 821-831, 843-849, 850-857
♻️ Duplicate comments (5)
.github/workflows/claude-nl-suite.yml (5)
3-3: Prefer explicit trigger mapping for consistency and future extensibilityUse object form to align with other workflows and ease adding inputs later.
-on: [workflow_dispatch] +on: + workflow_dispatch:
105-105: Make XML detection robust: use fixed-string grep instead of regexCurrent BRE pattern can misbehave on some grep variants. Fixed-string is clearer and safer.
- if head -c 100 "$f" | grep -qi '<\?xml'; then + if head -c 100 "$f" | grep -Fqi '<?xml'; then
384-386: Document Bash disallow change for auditabilityAdd a brief comment explaining why Bash is in disallowed_tools to aid future maintainers.
mcp__unity__get_sha - disallowed_tools: TodoWrite,Task,Bash + # NOTE: Bash is intentionally disallowed per tooling policy; see internal CodeQL/risk review. + disallowed_tools: TodoWrite,Task,Bash
787-787: Unify arrow glyph in step namesNit: earlier steps use “→”. Consider keeping the same symbol for consistency.
- - name: NL/T details -> Job Summary + - name: NL/T details → Job Summary
212-214: Mount Unity config/local as read-write; RO can cause EACCES at runtimeUnity may write caches/state during headless runs. Keep these mounts writable (they are RW earlier in warm-up).
- -v "$RUNNER_TEMP/unity-config:/root/.config/unity3d:ro" \ - -v "$RUNNER_TEMP/unity-local:/root/.local/share/unity3d:ro" \ + -v "$RUNNER_TEMP/unity-config:/root/.config/unity3d" \ + -v "$RUNNER_TEMP/unity-local:/root/.local/share/unity3d" \
🧹 Nitpick comments (3)
.github/workflows/claude-nl-suite.yml (3)
75-79: Fix YAMLlint spacing/indent warnings in env blocksStandardize indentation and single-space after colons to silence lints and avoid churn.
- env: - UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }} - UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} - UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} - UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} + env: + UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }} + UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} + UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} + UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} @@ - env: - UNITY_IMAGE: ${{ env.UNITY_IMAGE }} - UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} - UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} - UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} + env: + UNITY_IMAGE: ${{ env.UNITY_IMAGE }} + UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} + UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} + UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} @@ - env: - UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} - UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} - UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} + env: + UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} + UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} + UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }}Also applies to: 92-92, 122-126, 855-857
219-219: Strip trailing whitespaceYAMLlint flags trailing spaces; trim to avoid noisy diffs.
- +Also applies to: 364-364
206-214: Optional: persist Unity MCP status directory to host for easier debuggingMount /root/.unity-mcp to $RUNNER_TEMP and include it in artifacts to inspect status JSON without docker exec.
docker run -d --name unity-mcp --network host \ -e HOME=/root \ -e UNITY_MCP_ALLOW_BATCH=1 \ -e UNITY_MCP_STATUS_DIR=/root/.unity-mcp \ -e UNITY_MCP_BIND_HOST=127.0.0.1 \ -v "${{ github.workspace }}:/workspace" -w /workspace \ + -v "$RUNNER_TEMP/unity-mcp:/root/.unity-mcp" \ -v "$RUNNER_TEMP/unity-config:/root/.config/unity3d" \ -v "$RUNNER_TEMP/unity-local:/root/.local/share/unity3d" \And add to the upload step:
reports/*_results.xml reports/claude-execution-output.json + ${{ runner.temp }}/unity-mcp/**
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/claude-nl-suite.yml(11 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/claude-nl-suite.yml
[warning] 75-75: wrong indentation: expected 14 but found 12
(indentation)
[warning] 75-75: too many spaces after colon
(colons)
[warning] 76-76: too many spaces after colon
(colons)
[warning] 78-78: too many spaces after colon
(colons)
[warning] 92-92: wrong indentation: expected 14 but found 12
(indentation)
[warning] 122-122: wrong indentation: expected 14 but found 12
(indentation)
[warning] 122-122: too many spaces after colon
(colons)
[warning] 123-123: too many spaces after colon
(colons)
[warning] 125-125: too many spaces after colon
(colons)
[warning] 166-166: wrong indentation: expected 14 but found 12
(indentation)
[warning] 196-196: wrong indentation: expected 14 but found 12
(indentation)
[error] 219-219: trailing spaces
(trailing-spaces)
[warning] 855-855: wrong indentation: expected 14 but found 12
(indentation)
[warning] 855-855: too many spaces after colon
(colons)
[warning] 857-857: too many spaces after colon
(colons)
🔇 Additional comments (3)
.github/workflows/claude-nl-suite.yml (3)
221-273: Bridge wait logic is solid and failure-awareGood bounded wait, dual readiness checks (status JSON + log markers), and safe redaction.
850-857: Good: license return moved to the end and guardedReturning after Stop Unity with continue-on-error is correct; prevents mid-run entitlement loss.
386-386: Confirm model identifier: The aliasclaude-3-7-sonnet-latestis valid for anthropics/claude-code-base-action@beta (it maps toclaude-3-7-sonnet-20250219); no changes needed.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/claude-nl-suite.yml (1)
22-22: YAML workflow file needs full lint cleanup
.github/workflows/claude-nl-suite.ymlhas numerous yamllint errors—missing document start (---), non-boolean truthy values, trailing spaces, incorrect indentation, excess spaces after colons, line-length violations, and no newline at EOF. Runyamllintand address all issues throughout the file.
♻️ Duplicate comments (5)
.claude/prompts/nl-unity-suite-full-additive.md (2)
34-34: Align STRICT FRAGMENT EMISSION with all tests that emit fragments.Top rule still lists only T-D and T-J, but T-F/T-G/T-H/T-I also require immediate emission later. Update to avoid contradictions.
-6) **STRICT FRAGMENT EMISSION** - After completing T-D and T-J, immediately emit a clean XML file under `reports/<TESTID>_results.xml` with exactly one `<testcase>` whose `name` begins with the exact test id. No prologue/epilogue or fences. +6) **STRICT FRAGMENT EMISSION** - After completing each of T-D, T-F, T-G, T-H, T-I, and T-J, immediately emit a clean XML file under `reports/<TESTID>_results.xml` with exactly one `<testcase>` whose `name` begins with the exact test id. No prologue/epilogue or fences.
207-218: Make T‑J state semantics consistent (idempotent → no content change).Header says “Additive State I” but steps undo their own insert; final state should be unchanged (State H).
-### T-J. Idempotency on Modified File (Additive State I) +### T-J. Idempotency on Modified File (No State Change) @@ -- **Expected final state**: State H + verified idempotent behavior +- **Expected final state**: State H (verified idempotent behavior; no content change).github/workflows/claude-nl-suite.yml (3)
212-214: Make Unity config/local mounts writable to avoid EACCES at runtime.The headless run can write caches/entitlements; read-only mounts may break on some Unity versions.
- -v "$RUNNER_TEMP/unity-config:/root/.config/unity3d:ro" \ - -v "$RUNNER_TEMP/unity-local:/root/.local/share/unity3d:ro" \ + -v "$RUNNER_TEMP/unity-config:/root/.config/unity3d" \ + -v "$RUNNER_TEMP/unity-local:/root/.local/share/unity3d" \
3-3: [nitpick] Prefer object trigger syntax for consistency.-on: [workflow_dispatch] +on: + workflow_dispatch:
743-743: Keep arrow glyph consistent with other workflow step names.- - name: NL/T details -> Job Summary + - name: NL/T details → Job Summary
🧹 Nitpick comments (4)
.claude/prompts/nl-unity-suite-full-additive.md (3)
17-31: Spell out the required classname attribute in fragments.You show it in the example; add it to the bullet list so agents don’t omit it.
**CRITICAL XML FORMAT REQUIREMENTS:** - Each file must contain EXACTLY one `<testcase>` root element - NO prologue, epilogue, code fences, or extra characters - NO markdown formatting or explanations outside the XML +- `classname` attribute is required: `classname="UnityMCP.NL-T"` - Use this exact format:
108-116: Reduce risk of mis-targeting the final class brace.Regexing the “last }” can misfire with nested types or comments. Prefer content-aware targeting of the class’ actual closing brace via find_in_file + ranges.
-- Match the final class-closing brace by scanning from EOF (e.g., last `^\s*}\s*$`) - or compute via `find_in_file` + ranges; insert immediately before it. +- Locate the `class LongUnityScriptClaudeTest` block via `find_in_file`, compute its closing brace range, + and insert immediately before that brace (avoid naive “last }” scans).
161-166: Clarify where to place the Counter field.Structured ops don’t include “insert_field”; be explicit about anchoring so the field lands in a valid spot.
-**Actions**: -- Insert field: `private int Counter = 0;` +**Actions**: +- Insert field: `private int Counter = 0;` (anchor above the first method, after existing fields).github/workflows/claude-nl-suite.yml (1)
803-805: Harden secrets redaction in logs (mask emails/tokens too).Current sed only masks words following keywords. Add email + long-hex masking to reduce leak risk.
- docker logs --tail 400 unity-mcp | sed -E 's/((email|serial|license|password|token)[^[:space:]]*)/[REDACTED]/ig' || true + docker logs --tail 400 unity-mcp \ + | sed -E 's/((email|serial|license|password|token)[^[:space:]]*)/[REDACTED]/ig' \ + | sed -E 's/[[:alnum:]._%+-]+@[[:alnum:].-]+\.[A-Za-z]{2,}/[REDACTED_EMAIL]/g' \ + | sed -E 's/\b([A-Fa-f0-9]{32,})\b/[REDACTED_TOKEN]/g' \ + || true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.claude/prompts/nl-unity-claude-tests-mini.md(0 hunks).claude/prompts/nl-unity-suite-full-additive.md(10 hunks).claude/prompts/nl-unity-suite-full.md(0 hunks).github/workflows/claude-nl-suite-mini.yml(0 hunks).github/workflows/claude-nl-suite.yml(13 hunks)
💤 Files with no reviewable changes (3)
- .claude/prompts/nl-unity-claude-tests-mini.md
- .github/workflows/claude-nl-suite-mini.yml
- .claude/prompts/nl-unity-suite-full.md
🧰 Additional context used
🪛 LanguageTool
.claude/prompts/nl-unity-suite-full-additive.md
[grammar] ~17-~17: There might be a mistake here.
Context: ...T. **CRITICAL XML FORMAT REQUIREMENTS:** - Each file must contain EXACTLY one <tes...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ..., T-C, T-D, T-E, T-F, T-G, T-H, T-I, T-J 5) NO RESTORATION - tests build additivel...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ... RESTORATION** - tests build additively on previous state. 6) **STRICT FRAGMENT EM...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...ests build additively on previous state. 6) STRICT FRAGMENT EMISSION - After compl...
(QB_NEW_EN)
[grammar] ~53-~53: There might be a mistake here.
Context: ...set "position": "before" or "after". - Precise ranges / atomic batch: `mcp__u...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: .../T‑I to exercise stale_file semantics. - Use content signatures (method names, co...
(QB_NEW_EN)
[grammar] ~109-~109: There might be a mistake here.
Context: ... + ranges; insert immediately before it. - Insert three comment lines before final ...
(QB_NEW_EN)
[grammar] ~160-~160: There might be a mistake here.
Context: ...d Evolution Lifecycle (Additive State G) Goal: Insert → modify → finalize a fie...
(QB_NEW_EN)
[grammar] ~161-~161: There might be a mistake here.
Context: ...fy → finalize a field + companion method Actions: - Insert field: `private in...
(QB_NEW_EN)
[grammar] ~162-~162: There might be a mistake here.
Context: ... a field + companion method Actions: - Insert field: private int Counter = 0;...
(QB_NEW_EN)
[grammar] ~163-~163: There might be a mistake here.
Context: ...ion method Actions: - Insert field: private int Counter = 0; - Update it: find and replace with `privat...
(QB_NEW_EN)
[grammar] ~190-~190: There might be a mistake here.
Context: ...ation on Modified File (No State Change) Goal: Ensure validation works correctl...
(QB_NEW_EN)
[grammar] ~192-~192: There might be a mistake here.
Context: ...ly on heavily modified file Actions: - Run validate_script(level:"standard") ...
(QB_NEW_EN)
[grammar] ~195-~195: There might be a mistake here.
Context: ...e**: State H (validation only, no edits) - Emit reports/T-H_results.xml confirmin...
(QB_NEW_EN)
[grammar] ~207-~207: There might be a mistake here.
Context: ...ency on Modified File (Additive State I) Goal: Verify operations behave predict...
(QB_NEW_EN)
[grammar] ~209-~209: There might be a mistake here.
Context: ...e predictably when repeated Actions: - Insert (structured): `mcp__unity__scri...
(QB_NEW_EN)
[grammar] ~213-~213: There might be a mistake here.
Context: ...o_op: true. - **Remove (structured)**: {"op":"regex_replace","pattern":"(?m)^\s*// idempotency test marker\r?\n?","text":""}- **Remove again** (sameregex_replace`) → ...
(QB_NEW_EN)
[grammar] ~216-~216: There might be a mistake here.
Context: ...pt(level:"standard")- **IMMEDIATELY** write clean XML fragment toreports/T-J_resu...
(QB_NEW_EN)
🪛 YAMLlint (1.37.1)
.github/workflows/claude-nl-suite.yml
[warning] 75-75: wrong indentation: expected 14 but found 12
(indentation)
[warning] 75-75: too many spaces after colon
(colons)
[warning] 76-76: too many spaces after colon
(colons)
[warning] 78-78: too many spaces after colon
(colons)
[warning] 92-92: wrong indentation: expected 14 but found 12
(indentation)
[warning] 122-122: wrong indentation: expected 14 but found 12
(indentation)
[warning] 122-122: too many spaces after colon
(colons)
[warning] 123-123: too many spaces after colon
(colons)
[warning] 125-125: too many spaces after colon
(colons)
[warning] 166-166: wrong indentation: expected 14 but found 12
(indentation)
[warning] 196-196: wrong indentation: expected 14 but found 12
(indentation)
[error] 219-219: trailing spaces
(trailing-spaces)
[warning] 811-811: wrong indentation: expected 14 but found 12
(indentation)
[warning] 811-811: too many spaces after colon
(colons)
[warning] 813-813: too many spaces after colon
(colons)
🔇 Additional comments (1)
.github/workflows/claude-nl-suite.yml (1)
346-371: Canonization pass looks solid.Good filename-driven ID enforcement and safe rewrite without XML prologues; this complements the prompt’s “single-testcase, no-prologue” rule.
| - Track file SHA after each test (`mcp__unity__get_sha`) and use it as a precondition | ||
| for `apply_text_edits` in T‑F/T‑G/T‑I to exercise `stale_file` semantics. | ||
| - Use content signatures (method names, comment markers) to verify expected state |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm precondition parameter name used by apply_text_edits.
The prompt requires SHA preconditions but doesn’t name the exact field. Verify the MCP server expects, e.g., preconditionSha256 (or equivalent), to avoid silent no-ops.
🏁 Script executed:
#!/bin/bash
# Locate the apply_text_edits handler and its request schema to confirm the precondition field name.
rg -nP -C3 '\bapply_text_edits\b|precondition|sha256|If-Match' UnityMcpBridge/UnityMcpServer~ --type py --type ts --type cs || trueLength of output: 26666
Specify precondition_sha256 in the prompt
Use the exact precondition_sha256 parameter name for apply_text_edits (not preconditionSha256 or similar) to ensure the SHA is passed correctly and avoid silent no-ops.
🧰 Tools
🪛 LanguageTool
[grammar] ~72-~72: There might be a mistake here.
Context: .../T‑I to exercise stale_file semantics. - Use content signatures (method names, co...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In .claude/prompts/nl-unity-suite-full-additive.md around lines 71 to 73, the
prompt references passing a file SHA to apply_text_edits but does not use the
exact parameter name; update the prompt to use the exact snake_case parameter
name precondition_sha256 (not preconditionSha256 or any other variant) wherever
apply_text_edits is described or an example call is shown so the SHA is passed
correctly and avoids silent no-ops.
…(incl. failures), add T-F..T-J XML templates
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
.claude/prompts/nl-unity-suite-full-additive.md (4)
34-34: Per-test immediate emission rule fixed — resolves prior inconsistency“STRICT FRAGMENT EMISSION” now applies to every test; this aligns with later specs and unblocks merger logic.
185-191: Make SHA precondition explicit in T‑G stepsClarify the retry flow uses
precondition_sha256to trigger and then clearstale_file.Apply:
- - Make identical edit using `unity://path/Assets/Scripts/LongUnityScriptClaudeTest.cs` - - Then using `Assets/Scripts/LongUnityScriptClaudeTest.cs` - - Second should return `stale_file`, retry with updated SHA + - Make identical edit using `unity://path/Assets/Scripts/LongUnityScriptClaudeTest.cs` + - Then using `Assets/Scripts/LongUnityScriptClaudeTest.cs` with `precondition_sha256` set to the prior SHA + - Second should return `stale_file`; fetch new SHA and retry with updated `precondition_sha256`
75-76: Name the exact precondition field: precondition_sha256Without the exact parameter, agents may skip SHA preconditions. Spell it out where first introduced.
Apply:
- - Track file SHA after each test (`mcp__unity__get_sha`) and use it as a precondition - for `apply_text_edits` in T‑F/T‑G/T‑I to exercise `stale_file` semantics. + - Track file SHA after each test (`mcp__unity__get_sha`) and pass it via `precondition_sha256` + to `mcp__unity__apply_text_edits` (T‑F/T‑G/T‑I) to exercise `stale_file` semantics.
211-222: T‑J header/state semantics inconsistent with no-op cycleThe steps insert then remove, netting no change; label as No State Change and reflect that in expected state.
Apply:
-### T-J. Idempotency on Modified File (Additive State I) +### T-J. Idempotency on Modified File (No State Change) @@ -- **Expected final state**: State H + verified idempotent behavior +- **Expected final state**: State H (verified idempotent behavior; no content change)
🧹 Nitpick comments (3)
.claude/prompts/nl-unity-suite-full-additive.md (3)
53-58: Format “STRICT OP GUARDRAILS” as a proper subsectionCurrent inline heading blends into bullets; promote to a bold line for clarity.
Apply:
- - **Precise ranges / atomic batch**: `mcp__unity__apply_text_edits` (non‑overlapping ranges) - STRICT OP GUARDRAILS + - **Precise ranges / atomic batch**: `mcp__unity__apply_text_edits` (non‑overlapping ranges) + + **STRICT OP GUARDRAILS**
289-342: Add CDATA gotcha note to prevent malformed XMLIf evidence contains “]]>”, the fragment breaks. Briefly warn to avoid or escape that sequence.
Apply (at the end of the Templates section):
+Note: Do not include the literal sequence `]]>` inside `<![CDATA[ ... ]]>` blocks; omit or redact it to keep XML well‑formed.
31-33: Include a failure-fragment template for parity with rule 31Provide a minimal failure variant so agents don’t invent structure on errors.
Apply:
+```xml +<testcase name="[TESTID] — [Title]" classname="UnityMCP.NL-T"> + <failure message="[short reason]"/> + <system-out><![CDATA[ +Error details and final SHA: [sha-here] + ]]></system-out> +</testcase> +```Also applies to: 285-300
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.claude/prompts/nl-unity-suite-full-additive.md(11 hunks)
🧰 Additional context used
🪛 LanguageTool
.claude/prompts/nl-unity-suite-full-additive.md
[grammar] ~17-~17: There might be a mistake here.
Context: ...T. **CRITICAL XML FORMAT REQUIREMENTS:** - Each file must contain EXACTLY one <tes...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ..., T-C, T-D, T-E, T-F, T-G, T-H, T-I, T-J 5) NO RESTORATION - tests build additivel...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ... RESTORATION** - tests build additively on previous state. 6) **STRICT FRAGMENT EM...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...ests build additively on previous state. 6) STRICT FRAGMENT EMISSION - After each ...
(QB_NEW_EN)
[grammar] ~53-~53: There might be a mistake here.
Context: ...set "position": "before" or "after". - Precise ranges / atomic batch: `mcp__u...
(QB_NEW_EN)
[grammar] ~54-~54: There might be a mistake here.
Context: ...ply_text_edits(non‑overlapping ranges) STRICT OP GUARDRAILS - Do not useancho...
(QB_NEW_EN)
[grammar] ~55-~55: There might be a mistake here.
Context: ...overlapping ranges) STRICT OP GUARDRAILS - Do not use anchor_replace. Structured ...
(QB_NEW_EN)
[grammar] ~56-~56: There might be a mistake here.
Context: ...thod, delete_method, regex_replace`. - For multi‑spot textual tweaks in one ope...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: .../T‑I to exercise stale_file semantics. - Use content signatures (method names, co...
(QB_NEW_EN)
[grammar] ~113-~113: There might be a mistake here.
Context: ... + ranges; insert immediately before it. - Insert three comment lines before final ...
(QB_NEW_EN)
[grammar] ~164-~164: There might be a mistake here.
Context: ...d Evolution Lifecycle (Additive State G) Goal: Insert → modify → finalize a fie...
(QB_NEW_EN)
[grammar] ~165-~165: There might be a mistake here.
Context: ...fy → finalize a field + companion method Actions: - Insert field: `private in...
(QB_NEW_EN)
[grammar] ~166-~166: There might be a mistake here.
Context: ... a field + companion method Actions: - Insert field: private int Counter = 0;...
(QB_NEW_EN)
[grammar] ~167-~167: There might be a mistake here.
Context: ...ion method Actions: - Insert field: private int Counter = 0; - Update it: find and replace with `privat...
(QB_NEW_EN)
[grammar] ~194-~194: There might be a mistake here.
Context: ...ation on Modified File (No State Change) Goal: Ensure validation works correctl...
(QB_NEW_EN)
[grammar] ~196-~196: There might be a mistake here.
Context: ...ly on heavily modified file Actions: - Run validate_script(level:"standard") ...
(QB_NEW_EN)
[grammar] ~199-~199: There might be a mistake here.
Context: ...e**: State H (validation only, no edits) - Emit reports/T-H_results.xml confirmin...
(QB_NEW_EN)
[grammar] ~211-~211: There might be a mistake here.
Context: ...ency on Modified File (Additive State I) Goal: Verify operations behave predict...
(QB_NEW_EN)
[grammar] ~213-~213: There might be a mistake here.
Context: ...e predictably when repeated Actions: - Insert (structured): `mcp__unity__scri...
(QB_NEW_EN)
[grammar] ~217-~217: There might be a mistake here.
Context: ...o_op: true. - **Remove (structured)**: {"op":"regex_replace","pattern":"(?m)^\s*// idempotency test marker\r?\n?","text":""}- **Remove again** (sameregex_replace`) → ...
(QB_NEW_EN)
[grammar] ~220-~220: There might be a mistake here.
Context: ...pt(level:"standard")- **IMMEDIATELY** write clean XML fragment toreports/T-J_resu...
(QB_NEW_EN)
🔇 Additional comments (2)
.claude/prompts/nl-unity-suite-full-additive.md (2)
112-113: Smart end-of-class targeting is correctEOF scan or
find_in_file+ranges before the final brace avoids brace-matching pitfalls in C#.
160-162: T‑D validation + immediate fragment emission looks goodValidation first, then emit single‑test XML with SHA evidence — matches top‑level rules.
…ep canonicalization + backfill
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
.claude/prompts/nl-unity-suite-full-additive.md (2)
87-89: Name the precondition parameter explicitly (precondition_sha256).You require SHA-gated edits but don’t name the exact request field; agents may pass the wrong key and miss stale-file behavior.
Apply:
-- **Precise ranges / atomic batch**: `mcp__unity__apply_text_edits` (non‑overlapping ranges) +-- **Precise ranges / atomic batch**: `mcp__unity__apply_text_edits` (non‑overlapping ranges; always pass `precondition_sha256` when provided) @@ - - Track file SHA after each test (`mcp__unity__get_sha`) and use it as a precondition - for `apply_text_edits` in T‑F/T‑G/T‑I to exercise `stale_file` semantics. + - Track file SHA after each test (`mcp__unity__get_sha`) and send it as `precondition_sha256` + on `mcp__unity__apply_text_edits` in T‑F/T‑G/T‑I to exercise `stale_file` semantics.Also applies to: 62-70
225-236: T‑J is “No State Change”; fix header/state text for consistency.The steps insert then remove; final content equals State H. Rename section and clarify expected state.
-### T-J. Idempotency on Modified File (Additive State I) +### T-J. Idempotency on Modified File (No State Change) @@ -- **Expected final state**: State H + verified idempotent behavior +- **Expected final state**: State H (verified idempotent behavior; no content change).github/workflows/claude-nl-suite.yml (2)
212-214: Mount Unity config/local as read‑write; RO can cause EACCES at runtime.Warmup uses RW but the persistent container uses
:ro. Unity may write caches/state during the suite.- -v "$RUNNER_TEMP/unity-config:/root/.config/unity3d:ro" \ - -v "$RUNNER_TEMP/unity-local:/root/.local/share/unity3d:ro" \ + -v "$RUNNER_TEMP/unity-config:/root/.config/unity3d" \ + -v "$RUNNER_TEMP/unity-local:/root/.local/share/unity3d" \
105-116: Fix XML-prolog detection; current grep can mis-detect due to BRE\?.In basic grep,
\?is a quantifier; you want a literal string match.- if head -c 100 "$f" | grep -qi '<\?xml'; then + if head -c 100 "$f" | grep -qiF '<?xml'; then
🧹 Nitpick comments (6)
.claude/prompts/nl-unity-suite-full-additive.md (2)
198-207: Make T‑G’s stale-SHA path unambiguous.Call out that the second attempt must reuse the prior SHA via
precondition_sha256to forcestale_file, then retry with the fresh SHA.-- Second should return `stale_file`, retry with updated SHA +- Second should include the previous `precondition_sha256` and return `stale_file`; then retry with the latest SHA
57-59: Use bullets instead of em-dash paragraph for console scan rules.Minor clarity/readability; avoids agents missing sub-steps.
-— Console scans: perform two reads — last 10 `log/info` lines and up to 3 `error` entries; include ≤ 3 lines total in the fragment; if no errors, state "no errors". -— Final check is folded into T‑J: perform an errors‑only scan and include a single "no errors" line or up to 3 error lines within the T‑J fragment. +• Console scans: perform two reads — last 10 `log/info` lines and up to 3 `error` entries; include ≤ 3 lines total in the fragment; if no errors, state "no errors". +• Final check (T‑J only): errors‑only scan; include "no errors" or up to 3 error lines..github/workflows/claude-nl-suite.yml (4)
747-763: Step title punctuation: keep arrow style consistent across workflows.If other workflows use
→, align here; otherwise this is fine. Pure cosmetic.- - name: NL/T details -> Job Summary + - name: NL/T details → Job Summary
1-18: Pin actions to commit SHAs for supply‑chain hardening.Consider pinning actions (checkout, setup-uv, anthropics, upload-artifact, junit-report) to immutable SHAs.
71-87: Outputs quoting looks good; minor shell style.
has_serialsubshell is fine; optional: compute once and export also as env for later steps if reused.
219-219: Trim trailing spaces to satisfy linters.Minor YAML hygiene; see YAMLlint “trailing spaces” warning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.claude/prompts/nl-unity-suite-full-additive.md(12 hunks).github/workflows/claude-nl-suite.yml(13 hunks)
🧰 Additional context used
🪛 LanguageTool
.claude/prompts/nl-unity-suite-full-additive.md
[grammar] ~17-~17: There might be a mistake here.
Context: ...T. **CRITICAL XML FORMAT REQUIREMENTS:** - Each file must contain EXACTLY one <tes...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ..., T-C, T-D, T-E, T-F, T-G, T-H, T-I, T-J 5) NO RESTORATION - tests build additivel...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ... RESTORATION** - tests build additively on previous state. 6) **STRICT FRAGMENT EM...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...ests build additively on previous state. 6) STRICT FRAGMENT EMISSION - After each ...
(QB_NEW_EN)
[grammar] ~54-~54: There might be a mistake here.
Context: ...0 chars: brief status + latest SHA only. - Console evidence: fetch the last 10 line...
(QB_NEW_EN)
[grammar] ~56-~56: There might be a mistake here.
Context: ...i‑line diffs; reference markers instead. — Console scans: perform two reads — las...
(QB_NEW_EN)
[grammar] ~62-~62: There might be a mistake here.
Context: ... the T‑J fragment. --- ## Tool Mapping - Anchors/regex/structured: `mcp__unity_...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ...Mapping - Anchors/regex/structured: mcp__unity__script_apply_edits - Allowed ops: anchor_insert, `replace_m...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ...ethod, insert_method, delete_method, regex_replace - Foranchor_insert, always set "positi...
(QB_NEW_EN)
[grammar] ~65-~65: There might be a mistake here.
Context: ...set "position": "before" or "after". - Precise ranges / atomic batch: `mcp__u...
(QB_NEW_EN)
[grammar] ~66-~66: There might be a mistake here.
Context: ...ply_text_edits(non‑overlapping ranges) STRICT OP GUARDRAILS - Do not useancho...
(QB_NEW_EN)
[grammar] ~67-~67: There might be a mistake here.
Context: ...overlapping ranges) STRICT OP GUARDRAILS - Do not use anchor_replace. Structured ...
(QB_NEW_EN)
[grammar] ~68-~68: There might be a mistake here.
Context: ...thod, delete_method, regex_replace`. - For multi‑spot textual tweaks in one ope...
(QB_NEW_EN)
[grammar] ~88-~88: There might be a mistake here.
Context: .../T‑I to exercise stale_file semantics. - Use content signatures (method names, co...
(QB_NEW_EN)
[grammar] ~125-~125: There might be a mistake here.
Context: ... + ranges; insert immediately before it. - Insert three comment lines before final ...
(QB_NEW_EN)
[grammar] ~141-~141: There might be a mistake here.
Context: ...: State C (unchanged) - IMMEDIATELY write clean XML fragment to `reports/NL-4_res...
(QB_NEW_EN)
[grammar] ~178-~178: There might be a mistake here.
Context: ...d Evolution Lifecycle (Additive State G) Goal: Insert → modify → finalize a fie...
(QB_NEW_EN)
[grammar] ~179-~179: There might be a mistake here.
Context: ...fy → finalize a field + companion method Actions: - Insert field: `private in...
(QB_NEW_EN)
[grammar] ~180-~180: There might be a mistake here.
Context: ... a field + companion method Actions: - Insert field: private int Counter = 0;...
(QB_NEW_EN)
[grammar] ~181-~181: There might be a mistake here.
Context: ...ion method Actions: - Insert field: private int Counter = 0; - Update it: find and replace with `privat...
(QB_NEW_EN)
[grammar] ~208-~208: There might be a mistake here.
Context: ...ation on Modified File (No State Change) Goal: Ensure validation works correctl...
(QB_NEW_EN)
[grammar] ~210-~210: There might be a mistake here.
Context: ...ly on heavily modified file Actions: - Run validate_script(level:"standard") ...
(QB_NEW_EN)
[grammar] ~213-~213: There might be a mistake here.
Context: ...e**: State H (validation only, no edits) - Emit reports/T-H_results.xml confirmin...
(QB_NEW_EN)
[grammar] ~225-~225: There might be a mistake here.
Context: ...ency on Modified File (Additive State I) Goal: Verify operations behave predict...
(QB_NEW_EN)
[grammar] ~227-~227: There might be a mistake here.
Context: ...e predictably when repeated Actions: - Insert (structured): `mcp__unity__scri...
(QB_NEW_EN)
[grammar] ~231-~231: There might be a mistake here.
Context: ...o_op: true. - **Remove (structured)**: {"op":"regex_replace","pattern":"(?m)^\s*// idempotency test marker\r?\n?","text":""}- **Remove again** (sameregex_replace`) → ...
(QB_NEW_EN)
[grammar] ~235-~235: There might be a mistake here.
Context: ...e "no errors" if none - IMMEDIATELY write clean XML fragment to `reports/T-J_resu...
(QB_NEW_EN)
🪛 YAMLlint (1.37.1)
.github/workflows/claude-nl-suite.yml
[warning] 75-75: wrong indentation: expected 14 but found 12
(indentation)
[warning] 75-75: too many spaces after colon
(colons)
[warning] 76-76: too many spaces after colon
(colons)
[warning] 78-78: too many spaces after colon
(colons)
[warning] 92-92: wrong indentation: expected 14 but found 12
(indentation)
[warning] 122-122: wrong indentation: expected 14 but found 12
(indentation)
[warning] 122-122: too many spaces after colon
(colons)
[warning] 123-123: too many spaces after colon
(colons)
[warning] 125-125: too many spaces after colon
(colons)
[warning] 166-166: wrong indentation: expected 14 but found 12
(indentation)
[warning] 196-196: wrong indentation: expected 14 but found 12
(indentation)
[error] 219-219: trailing spaces
(trailing-spaces)
[warning] 815-815: wrong indentation: expected 14 but found 12
(indentation)
[warning] 815-815: too many spaces after colon
(colons)
[warning] 817-817: too many spaces after colon
(colons)
🔇 Additional comments (3)
.github/workflows/claude-nl-suite.yml (3)
346-373: Safe canonicalization; LGTM.Good guardrails: filename-derived IDs, single-
<testcase>enforcement, no XML declaration. Matches prompt constraints.
424-473: Backfill placeholders step is solid.Covers all NL/T IDs and writes minimal valid fragments with a
<failure>. Good resilience.
489-588: JUnit assembly logic is robust.Handles malformed fragments, enforces single testcase, recalculates counts; name repair matches canonicalization rules.
…ports/) to support multi-edit reporting
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
.github/workflows/claude-nl-suite.yml (6)
3-3: Prefer explicit trigger mapping for consistency.-on: [workflow_dispatch] +on: + workflow_dispatch:
105-108: Good fix: properly escaped XML prolog in grep.
850-858: License return moved to finalizer — LGTM.
341-341: Document the Bash disallow change inline.Add a YAML comment to explain policy/context for CI reviewers.
- disallowed_tools: TodoWrite,Task,Bash + # NOTE: Bash is explicitly disallowed to enforce in-editor edits/validation. + disallowed_tools: TodoWrite,Task,Bash
787-787: Unify arrow glyph in step names.Revert to the previously used “→” or standardize across workflows.
- - name: NL/T details -> Job Summary + - name: NL/T details → Job Summary
212-214: Mount Unity config/local as RW to prevent EACCES during runtime.Read-only mounts can break entitlement renewals and cache writes. Use RW.
- -v "$RUNNER_TEMP/unity-config:/root/.config/unity3d:ro" \ - -v "$RUNNER_TEMP/unity-local:/root/.local/share/unity3d:ro" \ + -v "$RUNNER_TEMP/unity-config:/root/.config/unity3d" \ + -v "$RUNNER_TEMP/unity-local:/root/.local/share/unity3d" \
🧹 Nitpick comments (2)
.github/workflows/claude-nl-suite.yml (2)
75-79: Fix YAML lint nits (indent, colons, trailing spaces).Normalize single space after colons, align env indentation, and remove trailing spaces.
- UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }} - UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} - UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} + UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }} + UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} + UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} ... - UNITY_IMAGE: ${{ env.UNITY_IMAGE }} - UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} - UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} - UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} + UNITY_IMAGE: ${{ env.UNITY_IMAGE }} + UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} + UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} + UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} ... - -executeMethod MCPForUnity.Editor.MCPForUnityBridge.StartAutoConnect + -executeMethod MCPForUnity.Editor.MCPForUnityBridge.StartAutoConnect ... - UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} - UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} - UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} + UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} + UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} + UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }}Tip: run yamllint to catch remaining indentation width differences.
Also applies to: 92-92, 122-126, 166-168, 196-198, 219-219, 855-857
205-213: Optional: expose bridge status dir to host for easier diagnostics.Mount
/root/.unity-mcpto$RUNNER_TEMP/.unity-mcpand read it directly withoutdocker exec. Helps artifacting/debugging.- docker run -d --name unity-mcp --network host \ + docker run -d --name unity-mcp --network host \ -e HOME=/root \ -e UNITY_MCP_ALLOW_BATCH=1 \ -e UNITY_MCP_STATUS_DIR=/root/.unity-mcp \ -e UNITY_MCP_BIND_HOST=127.0.0.1 \ - -v "${{ github.workspace }}:/workspace" -w /workspace \ + -v "${{ github.workspace }}:/workspace" -w /workspace \ + -v "$RUNNER_TEMP/.unity-mcp:/root/.unity-mcp" \ -v "$RUNNER_TEMP/unity-config:/root/.config/unity3d" \ -v "$RUNNER_TEMP/unity-local:/root/.local/share/unity3d" \You can then read
$RUNNER_TEMP/.unity-mcpfrom the host and include it in artifacts.Also applies to: 221-271
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/claude-nl-suite.yml(13 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/claude-nl-suite.yml
[warning] 75-75: wrong indentation: expected 14 but found 12
(indentation)
[warning] 75-75: too many spaces after colon
(colons)
[warning] 76-76: too many spaces after colon
(colons)
[warning] 78-78: too many spaces after colon
(colons)
[warning] 92-92: wrong indentation: expected 14 but found 12
(indentation)
[warning] 122-122: wrong indentation: expected 14 but found 12
(indentation)
[warning] 122-122: too many spaces after colon
(colons)
[warning] 123-123: too many spaces after colon
(colons)
[warning] 125-125: too many spaces after colon
(colons)
[warning] 166-166: wrong indentation: expected 14 but found 12
(indentation)
[warning] 196-196: wrong indentation: expected 14 but found 12
(indentation)
[error] 219-219: trailing spaces
(trailing-spaces)
[warning] 855-855: wrong indentation: expected 14 but found 12
(indentation)
[warning] 855-855: too many spaces after colon
(colons)
[warning] 857-857: too many spaces after colon
(colons)
🔇 Additional comments (1)
.github/workflows/claude-nl-suite.yml (1)
162-183: Warm-up flow and ULF handoff look solid.
| "$UNITY_IMAGE" bash -lc ' | ||
| set -euxo pipefail | ||
| if [[ -n "${UNITY_SERIAL:-}" ]]; then | ||
| /opt/unity/Editor/Unity -batchmode -nographics -logFile - \ | ||
| -username "$UNITY_EMAIL" -password "$UNITY_PASSWORD" -serial "$UNITY_SERIAL" -quit || true | ||
| else | ||
| /opt/unity/Editor/Unity -batchmode -nographics -logFile - \ | ||
| -username "$UNITY_EMAIL" -password "$UNITY_PASSWORD" -quit || true | ||
| fi | ||
| ls -la /root/.config/unity3d/Unity/licenses || true | ||
| ' |
There was a problem hiding this comment.
Remove command echoing to avoid leaking UNITY credentials.
Inside the container script you run set -euxo pipefail. The -x will echo the Unity -username/-password/-serial command with secrets expanded into logs. Drop -x (or temporarily disable it) around the activation calls.
- "$UNITY_IMAGE" bash -lc '
- set -euxo pipefail
+ "$UNITY_IMAGE" bash -lc '
+ set -euo pipefail
if [[ -n "${UNITY_SERIAL:-}" ]]; then
- /opt/unity/Editor/Unity -batchmode -nographics -logFile - \
- -username "$UNITY_EMAIL" -password "$UNITY_PASSWORD" -serial "$UNITY_SERIAL" -quit || true
+ set +x
+ /opt/unity/Editor/Unity -batchmode -nographics -logFile - \
+ -username "$UNITY_EMAIL" -password "$UNITY_PASSWORD" -serial "$UNITY_SERIAL" -quit || true
+ set -x || true
else
- /opt/unity/Editor/Unity -batchmode -nographics -logFile - \
- -username "$UNITY_EMAIL" -password "$UNITY_PASSWORD" -quit || true
+ set +x
+ /opt/unity/Editor/Unity -batchmode -nographics -logFile - \
+ -username "$UNITY_EMAIL" -password "$UNITY_PASSWORD" -quit || true
+ set -x || true
fi
ls -la /root/.config/unity3d/Unity/licenses || true
'Also applies to: 140-145
🤖 Prompt for AI Agents
.github/workflows/claude-nl-suite.yml around lines 137-147 (and also lines
140-145): the script uses set -euxo pipefail which causes shell command tracing
(-x) to print Unity activation commands with
UNITY_EMAIL/UNITY_PASSWORD/UNITY_SERIAL into logs; remove -x from the global set
or temporarily disable tracing immediately before the sensitive Unity calls
(e.g., use set +x, run the Unity command(s), then restore tracing with set -x if
needed) so credentials are not echoed, and ensure the rest of the script still
runs with -euo pipefail semantics.
|
Closing, resubmitting improved version. |
Summary by CodeRabbit
New Features
Documentation
Chores