Skip to content

Commit 09a876b

Browse files
AurelioloAureliolo
committed
fix: address 7 CodeRabbit round-2 review items
- task_engine: use async put (not put_nowait) for shutdown sentinel to avoid QueueFull aborting stop(); remove early-exit check from observer timeout branch -- loop now exits only on None sentinel - condition_eval: _parse_atom_token raises ValueError for missing operands, unclosed parens, and bare operator tokens (malformed expressions correctly resolve to False via outer handler) - yaml-to-nodes: only advance branch counter for explicit 'true' (not 'false') so mixed explicit+implicit entries stay synchronized - security.md: add window.open() sessionStorage inheritance caveat with noopener recommendation - tests: update unclosed-paren expectation (False, not True), use object form in counter-inference test, add positive assertions for parallel_branch depends_on
1 parent b3a03c5 commit 09a876b

7 files changed

Lines changed: 29 additions & 18 deletions

File tree

‎docs/security.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ surface:
8585
|---------|-----------|
8686
| **XSS prevention** | ESLint `no-restricted-syntax` rule bans `dangerouslySetInnerHTML` at write time. Override requires `// eslint-disable-next-line` with justification. |
8787
| **CSP nonce readiness** | `<MotionConfig nonce>` wrapper in `App.tsx` + `lib/csp.ts` reader. Framer Motion's dynamically injected `<style>` tags are nonce-ready. `react-style-singleton` (used by Radix Dialog/AlertDialog/Popover via `react-remove-scroll`) also supports nonces via the `get-nonce` package -- wiring `setNonce()` in `lib/csp.ts` is the remaining step. Currently staged -- see the activation checklist in `web/index.html` and the [accepted risk](#accepted-risk-inline-style-attributes) section below. |
88-
| **JWT storage** | `sessionStorage` (tab-scoped) with short-lived tokens, automatic expiry cleanup, and 401 interceptor. Tokens do not persist across browser sessions or leak to other tabs. Cookie-based auth (httpOnly) is a future enhancement tracked separately. |
88+
| **JWT storage** | `sessionStorage` (tab-scoped) with short-lived tokens, automatic expiry cleanup, and 401 interceptor. Tokens do not persist across browser sessions. Note: `window.open()` can clone the opener's sessionStorage into the child tab -- all programmatic `window.open` calls must use the `noopener` option and all `<a target="_blank">` links must include `rel="noopener"` to prevent token transfer. Cookie-based auth (httpOnly) is a planned future enhancement tracked separately. |
8989

9090
### Accepted Risk: Inline Style Attributes
9191

‎src/synthorg/engine/task_engine.py‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,9 @@ async def stop(self, *, timeout: float | None = None) -> None: # noqa: ASYNC109
186186

187187
await self._drain_processing(effective_timeout)
188188
# Signal the observer loop that no more events will arrive.
189-
self._observer_queue.put_nowait(None)
189+
# Use async put (not put_nowait) to avoid QueueFull aborting
190+
# shutdown when the observer queue is backed up.
191+
await self._observer_queue.put(None)
190192
observer_budget = max(0.0, deadline - asyncio.get_event_loop().time())
191193
await self._drain_observer(observer_budget)
192194

@@ -767,10 +769,6 @@ async def _observer_dispatch_loop(self) -> None:
767769
timeout=self._POLL_INTERVAL_SECONDS,
768770
)
769771
except TimeoutError:
770-
# Re-check: if not running and queue empty, the
771-
# sentinel may have arrived while we were waiting.
772-
if not self._running and self._observer_queue.empty():
773-
break
774772
continue
775773
if event is None:
776774
break # sentinel -- processing loop is done

‎src/synthorg/engine/workflow/condition_eval.py‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -191,22 +191,30 @@ def _parse_atom_token(
191191
pos: int,
192192
context: Mapping[str, object],
193193
) -> tuple[bool, int]:
194-
"""Parse an atom: parenthesized group or single comparison."""
194+
"""Parse an atom: parenthesized group or single comparison.
195+
196+
Raises:
197+
ValueError: On missing operand, unclosed parenthesis, or
198+
unexpected token.
199+
"""
195200
if pos >= len(tokens):
196-
return False, pos
201+
msg = "Missing operand"
202+
raise ValueError(msg)
197203

198204
if tokens[pos] == "(":
199205
pos += 1 # consume '('
200206
value, pos = _parse_or(tokens, pos, context)
201-
if pos < len(tokens) and tokens[pos] == ")":
202-
pos += 1 # consume ')'
203-
return value, pos
207+
if pos >= len(tokens) or tokens[pos] != ")":
208+
msg = "Unclosed parenthesis"
209+
raise ValueError(msg)
210+
return value, pos + 1 # consume ')'
204211

205212
# Anything else is an atom (comparison or key lookup)
206213
token = tokens[pos]
207-
# Guard: skip bare operators that ended up as atoms
214+
# Guard: bare operators that ended up as atoms are a parse error
208215
if token in ("AND", "OR", "NOT", ")"):
209-
return False, pos + 1
216+
msg = f"Unexpected token: {token}"
217+
raise ValueError(msg)
210218
return _eval_atom(token, context), pos + 1
211219

212220

‎tests/unit/engine/workflow/test_condition_eval.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -288,7 +288,8 @@ def test_leading_operator(self) -> None:
288288
assert evaluate_condition("AND true", {}) is False
289289

290290
def test_unclosed_paren(self) -> None:
291-
assert evaluate_condition("(a == 1 AND b == 2", {"a": "1", "b": "2"}) is True
291+
# Unclosed paren is a parse error -- resolves to False.
292+
assert evaluate_condition("(a == 1 AND b == 2", {"a": "1", "b": "2"}) is False
292293

293294
def test_double_not(self) -> None:
294295
assert evaluate_condition("NOT NOT true", {}) is True

‎web/src/__tests__/pages/workflow-editor/workflow-to-yaml.test.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,8 @@ describe('generateYamlPreview depends_on', () => {
9090
]
9191
const yaml = generateYamlPreview(nodes, edges, 'test', 'agile')
9292
// parallel_branch edges should emit plain string depends_on
93+
expect(yaml).toContain('depends_on')
94+
expect(yaml).toContain('- fork')
9395
expect(yaml).not.toContain('branch:')
9496
})
9597

‎web/src/__tests__/pages/workflow-editor/yaml-to-nodes.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -119,11 +119,11 @@ workflow_definition:
119119
- id: yes_step
120120
type: task
121121
depends_on:
122-
- check
122+
- id: check
123123
- id: no_step
124124
type: task
125125
depends_on:
126-
- check
126+
- id: check
127127
`
128128
const result = parseYamlToNodesEdges(yaml)
129129
expect(result.errors).toHaveLength(0)

‎web/src/pages/workflow-editor/yaml-to-nodes.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -264,8 +264,10 @@ export function parseYamlToNodesEdges(
264264
if (sourceStep.type !== 'conditional') {
265265
warnings.push(`Step '${stepId}': explicit branch '${explicitBranch}' on non-conditional dependency '${depId}'`)
266266
}
267-
// Increment counter so mixed explicit+implicit entries stay in sync
268-
if (sourceStep.type === 'conditional' && sourceStep.step.condition) {
267+
// Only advance the counter when the true slot is consumed so
268+
// a subsequent implicit entry correctly gets the false slot.
269+
// Explicit false does NOT advance -- the true slot is still open.
270+
if (sourceStep.type === 'conditional' && sourceStep.step.condition && explicitBranch === 'true') {
269271
const branchIdx = conditionalBranchCounters.get(depId) ?? 0
270272
conditionalBranchCounters.set(depId, branchIdx + 1)
271273
}

0 commit comments

Comments
 (0)