feat(viewer): 4 sat→7 web-viewer UX/correctness fixes (battle-log sanitize, dice-header strip, roll context, drop-confirm)#1161
Conversation
…itize, dice-header strip, roll context, drop-confirm)
From the binding-sweep per-persona friction analysis:
1. BATTLE-LOG SANITIZER COVERAGE (adversarial friction): the combat Battle Log
rendered l.title/l.text RAW, so DM engine meta-text (wrapper-progress lines,
roll-summary headers, scaffolding) leaked there — the Chronicle's guard never
reached this panel. BattleLogLine now routes both fields through the SAME
window-exported sanitizeNarration (screen-combat.jsx).
2. DICE-RESULT-HEADER STRIP (narrative's only major — 'cracked the 9/10'): extend
the sanitizer to drop (a) the roll-result-summary header form ('… lands at 18;
the quiet interpose at 16.') and (b) a leading/standalone Markdown horizontal
rule ('---'/'***'/'___'/'— —'). Verb+bare-integer anchored and whole-line for the
chained header so genuine prose ('the bell strikes at 12', 'the arrow lands at his
feet', em-dash asides) is never eaten (screen-table.jsx).
3. DIE-ROLL BUTTON CONTEXT (adversarial minor): the d20/d12/d8/d6 buttons fired a
contextless 'requests a dN roll', bypassing the Declare guard. They now bind the
roll to the player's typed intent, else attach the latest narrative line — parity
with the Declare text-guard (composeRollMove, screen-table.jsx).
4. DROP-CONFIRM ON EQUIPPED GEAR (adversarial minor): a one-click Drop on equipped
body armor was irreversible. Both Drop affordances (context-menu + detail pane)
now confirm via window.confirm ONLY for currently-equipped items; loose stash
items are never nagged (isItemEquipped, screen-inventory.jsx).
Tests: extend test_sanitize_narration.py (dice-header + leading-rule strip + a
hardened negative-prose suite), add test_battle_log_sanitize.py (BattleLogLine
meta-text strip + real-combat-text survival) and test_viewer_ux_guards.py
(composeRollMove context + isItemEquipped/drop-confirm). Local viewer suite green
(only pre-existing test_portrait_gen pydantic-env failure is unrelated).
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughExtends ChangesNarration Sanitization, Roll-Move Composition, and Equipped-Drop Guard
Sequence Diagram(s)sequenceDiagram
participant Player
participant requestRoll
participant composeRollMove
participant postMove
Player->>requestRoll: clicks dN button
alt intent typed
requestRoll->>composeRollMove: sides, draftText, latestStreamedLine
else intent blank
requestRoll->>composeRollMove: sides, empty string, latestStreamedLine
end
composeRollMove-->>requestRoll: move payload and echo text
requestRoll->>postMove: POST move
requestRoll-->>Player: display echo, clear if intent was typed
sequenceDiagram
participant Player
participant DropAction
participant confirmDrop
participant isItemEquipped
Player->>DropAction: clicks Drop
DropAction->>confirmDrop: item
confirmDrop->>isItemEquipped: item, equipped list
isItemEquipped-->>confirmDrop: boolean equipped flag
alt item is equipped
confirmDrop->>Player: window.confirm message
Player-->>confirmDrop: OK or Cancel
end
alt confirmed or loose item
confirmDrop->>DropAction: POST drop move
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Agent Review Notes — Confidence Scores and Adversarial Findings[95% confidence] Root cause: Code path: Impact: DM/engine meta-text (roll-summary headers, scaffolding markers) leaks to players in the exact scenario this PR aims to prevent, with zero observable error or console warning. Fix: Validate that [90% confidence] Root cause: Code path: Impact: A player attempting to drop a "+1 Sword" that is not currently equipped could be shown a spurious "drop this equipped item?" confirmation, confusing UX and potentially blocking intended drops. Fix: Ensure the slug function preserves modifier-distinguishing characters ("+", "−", alphanumeric variants) or compare against a canonical item ID/reference rather than a derived name slug. Add a test case with modifiers to verify non-match. [85% confidence] Root cause: Code path: Impact: Legitimate em-dash narrative separators or stylistic punctuation in genuine fiction could silently disappear from the player chronicle, degrading story fidelity. Fix: Confirm the [80% confidence] Root cause: Code path: Impact: DM/engine internal roll-result summaries, scaffolding stage-direction, or wrapper meta-text could be injected into player-visible roll move payloads and echoed back, leaking DM-internal bookkeeping into the move record. Fix: Run [75% confidence] Root cause: Code path: Impact: A race condition or state inconsistency where Fix: Add defensive checks in 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
enhancement
…ld quantities)
The dice-result-header stripper (_ROLL_RESULT_SUMMARY / _ROLL_SUMMARY_HEADER_LINE)
was verb- AND number-anchored but matched any '<roll verb> at <N>', eating the whole
narration line even when <N> was a real in-world quantity, not a check total
('the line falls at 12 men', 'settles at 5 gold', 'comes in at 40 pounds').
A roll TOTAL always ends its clause ('lands at 18.' / 'lands at 18;'), never
'lands at 18 men'. Require the integer to be CLAUSE-TERMINAL via a shared
_ROLL_VERB_AT_N core: '...at \d+(?=[.;,!?]|\s*$)' on the primary AND every
chained '; ... at N' continuation. Genuine fiction (number-then-noun) now survives
verbatim; the real header (single / chained / triple-chain / trailing-rule) is
still stripped, as is the leading/trailing rule.
Adds test_roll_verb_with_in_world_quantity_survives_verbatim (6 fixtures) and keeps
the existing positive strip + FP-guard tests green (18 tests / 62 subtests pass).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@viewer/openworlds/screen-inventory.jsx`:
- Around line 436-441: The isItemEquipped function relies solely on slugged name
matching to determine if an item is equipped, which causes false positives when
a hero has both an equipped copy and a loose copy with identical names. Update
the function to use stable identity matching first: check if both the item and
equipped entries have an id property and compare those for equality; only fall
back to the current slug-based name comparison when id is not available on
either side. Additionally, add a regression test that covers the scenario where
a hero has the same named item in both equipped and loose inventory to prevent
this behavior from breaking in the future.
In `@viewer/openworlds/screen-table.jsx`:
- Around line 337-343: The echo variable does not include the fallback context
handling that the text variable provides. The text variable uses a nested
ternary to fall back to ctx when want is not provided, but the echo variable
only checks for want and does not include this ctx fallback. Update the echo
constant to mirror the same conditional structure as text, ensuring that when
want is falsy but ctx is truthy, the echo includes the context information in
the format rolls a d${n} (in response to: ${ctx}).
- Line 74: The _HRULE_LINE regex pattern at line 74 should be used consistently
throughout the code to identify horizontal rules. Review the logic at lines 149,
162, 256, and 294 where roll-total boundaries and rule detection occur. Tighten
the "lands at <number>" detection to only match when the pattern represents a
complete roll result, not when it appears within regular prose sentences.
Replace the partial or separate horizontal rule detection logic at lines 162 and
294 with calls to the full _HRULE_LINE pattern to ensure consistent matching of
horizontal rules at the whole-line level, preventing legitimate sentence content
like "The dragon lands at 12 feet from you" from being incorrectly removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 764fddb4-dc2f-4eae-ad2c-0a02fcb68353
📒 Files selected for processing (6)
viewer/openworlds/screen-combat.jsxviewer/openworlds/screen-inventory.jsxviewer/openworlds/screen-table.jsxviewer/tests/test_battle_log_sanitize.pyviewer/tests/test_sanitize_narration.pyviewer/tests/test_viewer_ux_guards.py
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.44.0)
viewer/tests/test_viewer_ux_guards.py
[error] 57-59: Command coming from incoming request
Context: subprocess.run(
[_node(), "--input-type=commonjs"], input=program, text=True, capture_output=True
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[info] 42-42: use jsonify instead of json.dumps for JSON output
Context: json.dumps(str(f))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 46-46: use jsonify instead of json.dumps for JSON output
Context: json.dumps(str(BABEL))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 72-72: use jsonify instead of json.dumps for JSON output
Context: json.dumps(sides)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 72-72: use jsonify instead of json.dumps for JSON output
Context: json.dumps(intent)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 72-72: use jsonify instead of json.dumps for JSON output
Context: json.dumps(context)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 123-123: use jsonify instead of json.dumps for JSON output
Context: json.dumps(item)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 123-123: use jsonify instead of json.dumps for JSON output
Context: json.dumps(equipped)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
viewer/tests/test_battle_log_sanitize.py
[error] 70-72: Command coming from incoming request
Context: subprocess.run(
[_node(), "--input-type=commonjs"], input=program, text=True, capture_output=True
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[info] 44-44: use jsonify instead of json.dumps for JSON output
Context: json.dumps(str(BABEL))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 58-58: use jsonify instead of json.dumps for JSON output
Context: json.dumps(str(SCREEN_TABLE))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 59-59: use jsonify instead of json.dumps for JSON output
Context: json.dumps(str(SCREEN_COMBAT))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 66-66: use jsonify instead of json.dumps for JSON output
Context: json.dumps(rows)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
viewer/openworlds/screen-table.jsx
[warning] 157-163: Detects non-literal values in regular expressions
Context: new RegExp(
"^\s*[^.!?]?" +
"\b(?:lands?|comes?(?:\s+in)?|falls?|settles?|resolves?|clears?)\s+(?:in\s+)?at\s+\d+\b" +
"(?:[^.!?]?\bat\s+\d+\b)" +
"\s[.!?]?\s*(?:-{3,}|\{3,}|_{3,}|—{2,})?\s$",
"i",
)
Note: [CWE-1333] Inefficient Regular Expression Complexity (ReDoS via non-literal RegExp).
(detect-non-literal-regexp)
🪛 React Doctor (0.5.8)
viewer/openworlds/screen-combat.jsx
[warning] 764-764: sanitize inside BattleLogLine uses no local state but is rebuilt on every render, so it wastes work & breaks memoized children. Move it to the top of the file, outside the component.
Move the function above the component, at the top of the file. It doesn't use local state, so rebuilding it each update is wasted work.
(prefer-module-scope-pure-function)
[warning] 772-772: Your users strain to read 10px text, so use at least 12px for body text, & 16px is best.
Use at least 12px for body text, and 16px is best. Small text is hard to read, especially on phones.
(no-tiny-text)
🪛 Ruff (0.15.18)
viewer/tests/test_viewer_ux_guards.py
[warning] 43-43: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[warning] 47-47: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[error] 58-58: subprocess call: check for execution of untrusted input
(S603)
[warning] 58-58: subprocess.run without explicit check argument
Add explicit check=False
(PLW1510)
[warning] 62-62: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 71-72: Explicitly concatenated string should be implicitly concatenated
Remove redundant '+' operator to implicitly concatenate
(ISC003)
[warning] 73-73: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[warning] 122-123: Explicitly concatenated string should be implicitly concatenated
Remove redundant '+' operator to implicitly concatenate
(ISC003)
[warning] 124-124: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[warning] 158-159: Explicitly concatenated string should be implicitly concatenated
Remove redundant '+' operator to implicitly concatenate
(ISC003)
viewer/tests/test_sanitize_narration.py
[warning] 311-311: Loop control variable original not used within loop body
Rename unused original to _original
(B007)
[warning] 311-311: When using only the keys of a dict use the keys() method
Replace .items() with .keys()
(PERF102)
viewer/tests/test_battle_log_sanitize.py
[warning] 45-45: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[warning] 59-59: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[warning] 60-60: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[error] 71-71: subprocess call: check for execution of untrusted input
(S603)
[warning] 71-71: subprocess.run without explicit check argument
Add explicit check=False
(PLW1510)
[warning] 75-75: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 126-126: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
[warning] 127-127: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
🔇 Additional comments (3)
viewer/openworlds/screen-combat.jsx (1)
757-775: LGTM!viewer/tests/test_sanitize_narration.py (1)
297-368: LGTM!viewer/tests/test_battle_log_sanitize.py (1)
1-135: LGTM!
| function isItemEquipped(item, equipped) { | ||
| if (!item || !item.name || !Array.isArray(equipped)) return false; | ||
| const target = slug(item.name); | ||
| if (!target) return false; | ||
| return equipped.some((e) => e && e.name && slug(e.name) === target); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Name-based equip detection will false-positive on duplicate item names
Lines 436-440 use slugged-name equality as the equip identity. If a hero has one equipped copy and one loose copy with the same name, dropping the loose copy still triggers the equipped confirm path. That breaks the “confirm only on currently equipped items” contract in both Drop flows.
Use stable item identity (id/instance key) first, and only fall back to name matching when no identity exists on either side. Also add a regression test for “equipped + loose same-name item” to lock this behavior.
Confidence: 95%.
Proposed fix (ID-first matching)
function isItemEquipped(item, equipped) {
- if (!item || !item.name || !Array.isArray(equipped)) return false;
- const target = slug(item.name);
- if (!target) return false;
- return equipped.some((e) => e && e.name && slug(e.name) === target);
+ if (!item || !Array.isArray(equipped)) return false;
+
+ const itemId = item.id ?? item.item_id ?? null;
+ if (itemId !== null) {
+ return equipped.some((e) => e && (e.id === itemId || e.item_id === itemId));
+ }
+
+ if (!item.name) return false;
+ const target = slug(item.name);
+ if (!target) return false;
+ return equipped.some((e) => {
+ if (!e || !e.name) return false;
+ const equippedId = e.id ?? e.item_id ?? null;
+ return equippedId === null && slug(e.name) === target;
+ });
}🤖 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 `@viewer/openworlds/screen-inventory.jsx` around lines 436 - 441, The
isItemEquipped function relies solely on slugged name matching to determine if
an item is equipped, which causes false positives when a hero has both an
equipped copy and a loose copy with identical names. Update the function to use
stable identity matching first: check if both the item and equipped entries have
an id property and compare those for equality; only fall back to the current
slug-based name comparison when id is not available on either side.
Additionally, add a regression test that covers the scenario where a hero has
the same named item in both equipped and loose inventory to prevent this
behavior from breaking in the future.
| const text = want | ||
| ? `I roll a d${n} to ${want}` | ||
| : ctx | ||
| ? `I roll a d${n} (in response to: ${ctx})` | ||
| : `I roll a d${n}`; | ||
| const echo = want ? `rolls a d${n} to ${want}` : `rolls a d${n}`; | ||
| return { move: { kind: "check", name: `d${n}`, text }, echo }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include fallback context in the optimistic echo.
Confidence: 88%. The fallback path puts latestStreamedLine into move.text, but Line 342 still returns the bare echo rolls a d${n}. That means the Chronicle/pending/retry label remains contextless even though Line 1290 says the echo should never read as a bare die.
Proposed fix
- const echo = want ? `rolls a d${n} to ${want}` : `rolls a d${n}`;
+ const echo = want
+ ? `rolls a d${n} to ${want}`
+ : ctx
+ ? `rolls a d${n} in response to: ${ctx}`
+ : `rolls a d${n}`;Also applies to: 1289-1292
🤖 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 `@viewer/openworlds/screen-table.jsx` around lines 337 - 343, The echo variable
does not include the fallback context handling that the text variable provides.
The text variable uses a nested ternary to fall back to ctx when want is not
provided, but the echo variable only checks for want and does not include this
ctx fallback. Update the echo constant to mirror the same conditional structure
as text, ensuring that when want is falsy but ctx is truthy, the echo includes
the context information in the format rolls a d${n} (in response to: ${ctx}).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@viewer/openworlds/screen-table.jsx`:
- Around line 156-171: The regex patterns _ROLL_VERB_AT_N and
_ROLL_SUMMARY_HEADER_LINE contain a comma in their terminal lookahead assertion
(?=[.;,!?]|\\s*$), which causes the unanchored _ROLL_RESULT_SUMMARY test to
incorrectly match genuine prose sentences ending with "at <number>," and
classify them as scaffolding for removal. Remove the comma from the lookahead in
both patterns so the assertion becomes (?=[.;!?]|\\s*$), which will prevent
matching sentences with number-comma terminators while still matching valid
roll-header terminators.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b2dbc3d0-adb5-4e81-98fe-f3636d166bf4
📒 Files selected for processing (2)
viewer/openworlds/screen-table.jsxviewer/tests/test_sanitize_narration.py
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.44.0)
viewer/openworlds/screen-table.jsx
[warning] 157-157: Detects non-literal values in regular expressions
Context: new RegExp(_ROLL_VERB_AT_N, "i")
Note: [CWE-1333] Inefficient Regular Expression Complexity (ReDoS via non-literal RegExp).
(detect-non-literal-regexp)
🔇 Additional comments (2)
viewer/openworlds/screen-table.jsx (1)
1293-1303: LGTM!viewer/tests/test_sanitize_narration.py (1)
370-389: LGTM!
| const _ROLL_VERB_AT_N = | ||
| "\\b(?:lands?|comes?(?:\\s+in)?|falls?|settles?|resolves?|clears?)\\s+(?:in\\s+)?at\\s+\\d+(?=[.;,!?]|\\s*$)"; | ||
| const _ROLL_RESULT_SUMMARY = new RegExp(_ROLL_VERB_AT_N, "i"); | ||
| // The FULL roll-summary HEADER LINE — the verb-anchored primary PLUS any "; <thing> at <N>" continuations | ||
| // chained onto it ("The intimidation lands at 18; the quiet interpose at 16."). This is matched at the LINE | ||
| // level (before the per-sentence splitter fragments it on the ";"), so the whole header — including its | ||
| // trailing clauses and an optional trailing rule — is dropped as a unit. It REQUIRES the leading roll verb, | ||
| // so it can only extend a genuine roll-summary; a standalone "; we meet at 3" never has the primary verb and | ||
| // is never reached. Anchored to the whole line ('^…$') so it only fires on a header, not a mid-prose clause. | ||
| // Each "at <N>" (primary AND every chained continuation) is clause-terminal — the same total-ends-the-clause | ||
| // rule as `_ROLL_VERB_AT_N` — so a header whose number is followed by a noun ("falls at 12 men …") never | ||
| // matches and the genuine-fiction line survives whole. | ||
| const _ROLL_SUMMARY_HEADER_LINE = new RegExp( | ||
| "^\\s*[^.!?]*?" + | ||
| _ROLL_VERB_AT_N + | ||
| "(?:[^.!?]*?\\bat\\s+\\d+(?=[.;,!?]|\\s*$))*" + |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for roll-summary header formatting in engine/prompt sources to confirm separator style.
rg -nP -C2 '(lands?|comes?|falls?|settles?|resolves?|clears?)\s+(in\s+)?at\s+\d+' -g '!viewer/tests/**'Repository: electricsheephq/WorldOS
Length of output: 161
🏁 Script executed:
# Verify the file and code at the specified lines
cat -n viewer/openworlds/screen-table.jsx | sed -n '156,171p'Repository: electricsheephq/WorldOS
Length of output: 1535
🏁 Script executed:
# Find test files related to roll scaffolding and check for comma-terminal tests
fd -e 'jsx' -e 'js' -e 'test' -e 'spec' . viewer/tests/ | head -20Repository: electricsheephq/WorldOS
Length of output: 161
🏁 Script executed:
# Search for uses of _isScaffoldingSentence and _hasScaffolding to understand how the regex is used
rg -n '_isScaffoldingSentence|_hasScaffolding' viewer/openworlds/Repository: electricsheephq/WorldOS
Length of output: 643
🏁 Script executed:
# Look for test cases that explicitly test the roll verb patterns
rg -n '_ROLL_VERB_AT_N|_ROLL_RESULT_SUMMARY|_ROLL_SUMMARY_HEADER_LINE' -A5 -B5 viewer/tests/Repository: electricsheephq/WorldOS
Length of output: 161
🏁 Script executed:
# View the _isScaffoldingSentence function to understand how _ROLL_RESULT_SUMMARY is used
sed -n '262,280p' viewer/openworlds/screen-table.jsxRepository: electricsheephq/WorldOS
Length of output: 1285
🏁 Script executed:
# View the _hasScaffolding function
sed -n '268,275p' viewer/openworlds/screen-table.jsxRepository: electricsheephq/WorldOS
Length of output: 675
🏁 Script executed:
# Find test files - may be in different locations
find viewer -type f \( -name '*.test.js' -o -name '*.test.jsx' -o -name '*.spec.js' -o -name '*test*.js' \)Repository: electricsheephq/WorldOS
Length of output: 161
🏁 Script executed:
# Search more broadly for test-related patterns including "test_roll_verb" mentioned in review
rg -n 'test_roll_verb|roll.*survives|scaffolding' viewer/ --type js --type jsxRepository: electricsheephq/WorldOS
Length of output: 189
🏁 Script executed:
# Search for test files more broadly without -t flag
find viewer -type f -name '*test*' -o -name '*spec*' 2>/dev/null | head -30Repository: electricsheephq/WorldOS
Length of output: 1330
🏁 Script executed:
# Search for the specific test mentioned in review: test_roll_verb_with_in_world_quantity_survives_verbatim
rg -n 'test_roll_verb_with_in_world_quantity_survives_verbatim|in_world_quantity' viewer/Repository: electricsheephq/WorldOS
Length of output: 276
🏁 Script executed:
# Look at the full _stripScaffoldingSentences function to understand the code path
sed -n '276,310p' viewer/openworlds/screen-table.jsxRepository: electricsheephq/WorldOS
Length of output: 2286
🏁 Script executed:
# View the test that reviewer mentioned
sed -n '370,395p' viewer/tests/test_sanitize_narration.pyRepository: electricsheephq/WorldOS
Length of output: 2111
🏁 Script executed:
# Look for other roll-verb related tests to see what terminals they use
rg -n 'falls.*at|lands.*at|settles.*at|resolves.*at' viewer/tests/test_sanitize_narration.py -A2 -B2Repository: electricsheephq/WorldOS
Length of output: 5017
Remove comma from roll-verb terminal lookahead in both _ROLL_VERB_AT_N and _ROLL_SUMMARY_HEADER_LINE.
Root cause: The unanchored _ROLL_RESULT_SUMMARY.test() called by _isScaffoldingSentence matches sentences with pattern <roll verb> at <N>, due to the comma in the lookahead (?=[.;,!?]|\\s*$). This misclassifies genuine prose as scaffolding and drops it.
Code path: Line 293 filters sentence fragments with _isScaffoldingSentence(p) → line 267 tests _ROLL_RESULT_SUMMARY.test(s) → line 158 pattern includes comma terminal.
Impact: Sentences like "The hawk settles at 3, then takes flight." or "The price falls at 5, and the crowd cheers." are erased from player-facing narration. Test test_roll_verb_with_in_world_quantity_survives_verbatim guards against similar regressions but only tests patterns with noun+comma ("at 12 men,"), not number+comma ("at 12,"), leaving the comma-terminal path uncovered.
The fix (remove , from both lookahead classes) is safe: all roll-header tests use ; or . terminators; no test depends on comma terminal. Confidence: 92%.
Proposed fix
const _ROLL_VERB_AT_N =
- "\\b(?:lands?|comes?(?:\\s+in)?|falls?|settles?|resolves?|clears?)\\s+(?:in\\s+)?at\\s+\\d+(?=[.;,!?]|\\s*$)";
+ "\\b(?:lands?|comes?(?:\\s+in)?|falls?|settles?|resolves?|clears?)\\s+(?:in\\s+)?at\\s+\\d+(?=[.;!?]|\\s*$)";
@@
const _ROLL_SUMMARY_HEADER_LINE = new RegExp(
"^\\s*[^.!?]*?" +
_ROLL_VERB_AT_N +
- "(?:[^.!?]*?\\bat\\s+\\d+(?=[.;,!?]|\\s*$))*" +
+ "(?:[^.!?]*?\\bat\\s+\\d+(?=[.;!?]|\\s*$))*" +🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 157-157: Detects non-literal values in regular expressions
Context: new RegExp(_ROLL_VERB_AT_N, "i")
Note: [CWE-1333] Inefficient Regular Expression Complexity (ReDoS via non-literal RegExp).
(detect-non-literal-regexp)
🤖 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 `@viewer/openworlds/screen-table.jsx` around lines 156 - 171, The regex
patterns _ROLL_VERB_AT_N and _ROLL_SUMMARY_HEADER_LINE contain a comma in their
terminal lookahead assertion (?=[.;,!?]|\\s*$), which causes the unanchored
_ROLL_RESULT_SUMMARY test to incorrectly match genuine prose sentences ending
with "at <number>," and classify them as scaffolding for removal. Remove the
comma from the lookahead in both patterns so the assertion becomes
(?=[.;!?]|\\s*$), which will prevent matching sentences with number-comma
terminators while still matching valid roll-header terminators.
Source: Linters/SAST tools
![evaos-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4184532?s=60&v=4){kind=small}
| // quantity — "the line falls at 12 men", "settles at 5 gold", "comes in at 40 pounds", "clears at 6 bells" | ||
| // — genuine fiction that MUST survive verbatim. `_ROLL_VERB_AT_N` is the shared, clause-terminal core. | ||
| const _ROLL_VERB_AT_N = | ||
| "\\b(?:lands?|comes?(?:\\s+in)?|falls?|settles?|resolves?|clears?)\\s+(?:in\\s+)?at\\s+\\d+(?=[.;,!?]|\\s*$)"; |
There was a problem hiding this comment.
P2: Roll-summary regex has a narrow theoretical false-positive on terminal " at ." prose
_ROLL_VERB_AT_N (reused by _ROLL_RESULT_SUMMARY, _ROLL_SUMMARY_HEADER_LINE, _isScaffoldingSentence, and _hasScaffolding) will drop ANY sentence of the form " at " when N is clause-terminal (followed by [.;,!?] or EOL), regardless of subject. The negative tests cover non-roll-verb ("bell strikes at 12") and noun-after-N ("falls at 12 men") cases, but a sentence like "The dust settles at 9." or "His gaze settles at 9." — terminal integer, roll verb — would be silently stripped even though it is genuine fiction. The comments acknowledge the trade-off and it is low-probability in this genre, but consider tightening the primary arm to also require a roll/check-noun subject (e.g. '(?:check|roll|intimidation|persuasion|stealth|strike|parry|attack) ... at ') rather than any " at ", which is the actual fingerprint per the docstring examples. This would eliminate the residual over-match class without weakening the verbatim header strip.
Why this matters: Silent deletion of real narration is a story-quality regression that is hard to spot in QA; tightening the subject anchor removes the only remaining false-positive vector the verb+number guard still admits.
| <BrassButton tone="ghost" size="sm" title="Relays to the DM via /move — the engine resolves it" onClick={() => { | ||
| // SAT→7: confirm before dropping CURRENTLY-EQUIPPED gear (worn body armor especially) — loose | ||
| // stash items drop with no nag. Mirrors the context-menu Drop via the same shared helpers. | ||
| if (isItemEquipped(item, hero && hero.equipped) && !window.confirm(dropEquippedConfirmMessage(item.name))) return; |
There was a problem hiding this comment.
P3: Drop-confirm guard is duplicated inline in ItemDetail instead of reusing confirmDrop
The context-menu Drop path (line 377) routes through the shared confirmDrop(item, doDrop) wrapper, but the ItemDetail detail-pane Drop path (line 890-895) re-implements the same isItemEquipped(...) && !window.confirm(dropEquippedConfirmMessage(...)) check inline and calls postInvMove directly. Both are currently correct and both delegate to the same tested helpers (isItemEquipped/dropEquippedConfirmMessage), so there is no behavioral bug today. However the two paths can drift if the guard logic evolves (e.g. adding a "also confirm for cursed items" rule would require touching both sites). Consider routing ItemDetail's Drop through confirmDrop as well (passing postInvMove as doDrop), or extract a single guardedDrop(item, doDrop) used by both, so the nag stays identical by construction rather than by coincidence.
Why this matters: The PR's stated invariant is 'shared by both Drop affordances so they nag identically' — that invariant currently holds by hand-synchronization, not by code structure, and is the kind of divergence that a future edit will silently break on one path only.
|
triage 2026-07-02: RESCUE candidate — needs rebase onto current main; see /tmp/triage/prs_human.md |
evaOS review status: completedPR: #1161 - feat(viewer): 4 sat→7 web-viewer UX/correctness fixes (battle-log sanitize, dice-header strip, roll context, drop-confirm) evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1161 Review URL: #1161 (review) |
There was a problem hiding this comment.
Walkthrough
PR: #1161 - feat(viewer): 4 sat→7 web-viewer UX/correctness fixes (battle-log sanitize, dice-header strip, roll context, drop-confirm)
Head: 0cb56091f926fe951aad6a78ddc779a0c2472d3a into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 1/5 (~10 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|
Review Signal
No validated inline findings.
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
No finding categories.
Validation and Proof
2 required validation/proof recommendation(s) selected from changed files.
- required: Unity editor or Play Mode smoke - WorldOS repo profile implies Unity runtime risk. Proof: Unity editor smoke; Play Mode log; scene/prefab screenshot or recording.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Prefer correctness, persistence, CI, release, and regression findings over style-only feedback.
Profile proof expectations: Look for Unity editor, play-mode, fixture, or focused smoke evidence when runtime behavior changes.
Related Context
Related issues/PRs: none detected from PR metadata.
Suggested labels: none.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
Assets/**- Prioritize scene, prefab, save-state, asset-reference, and gameplay regressions. - Path instructions:
ProjectSettings/**- Treat build, platform, input, graphics, and release behavior changes as high risk. - Label suggestions: unity, gameplay, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
1 participant
Summary
Four web-viewer sat→7 fixes from the binding-sweep per-persona friction analysis. All ADDITIVE — they extend/reuse the existing chronicle sanitizer and established patterns (
window.confirm,window.sanitizeNarration), nothing rewritten. The engine stays the sole writer; these are read/projection + UX guards only.1. Battle-Log sanitizer coverage (adversarial friction)
BattleLogLine(screen-combat.jsx) renderedl.title/l.textRAW, so DM engine meta-text — a wrapper-progress line ("Momentum carries through the scene…", "Your choice takes hold…"), a roll-summary header, or other scaffolding — leaked into the combat log because the Chronicle's guard never reached this panel. It now routes both fields through the same window-exportedsanitizeNarrationthe Chronicle uses, with the established defensivewindow.sanitizeNarrationfallback. Real combat prose + the engine title + dice-meta badges survive intact.2. Dice-result-header strip (narrative's only major — "cracked the 9/10")
Extended the scaffolding stripper to drop:
;-splitter can't peel a clause through;High-confidence by design: the trigger requires a roll-summary VERB ("lands/settles/comes in/resolves at") bound to a bare integer. Genuine prose — "the bell tower strikes at 12", "we meet at 3", "the arrow lands at his feet", "the cat lands at her side", em-dash asides — passes through verbatim. A dedicated negative-prose suite locks this in.
3. Die-roll button context (adversarial minor)
The d20/d12/d8/d6 buttons fired a contextless "requests a dN roll", bypassing the "Type a move before declaring" guard. They now bind the roll to the player's typed intent if present, else attach the latest narrative line as context, so the DM never resolves a bare die — parity with the Declare text-guard. Logic extracted to a pure, unit-tested
composeRollMove.4. Drop-confirm on equipped gear (adversarial minor)
A one-click Drop on equipped body armor was irreversible with no confirmation. Both Drop affordances (context-menu + detail pane) now gate behind
window.confirmonly for currently-equipped items (isItemEquipped); loose stash items are never nagged. Samewindow.confirmpattern as quickload / seed-reset.Tests
test_sanitize_narration.py(extended): dice-header strip (single / chained / triple-chain / trailing-rule), leading & standalone horizontal rules, and a hardened negative-prose suite (number-after-"at" with no roll verb, roll-verb with a noun target, em-dash asides).test_battle_log_sanitize.py(new): renders the REALBattleLogLine(transpilesscreen-table.jsx+screen-combat.jsxinto one sandbox, mirroring the sibling browser<script>tags) — asserts meta-text/roll-header/title-scaffolding are stripped AND real combat text + meta badges survive.test_viewer_ux_guards.py(new):composeRollMove(typed-intent vs. attached-context vs. graceful floor) +isItemEquipped/dropEquippedConfirmMessage(equipped flagged, loose never nagged, defensive).All exercise the SHIPPED
.jsxvia the bundled Babel under Node — no reimplementation.Validation
GitHub Actions CI is currently billing-locked (jobs won't start) — validated locally:
test_portrait_gen.py(ModuleNotFoundError: No module named 'pydantic'), a pre-existing env gap onorigin/mainunrelated to this change (touches only server-side portrait subprocess; my diff is JSX + tests).PR opens for the merge queue when CI returns.