Uh oh!
There was an error while loading. Please reload this page.
docs(roadmap): mark 1.P Done + pre-1.Q hardening (condition-edge validation + Sonnet-reviewed) - #21
Merged
Conversation
…e next workstream PR #20 merged — the six non-agent NodeExecutor arms (condition / transform / fan_out / fan_in / input / output) behind a dispatching executor, executor-only, with the pre-merge BLOCKER secret-leak fixed via a secretInputNames masking gate on NodeExecContext. - phase-1-engine-and-llm.md: §1.P → ✅ Done (PR #20); top status block updated; 1.Q/1.R/1.S/1.AC remain. - current.md: 1.P ✅ Done in both status passages; the human gate (1.Q) named as next. - CLAUDE.md + README.md: status lines advanced to 1.P done, 1.Q next. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…condition (1.M) A pre-1.Q triage of deferred-tasks.md. Most open items are legitimately blocked (a future workstream, an external key/license, or a recorded decision); two were actionable now. - dag.ts (1.M validation): `validateStructuralEdge` now rejects a plain (handle-less) edge whose `from` is a `condition` node — such an edge would wire a dependent the handler's `selected` never names, a silently-dead downstream. Reuses the `invalid_handle` issue; safe (no fixture/spec/test used one — conditions route via `branches` + `nodeId:when` handles). Pinned by dag.test.ts and documented in workflow-yaml-spec.md §edges. - Bookkeeping: the **secret-into-`run.outputs` runtime-taint** item (ADR-0029(c)) is closed at the source by 1.P — `buildExpressionScope` masks secret inputs, so a transform/condition/merge_fn can never read a raw secret to launder; no runtime taint needed. Checked off. deferred-tasks.md triage recorded: `ctx.*` threading is the highest-value open engine gap but is a run-lifecycle + seam change (async `resolveContext` at run start) — best folded into 1.Q/1.R, not a drive-by. Everything else stays legitimately deferred. pnpm turbo run lint typecheck test build format:check: green (577 core tests). Leakwatch: 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-NITS) The Sonnet review of the hardening pass returned MERGE-WITH-NITS (zero blocker/high; the secret-taint closure confirmed substantively correct). Folds the confirmed mediums/lows. - DOC-1 (medium): the handle-less-condition-edge rejection message + the workflow-yaml-spec §edges note overstated "silently dead node" — a plain edge to a node that IS a branch target is ALSO rejected, but that target IS activated (via materialization), so it's redundant, not dead. Reworded both (dag.ts message + the spec note) to cover both cases (redundant-with-a-branch-target OR a never-activated node). - T-1 (medium): added a dag.test.ts case pinning that a plain edge from a condition to its OWN branch target is also rejected — the rejection is unconditional (unlike the `parallel_of` "redundant-if-agrees" leniency); the existing test only covered the dead-downstream (stray) topology. - DOC-2 (low): run-plan.md's `invalid_handle` enumeration now lists the new plain-edge-from-condition case. - TRIAGE-02 (nit): clarified `merge_fn` → fan_in `merge_fn` in the secret-taint closure entry. Skipped (with reason): the double-push of unknown_edge_target + invalid_handle for a missing `to` (both true, harmless, parse fails regardless); assertion-strength + test-title nits (subsumed by the new test); TRIAGE-01/03/04 doc-annotation polish (the substance is already accurate). pnpm turbo run lint typecheck test build format:check: green (578 core tests). Leakwatch: 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…findings) The re-run of the two socket-failed Sonnet dimensions confirmed every ✅ claim GENUINE — an exhaustive (a)–(g) trace verified the secret-taint closure is airtight (no pipe/filter unwrap, no scope bypass), and the audited resolved-items (handle-less edge, six handlers + dispatcher + secretInputNames gate, tool-output spill, Sonar fixes, concurrent-agent test, reportDuplicates) all match the code. No real defects; the actionable items are doc/comment improvements that make implicit invariants explicit. - AUDIT-9 (medium): scope.ts `outputsRecord` now documents the load-bearing secret invariant — it does NOT re-mask; `run.outputs` stays raw-secret-free solely because the `input` handler masks at the ingress and expression handlers read through `buildExpressionScope`. A future handler emitting `ctx.inputs`-derived data MUST `maskSecretInputs` first. (Defense against a future tool/human-gate/subworkflow handler.) - SEC-3 (low): sse-event-schema.md §security now states a `MaskedSecret` marker can appear in `node:completed.output` / `run:completed.outputs` (the input node emits masked inputs) and surfaces must render it as a redacted placeholder. - AUDIT-6 (nit): corrected the `readBracket` description in deferred-tasks.md (it's a thin dispatcher, not literally "3-line"). AUDIT-2 was a false positive (no `node-handlers.test.ts:618-655` citation exists in deferred-tasks.md). pnpm turbo run lint typecheck test build format:check: green (578 core tests). Leakwatch: 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reviewer's GuideMarks roadmap item 1.P (node-type handlers) as Done across docs, and hardens the engine by adding structural validation to reject plain edges from condition nodes while documenting and pinning the secret-masking invariants and SSE/event behavior; no runtime behavior change beyond stricter parse-time validation. Sequence diagram for new condition-edge structural validationsequenceDiagram
participant WorkflowYAMLParser
participant buildRunPlan
participant validateStructuralEdge
participant Issues
WorkflowYAMLParser->>buildRunPlan: buildRunPlan(workflowDoc)
loop for each edge
buildRunPlan->>validateStructuralEdge: validateStructuralEdge(edge, fromNode, locator, issues)
alt edge.from is condition and edge.handle is absent
validateStructuralEdge->>Issues: issues.push(invalid_handle)
validateStructuralEdge-->>buildRunPlan: return
else other edge
validateStructuralEdge-->>buildRunPlan: addEdge(fromBase, edge.to)
end
end
buildRunPlan-->>WorkflowYAMLParser: throw WorkflowGraphError(issues)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Warning Review limit reached
More reviews will be available in 1 hour, 39 minutes, and 6 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a runtime guard in ChangesCondition node edge validation
Phase 1 completion documentation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
invalid_handleerror message for plain condition edges invalidateStructuralEdgeis quite long and repeats explanation already present in docs; consider shortening it and/or extracting a shared constant so the error text and spec stay aligned. - The two new tests in
dag.test.tsshare almost identical graph setup; you could factor the shared YAML into a small helper or fixture to reduce duplication and make future edge-case additions easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments- The new `invalid_handle` error message for plain condition edges in `validateStructuralEdge` is quite long and repeats explanation already present in docs; consider shortening it and/or extracting a shared constant so the error text and spec stay aligned.
- The two new tests in `dag.test.ts` share almost identical graph setup; you could factor the shared YAML into a small helper or fixture to reduce duplication and make future edge-case additions easier.
## Individual Comments### Comment 1
<locationpath="docs/reference/contracts/sse-event-schema.md"line_range="139" />
<code_context>
+The same `{ secret: true, ref }`**`MaskedSecret`** marker can also appear in **`node:completed.output`** (for an `input` node, which emits the masked inputs) and therefore in **`run:completed.outputs`** / **`run:failed.partialOutputs`** wherever a `secret`-typed input would otherwise surface — the engine masks `secret` inputs at the ingress so a raw secret never reaches an output payload (see [run-plan.md §output capture](../shared-core/run-plan.md)). **A surface rendering node/run outputs must treat a `MaskedSecret` object as a redacted placeholder, not displayable data.**
</code_context>
<issue_to_address>
**suggestion (typo):** Clarify the phrasing in “A surface rendering node/run outputs…” for grammatical correctness.
The phrase “A surface rendering node/run outputs must treat…” is ungrammatical. Please reword, e.g. “Any surface rendering of node/run outputs must treat a `MaskedSecret` object as a redacted placeholder…” or “A surface rendering of node/run outputs must treat…”.
```suggestionThe same `{ secret: true, ref }` **`MaskedSecret`** marker can also appear in **`node:completed.output`** (for an `input` node, which emits the masked inputs) and therefore in **`run:completed.outputs`** / **`run:failed.partialOutputs`** wherever a `secret`-typed input would otherwise surface — the engine masks `secret` inputs at the ingress so a raw secret never reaches an output payload (see [run-plan.md §output capture](../shared-core/run-plan.md)). **Any surface rendering of node/run outputs must treat a `MaskedSecret` object as a redacted placeholder, not displayable data.**```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Code Review
This pull request implements validation to reject plain (handle-less) edges originating from a condition node, ensuring routing from conditions is explicitly handled via branches or named handles. It also updates documentation regarding the landing of node-type handlers (1.P), the prevention of secret leaks via MaskedSecret placeholders in outputs, and adds corresponding unit tests. Feedback is provided to guard the new edge validation in packages/core/src/dag.ts to prevent redundant and confusing error messages when the edge endpoints themselves are invalid.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } | ||
| return; // a handled edge's dependency comes from branch materialization, never a second edge here | ||
| } | ||
| if (fromNode?.type === 'condition') { |
There was a problem hiding this comment.
If either the source node (fromNode) is undefined or the target node (edge.to) does not exist in the graph, the edge already has a critical structural error (unknown_edge_target) which is reported above. Checking fromNode?.type === 'condition' and pushing an invalid_handle error in those cases leads to redundant and confusing error messages for the user.
We should guard this check to ensure we only validate condition routing when both endpoints of the edge are valid.
Suggested change
| if(fromNode?.type==='condition'){ | |
| if(fromNode!==undefined&&nodesById.has(edge.to)&&fromNode.type==='condition'){ |
…#21 review) Addresses the PR #21 review of the new handle-less-condition-edge validation. - dag.ts (the valid finding): the condition-routing check now also requires `nodesById.has(edge.to)`, so an edge with a missing `to` (or `from`) — already reported as `unknown_edge_target` — does NOT also push a redundant/confusing `invalid_handle` for the same edge. `addEdge` no-ops on a phantom endpoint, so the fall-through is safe. Pinned by a new dag.test.ts case (condition edge to a nonexistent target ⇒ only `unknown_edge_target`, never `invalid_handle`). - dag.ts: shortened the `invalid_handle` message to the actionable instruction (route via `branches[].target_node` or the `nodeId:when` handle form); the redundant-vs-dead rationale lives in the code comment + workflow-yaml-spec.md §edges, not the error string. - sse-event-schema.md: grammar fix ("A surface rendering node/run outputs" → "Any surface rendering of node/run outputs"). Skipped (with reason): a shared error-message constant (over-engineering for one call site); factoring the two condition-edge tests into a fixture (dag.test.ts's idiom is self-contained inline YAML per test — ~40 tests follow it; a 2-test helper would break that consistency for ~3 lines). pnpm turbo run lint typecheck test build format:check: green (579 core tests). Leakwatch: 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
cemililik added a commit
that referenced
this pull request
Sep 1, 2026
… the transport W4 Step 1 — CR-40, and the 2.5.5 connect-timeout queue (#35, G32, #205) plus the orphaned-child half of #21. Two of the three measurements ADR-0088 opens with become assertions here. **A connect had no upper bound.** Measured: a transport whose `start()` never resolves hung `connectSdkTransport` indefinitely. Passing the SDK a `RequestOptions` would not have fixed it — `Client.connect` awaits `transport.start()` FIRST and with no options at all, and `start()` is where the hang lives. So the window opens before any I/O, the whole `client.connect()` is raced against it, and the REMAINING time is handed to the SDK's own `timeout` so its internal 60 s default can neither cut an authored 120 s window short nor outlive one. Four named deadlines replace four inherited or absent ones: stdio connect 120 s (a cold `npx` is the canonical example), network connect 30 s, discovery 60 s for the whole paged walk, call 60 s. **Cancellation was dropped.** Measured: an aborted `tools/call` was still pending 500 ms later. The capability closure took `(input)` and ignored its second parameter, and `McpConnection.callTool(name, args)` had nowhere to put a signal. Both round-trips now take an `AbortSignalLike`, bridged to a real `AbortSignal` at the SDK fence — `@relavium/shared` keeps `types: []` rather than widening its signal type to a DOM class. **A signal does not run `finally`.** Measured with a host that spawns a child exactly the way the SDK does: `SIGTERM` printed no teardown line and left the child alive with `ppid 1`. `run` and `agent run` tear down in a `finally`, which Node's default signal handling never unwinds. A cooperative guard on all four signals reaps the children and exits 128+signo, bounded so a wedged transport cannot turn Ctrl-C into a hung shell. `connect_timeout_ms` is authored on both server schemas, ceiling 600 000 ms, rejected above it rather than clamped (ADR-0086's shape). It is an inline CONNECTION field, so it is refused alongside a by-name `ref` — no precedence rule is needed because none is possible — and it enters the duplicate-declaration fingerprint for the reason `allow_local_endpoint` already does. It is deliberately NOT in the ADR-0084 consent digest: that answers "is this the same program", and a timeout changes nothing about what executes. **One of my own tests was hollow, and mutation caught it.** `raceDeadline` originally took a `dispose` callback said to be load-bearing; removing its call left every test green, because `connectSdkTransport`'s own `catch` already closes the client on every failure path. The parameter had no owner, so it is gone rather than left as a second mechanism nobody exercises — and the close assertion now binds the `catch`, verified by removing it (2 red). Every new test is break-verified: the race removed (6 red), the SDK timeout handoff dropped (1 red), the catch's close removed (2 red), the signal forwarding reverted (2 red), the guard's close removed (8 red), its latch removed (1 red). `pnpm run ci` and `pnpm coverage` both exit 0; `deadlines.ts` is at 100%. Refs: ADR-0088, ADR-0082, ADR-0084, ADR-0086 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cemililik added a commit
that referenced
this pull request
Sep 1, 2026
W4 is complete on `development`: five steps, eight review rounds, `CR-40`/`CR-41`/`CR-42` plus the whole 2.5.5 MCP queue, `#202`, `G19`, and the MCP half of `#21`. The closing register names, per item, the code that closes it AND the test that fails if that code is reverted — exit criterion 7's requirement, and the answer to the failure this phase keeps finding, where a register records a state the code does not hold. Three things are stated that a status line would otherwise hide. **What the wave cost.** Two previously-working configurations are now refused: a remote `websocket` server, and a project config redefining a global MCP registration. Both are in the reference specs with the remedy named, and both are consequences of decisions rather than side effects. **What it did not close.** Three residuals go to `deferred-tasks.md` rather than being implied by a green register: the admission and the dialer canonicalize a url differently (fails closed, so it costs usability not safety); the MCP hop does not go through `withEgressTimeout`; and `allow_local_endpoint` may still name a host whose resolution a LAN-adjacent attacker steers — narrowed by the dialer's refusal of a public or metadata answer, not eliminated. `#297` is satisfied incidentally rather than by a coverage threshold, and says so instead of being claimed. **What the reviews found.** Eight rounds, three blockers and nine highs — and every blocker was in a FIX rather than in the original code. The recurring shape was a guarantee asserted in a comment, an ADR or a commit message with nothing between it and the code: a §2.3 clause I wrote and never implemented, a fetch nothing proved arrived, a claim that "both checks now redden" when only one did. The wave's own tests were the other half — four could not fail at all, each stopping at a seam one layer above the thing it claimed to prove. That is recorded in the register rather than smoothed over, because the next wave inherits the habit, not the outcome. `current.md`, the phase doc and `CLAUDE.md` now agree on 28 of 48 closed. `W3`'s blocker is untouched and still first in its residuals. Refs: ADR-0088 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cemililik added a commit
that referenced
this pull request
Sep 1, 2026
…egister overclaiming **§7.2 was decided and never implemented.** `ToolDef.source` records that a description was written by an MCP server — the party the hostile-MCP class defends against — and both lowering sites dropped it, so the model received the server's text indistinguishable from the operator's. The ADR calls sanitization-plus-provenance "the honest mechanism"; only half of it existed. Advisory by design, exactly as §7.2 says, and now asserted on the request a provider actually receives rather than one seam above it. **`#208` claimed a test that does not exist.** The register cited `errors.test.ts` for the host-supplied client version; that file has no version test, and nothing else covered it — a deleted `setMcpClientVersion` call or a composition-root rewire was invisible. Now read back from a real server's handshake, which is where a server sees it. Four more register rows described a `W4` that was more complete than the code: `CR-42` did not stop paging, `#207`'s reporter could not fire, `CR-41`'s dialer was optional and its metadata carve-out incomplete, and `#21`'s e2e could not catch the orphan the smoke check now does. Each row now names what actually closes it and where the earlier claim was wrong. `W4` is marked **REVIEW-BLOCKED** rather than complete in all four status homes. Five merge blockers in one wave is not a detail to leave implied by a green register, and the phase doc records the pattern: the mechanisms were built and the bindings between them were assumed. Also recorded rather than rushed: the MCP schema compiler's denylist silently DROPS unsupported assertions — `{"type":"number","exclusiveMinimum":0}` compiles and accepts `0`, contradicting ADR-0052's fail-closed claim. Pre-existing, not a `W4` regression, and the fix (a per-type supported-key allowlist) starts refusing schemas that work today, so it needs its own change with its own compatibility review. Refs: ADR-0088 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A post-1.P-merge bookkeeping + pre-1.Q hardening batch. Docs-and-validation only on the engine side
(one small
dag.tsparse-time check + its test); no engine-behavior change to a run.Commits
ec99615— roadmap: 1.P ✅ DoneMarks the node-type handlers (1.P, PR #20) Done across phase-1-engine-and-llm.md, current.md, CLAUDE.md,
README.md; 1.Q (human gate) named as the next workstream.
2b19f7f— deferred-tasks hardening (1.M validation)A pre-1.Q triage of
deferred-tasks.md. Most open items are legitimately blocked (a future workstream,an external key/license, or a recorded decision); two were actionable now:
validateStructuralEdge): reject a plain (handle-less) edge whosefromis aconditionnode — it would wire a dependent the handler's
selectednever names (a silently-dead node), or beredundant with a branch target. Reuses the
invalid_handleissue; safe (no fixture/spec/test used one).Pinned by
dag.test.ts+ documented in workflow-yaml-spec.md §edges.run.outputsruntime-taint item is closed at the source by 1.P(
buildExpressionScopemasks secret inputs, so a transform/condition/merge_fn can never read a rawsecret to launder). The
ctx.*-threading item is annotated as a run-lifecycle + seam change (asyncresolveContextat run start) — best folded into 1.Q/1.R, not a drive-by.d3eca05+01b5933— Sonnet pre-merge review, foldedTwo multi-dimensional Sonnet reviews (each finding adversarially verified) audited this batch with a
focus on verifying every ✅-marked item is genuinely resolved. Verdict MERGE-WITH-NITS — zero
blocker/high, zero real defects. The secret-taint closure was confirmed airtight by an exhaustive path
trace (no pipe/filter unwrap, no scope bypass), and all audited resolved-items match the code. Folded:
a plain edge to a branch target is also rejected, but that target IS activated, so it's redundant).
invalid_handleenumeration updated.outputsRecorddoes not re-mask; a futurehandler emitting
ctx.inputs-derived data mustmaskSecretInputsfirst).MaskedSecretmarker can appear innode:completed.output/run:completed.outputs(surfaces must render it as a redacted placeholder).
Validation
pnpm turbo run lint typecheck test build format:check: green — 578 core tests.🤖 Generated with Claude Code
Summary by Sourcery
Mark node-type handlers roadmap item 1.P as completed and harden pre-1.Q validation and contracts around condition edges and secret masking in outputs.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
Documentation
Bug Fixes
Tests