Implement v18 property projection cutover#101
Open
flyingrobots wants to merge 8 commits into
Open
Conversation
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 31 minutes and 8 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR implements the final closeout of V18 property projection work (slices 31–35) by introducing typed property write intents, extending graph operations to support property-set operations, refactoring state readers and query controllers to route through projection-backed helpers, and adding snapshot-to-projection hydration for immutable reader sources. ChangesV18 Property Projection Closeout (Slices 31–35)
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant Reader as createStateReader
participant Hydration as createStateReaderProjectionState
participant Snapshot as SnapshotWarpState
participant Projection as NodePropertyProjection
Client->>Reader: createStateReader(source: StateReaderSource)
Reader->>Hydration: convert source to live WarpState
Hydration->>Snapshot: check if SnapshotWarpState
Snapshot->>Hydration: warpStateFromSnapshot (rebuild CRDT/frontier)
Hydration-->>Reader: liveState (hydrated or original)
Reader->>Projection: derive property records
Projection-->>Reader: visible node/edge property records
Reader-->>Client: VisibleStateReader with projection-backed props
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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)
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 |
Release Preflight
If you tag this commit as |
Release Preflight
If you tag this commit as |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/domain/services/state/StateReader.ts (1)
193-207: ⚡ Quick winCache node property records once during context build.
createProjectionProps(projectionState)andcreateNodePropertyRecords(projectionState)both end up walkingNodePropertyProjection.fromState(...), so each reader build decodes/sorts the same node-property set twice. Materializing that array once here and deriving bothprojection.propsandnodePropsByIdfrom it would avoid the duplicate pass.🤖 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 `@src/domain/services/state/StateReader.ts` around lines 193 - 207, Materialize node property records once: call createNodePropertyRecords(projectionState) into a local const (e.g., nodePropertyRecords) and reuse it for both building projection.props and populating nodePropsById instead of calling createNodePropertyRecords(...) twice; to do this, add or modify a helper so you can derive projection.props from the precomputed records (e.g., createProjectionPropsFromRecords(nodePropertyRecords) or adjust createProjectionProps to accept the records), then pass nodePropertyRecords into populateVisibleNodeProps(nodePropertyRecords, nodePropsById) and remove the duplicate createNodePropertyRecords(...) call.
🤖 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 @.github/workflows/ci.yml:
- Around line 36-38: Replace floating action tags by pinning the uses fields for
actions/checkout and actions/setup-node to their immutable commit SHAs (replace
uses: actions/checkout@v6 and uses: actions/setup-node@v6 with
actions/checkout@<commit-sha> and actions/setup-node@<commit-sha>, and update
every occurrence), add a top-level permissions: block with least-privilege
scopes required by the workflow, and set persist-credentials: false on each
actions/checkout step to avoid credential persistence (locate and modify the
checkout steps and the workflow header permissions in the CI yaml).
- Around line 33-48: The workflow grants excessive token scope and leaves
checkout credentials persisted; for the job type-firewall-lint (and all other
jobs using actions/checkout@v6) add a minimal job-level permissions block (e.g.,
permissions: contents: read and only the specific permissions needed) and update
each actions/checkout@v6 step to include with: persist-credentials: false so the
GITHUB_TOKEN is not left in the runner git config; apply the same changes to all
jobs named in the comment (type-firewall-types, type-firewall-lint,
type-firewall-semgrep, type-firewall-quarantine, type-firewall-surface,
type-firewall-docs, type-firewall-audit-advisory, typecheck-test-advisory,
test-node, test-bun, test-deno, coverage-threshold) to harden token scope and
disable credential persistence.
In `@src/domain/services/PatchBuilder.ts`:
- Around line 463-507: requirePatchPropertyValue currently can recurse forever
on cyclic inputs and can be polluted via prototype keys; fix by adding cycle
detection (pass a WeakSet seen through recursive calls and throw PatchError with
code 'E_PATCH_INVALID_PROPERTY_VALUE' if a non-scalar object/array is already in
seen) and by guarding against prototype-polluting keys when normalizing plain
objects (either reject keys like '__proto__' and 'constructor' with PatchError
or create records with Object.create(null) and still reject any key named
'__proto__' or 'constructor'); update requirePatchPropertyValue to accept an
internal seen: WeakSet<object> param, mark objects/arrays before recursing, and
keep isPlainPatchPropertyObject as-is for detection but ensure the object
key-check happens in requirePatchPropertyValue when building record/array to
deterministically fail on cycles and prototype-key pollution.
In `@test/unit/domain/services/PatchBuilderPropertyIntent.test.ts`:
- Around line 16-63: Add unit tests in PatchBuilderPropertyIntent.test.ts that
exercise the remaining property normalization branches: add a test that calls
createBuilder(...) and builder.setProperty('node:1','bin', new
Uint8Array([1,2,3])) then build() and assert the op value accepts Uint8Array and
patch.schema is correct; add tests that pass nested/recursive structures via
builder.setProperty (e.g., arrays and objects) to ensure they are normalized and
appended to ops by build(); and add a test that supplies a cyclic value to
builder.setProperty and asserts it throws PatchError (or the same cyclic
rejection path) and that builder.build().ops remains empty. Reference the
existing helpers and symbols used in the file (createBuilder,
builder.setProperty, setEdgeProperty, InvalidPropertyCarrier, PatchError,
requirePropSet, encodeLegacyEdgePropNode) so tests align with current test
patterns.
In `@test/unit/domain/services/StateReaderPropertyProjection.test.ts`:
- Around line 26-81: Add a parallel test that constructs a SnapshotWarpState via
warpStateFromSnapshot (or by calling StateReaderSource.snapshot path) containing
the same nodes/edges/prop entries as the existing WarpState test, then call
createStateReader with that SnapshotWarpState and assert the same results for
getNodeProps, getEdgeProps, getEdges, project().props and
getNodeContentMeta/getEdgeContentMeta as in the live-state cases; locate where
the existing tests create the live WarpState (functions addLiveNode,
addLiveEdge, encodePropKey, encodeEdgePropKey) and replicate those setup steps
but convert the final state into a SnapshotWarpState using warpStateFromSnapshot
(or the StateReaderSource API) before creating the reader so the snapshot code
path (SnapshotWarpState/StateReaderSource) is exercised and assertions remain
identical.
---
Nitpick comments:
In `@src/domain/services/state/StateReader.ts`:
- Around line 193-207: Materialize node property records once: call
createNodePropertyRecords(projectionState) into a local const (e.g.,
nodePropertyRecords) and reuse it for both building projection.props and
populating nodePropsById instead of calling createNodePropertyRecords(...)
twice; to do this, add or modify a helper so you can derive projection.props
from the precomputed records (e.g.,
createProjectionPropsFromRecords(nodePropertyRecords) or adjust
createProjectionProps to accept the records), then pass nodePropertyRecords into
populateVisibleNodeProps(nodePropertyRecords, nodePropsById) and remove the
duplicate createNodePropertyRecords(...) call.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42937f48-5de7-4608-85e0-a36a20e15952
📒 Files selected for processing (33)
.github/workflows/ci.ymlCHANGELOG.mddocs/BEARING.mddocs/design/0179-v18-state-reader-property-projection/v18-state-reader-property-projection.mddocs/design/0180-v18-property-write-intent-nouns/v18-property-write-intent-nouns.mddocs/design/0181-v18-patchbuilder-property-intent-lowering/v18-patchbuilder-property-intent-lowering.mddocs/design/0182-v18-graph-op-projection-property-cutover/v18-graph-op-projection-property-cutover.mddocs/design/0183-v18-property-projection-closeout/v18-property-projection-closeout.mddocs/method/backlog/v18.0.0/PROTO_legacy-props-as-projection.mdsrc/domain/graph/EdgePropertyWriteIntent.tssrc/domain/graph/GraphContentAttachmentSetOp.tssrc/domain/graph/GraphEdgePropertySetOp.tssrc/domain/graph/GraphNodePropertySetOp.tssrc/domain/graph/GraphOpAlgebra.tssrc/domain/graph/GraphOperation.tssrc/domain/graph/NodePropertyWriteIntent.tssrc/domain/graph/publicGraphSubstrate.tssrc/domain/services/GraphOpAlgebraProjection.tssrc/domain/services/PatchBuilder.tssrc/domain/services/TranslationCost.tssrc/domain/services/controllers/QueryReads.tssrc/domain/services/query/StateQueryReadModel.tssrc/domain/services/state/StateReader.tssrc/domain/services/state/StateReaderContext.tstest/type-check/consumer.tstest/unit/domain/WarpGraph.coverageGaps.test.tstest/unit/domain/graph/GraphOpAlgebra.test.tstest/unit/domain/graph/PropertyWriteIntent.test.tstest/unit/domain/services/GraphOpAlgebraProjection.test.tstest/unit/domain/services/PatchBuilderPropertyIntent.test.tstest/unit/domain/services/QueryReadsPropertyProjection.test.tstest/unit/domain/services/StateReaderPropertyProjection.test.tstest/unit/domain/services/query/StateQueryReadModelPropertyProjection.test.ts
Comment on lines
+33
to
48
| type-firewall-lint: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - name: Use Node.js | ||
| uses: actions/setup-node@v6 | ||
| with: | ||
| node-version: '22' | ||
| cache: 'npm' | ||
| - run: npm ci | ||
| - name: 'Gate 4: ESLint (typed rules + no-explicit-any)' | ||
| run: npm run lint | ||
| - name: 'Gate 4b: Lint ratchet (zero-error invariant)' | ||
| run: npm run lint:ratchet | ||
| - name: 'Gate 4c: Anti-sludge shell checks (junk-drawer filenames)' | ||
| run: npm run lint:sludge |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/ci.yml"
echo "== File = $FILE =="
ls -la "$FILE"
echo
echo "== Show lines 1-220 =="
nl -ba "$FILE" | sed -n '1,220p'Repository: git-stunts/git-warp
Length of output: 261
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/ci.yml"
echo "== Search for permissions and checkout credential handling =="
rg -n "permissions:|persist-credentials|token:|persist_credentials|checkout@" "$FILE" -S || trueRepository: git-stunts/git-warp
Length of output: 576
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/ci.yml"
echo "== Extract all checkout steps =="
python3 - <<'PY'
import re, pathlib
p = pathlib.Path(".github/workflows/ci.yml")
txt = p.read_text()
# naive scan: locate lines containing uses: actions/checkout@
lines = txt.splitlines()
for i,l in enumerate(lines, start=1):
if "uses:" in l and "actions/checkout" in l:
print(f"{i}: {l.strip()}")
# show next 6 lines for context
for j in range(i, min(i+7, len(lines)+1)):
print(f" {j}: {lines[j-1].rstrip()}")
print()
PYRepository: git-stunts/git-warp
Length of output: 3318
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/ci.yml"
echo "== permissions-related grep =="
rg -n "^(permissions:|\\s+permissions:|persist-credentials:|checkout@|GITHUB_TOKEN|token:)" "$FILE" -S || true
echo
echo "== Show lines 1-120 =="
cat -n "$FILE" | sed -n '1,120p'
echo
echo "== Show lines 120-260 =="
cat -n "$FILE" | sed -n '120,260p'Repository: git-stunts/git-warp
Length of output: 9154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/ci.yml"
echo "== Extract each checkout step block (uses + following 10 lines) =="
python3 - <<'PY'
import pathlib
p = pathlib.Path(".github/workflows/ci.yml")
lines = p.read_text().splitlines()
for i,l in enumerate(lines):
if "uses: actions/checkout@" in l:
start=i
end=min(len(lines), i+15)
print(f"\n-- checkout at line {i+1} --")
for j in range(start, end):
print(f"{j+1}: {lines[j]}")
PYRepository: git-stunts/git-warp
Length of output: 6790
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/ci.yml"
echo "== Search for write-requiring patterns =="
rg -n "permissions:|GITHUB_TOKEN|token:|persist-credentials|checkout@|write|contents: write|pull-requests:|issues:|statuses:|checks:|deployments:|actions/upload-artifact|comment|gh api|create|push|workflow_dispatch" "$FILE" -S || true
echo
echo "== Search for any explicit checkout 'with:' blocks =="
python3 - <<'PY'
import pathlib
p = pathlib.Path(".github/workflows/ci.yml")
lines = p.read_text().splitlines()
for i,l in enumerate(lines):
if "uses: actions/checkout@" in l:
# check next 6 lines for 'with:'
window = lines[i:i+10]
if any("with:" in x for x in window):
for j in range(i, i+10):
print(f"{j+1}: {lines[j]}")
print("----")
PYRepository: git-stunts/git-warp
Length of output: 4447
🌐 Web query:
actions/checkout persist-credentials default value
💡 Result:
The default value of actions/checkout input persist-credentials is true (i.e., it persists the token/credentials in the local git configuration by default). [1][2] In the checkout action README, persist-credentials is described as “Whether to configure the token or SSH key with the local git config” with Default: true. [1] The action.yml also sets the input default for persist-credentials to true. [2]
Citations:
Harden GITHUB_TOKEN scope and disable checkout credential persistence.
.github/workflows/ci.yml has no workflow/job-level permissions: minimization, and every actions/checkout@v6 step runs without with: persist-credentials: false (so the default credentials/token remain configured in the runner git config).
Suggested baseline
+permissions:
+ contents: read
jobs:
type-firewall-lint:
runs-on: ubuntu-latest
steps:
- - uses: actions/checkout@v6
+ - uses: actions/checkout@<full-commit-sha>
+ with:
+ persist-credentials: falseApplies to all jobs that use actions/checkout@v6 in this workflow (type-firewall-types, type-firewall-lint, type-firewall-semgrep, type-firewall-quarantine, type-firewall-surface, type-firewall-docs, type-firewall-audit-advisory, typecheck-test-advisory, test-node, test-bun, test-deno, coverage-threshold).
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 36-36: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 33-48: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[error] 36-36: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 38-38: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 38-38: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): this step
(cache-poisoning)
🤖 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 @.github/workflows/ci.yml around lines 33 - 48, The workflow grants excessive
token scope and leaves checkout credentials persisted; for the job
type-firewall-lint (and all other jobs using actions/checkout@v6) add a minimal
job-level permissions block (e.g., permissions: contents: read and only the
specific permissions needed) and update each actions/checkout@v6 step to include
with: persist-credentials: false so the GITHUB_TOKEN is not left in the runner
git config; apply the same changes to all jobs named in the comment
(type-firewall-types, type-firewall-lint, type-firewall-semgrep,
type-firewall-quarantine, type-firewall-surface, type-firewall-docs,
type-firewall-audit-advisory, typecheck-test-advisory, test-node, test-bun,
test-deno, coverage-threshold) to harden token scope and disable credential
persistence.
Comment on lines
+36
to
+38
| - uses: actions/checkout@v6 | ||
| - name: Use Node.js | ||
| uses: actions/setup-node@v6 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/ci.yml"
test -f "$FILE" || { echo "Missing $FILE"; exit 1; }
echo "== File header =="
head -n 80 "$FILE" | cat -n
echo
echo "== Target region around lines 30-140 =="
sed -n '25,140p' "$FILE" | cat -n
echo
echo "== Full grep for actions/checkout/setup-node and permissions/persist-credentials =="
rg -n "uses:\s*actions/(checkout|setup-node)@|permissions:|persist-credentials:" "$FILE" || trueRepository: git-stunts/git-warp
Length of output: 8890
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/ci.yml"
echo "== rg 'permissions' (case-sensitive) =="
rg -n "permissions" "$FILE" || true
echo
echo "== rg 'persist-credentials' (case-sensitive) =="
rg -n "persist-credentials" "$FILE" || trueRepository: git-stunts/git-warp
Length of output: 152
Pin GitHub Actions to immutable SHAs.
.github/workflows/ci.yml uses floating @v6 tags for actions/checkout/actions/setup-node in CI jobs (e.g., lines 36-38, 53-55, 68-70, 92-94, 105-107, 121-123; also later in the file around ~162-164, ~178-180, and ~215-217), which weakens supply-chain integrity for CI gates.
- No
permissions:blocks found (least-privilege hardening missing). - No
persist-credentials: falsefound onactions/checkoutsteps (credential persistence/blast-radius not reduced).
Suggested hardening pattern
- - uses: actions/checkout@v6
+ - uses: actions/checkout@<full-commit-sha>
- - uses: actions/setup-node@v6
+ - uses: actions/setup-node@<full-commit-sha>🧰 Tools
🪛 zizmor (1.25.2)
[warning] 36-36: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 36-36: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 38-38: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 38-38: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): this step
(cache-poisoning)
🤖 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 @.github/workflows/ci.yml around lines 36 - 38, Replace floating action tags
by pinning the uses fields for actions/checkout and actions/setup-node to their
immutable commit SHAs (replace uses: actions/checkout@v6 and uses:
actions/setup-node@v6 with actions/checkout@<commit-sha> and
actions/setup-node@<commit-sha>, and update every occurrence), add a top-level
permissions: block with least-privilege scopes required by the workflow, and set
persist-credentials: false on each actions/checkout step to avoid credential
persistence (locate and modify the checkout steps and the workflow header
permissions in the CI yaml).
Comment on lines
+463
to
+507
| /** Validates public patch property values before intent construction. */ | ||
| function requirePatchPropertyValue(value: unknown): PropValue { // nosemgrep: ts-no-unknown-outside-adapters -- public PatchBuilder boundary | ||
| if (isScalarPatchPropertyValue(value)) { | ||
| return value; | ||
| } | ||
| if (value instanceof Uint8Array) { | ||
| return value; | ||
| } | ||
| if (Array.isArray(value)) { | ||
| return value.map((entry) => requirePatchPropertyValue(entry)); | ||
| } | ||
| if (isPlainPatchPropertyObject(value)) { | ||
| const record: { [key: string]: PropValue } = {}; | ||
| for (const [key, entry] of Object.entries(value)) { | ||
| record[key] = requirePatchPropertyValue(entry); | ||
| } | ||
| return record; | ||
| } | ||
| throw new PatchError('Property value must be property-compatible data', { | ||
| code: 'E_PATCH_INVALID_PROPERTY_VALUE', | ||
| }); | ||
| } | ||
|
|
||
| /** Returns true for scalar property values. */ | ||
| function isScalarPatchPropertyValue( | ||
| value: unknown, // nosemgrep: ts-no-unknown-outside-adapters -- public PatchBuilder boundary | ||
| ): value is string | number | boolean | null { | ||
| return value === null | ||
| || typeof value === 'string' | ||
| || typeof value === 'number' | ||
| || typeof value === 'boolean'; | ||
| } | ||
|
|
||
| /** Returns true for plain recursive property objects. */ | ||
| function isPlainPatchPropertyObject( | ||
| value: unknown, // nosemgrep: ts-no-unknown-outside-adapters -- public PatchBuilder boundary | ||
| ): value is { readonly [key: string]: unknown } { // nosemgrep: ts-no-unknown-outside-adapters -- public PatchBuilder boundary | ||
| if (value === null || typeof value !== 'object') { | ||
| return false; | ||
| } | ||
| if (Array.isArray(value) || value instanceof Uint8Array) { | ||
| return false; | ||
| } | ||
| return Object.getPrototypeOf(value) === Object.prototype | ||
| || Object.getPrototypeOf(value) === null; |
There was a problem hiding this comment.
Guard recursive property normalization against cycles and prototype-key pollution.
requirePatchPropertyValue can recurse infinitely on cyclic arrays/objects, and writing dynamic keys into {} allows __proto__ to mutate the normalized object’s prototype. Both should fail deterministically with PatchError.
🔧 Suggested hardening patch
-function requirePatchPropertyValue(value: unknown): PropValue { // nosemgrep: ts-no-unknown-outside-adapters -- public PatchBuilder boundary
+function requirePatchPropertyValue(
+ value: unknown,
+ seen: WeakSet<object> = new WeakSet(),
+): PropValue { // nosemgrep: ts-no-unknown-outside-adapters -- public PatchBuilder boundary
if (isScalarPatchPropertyValue(value)) {
return value;
}
if (value instanceof Uint8Array) {
return value;
}
if (Array.isArray(value)) {
- return value.map((entry) => requirePatchPropertyValue(entry));
+ if (seen.has(value)) {
+ throw new PatchError('Property value must be property-compatible data', {
+ code: 'E_PATCH_INVALID_PROPERTY_VALUE',
+ });
+ }
+ seen.add(value);
+ const normalized = value.map((entry) => requirePatchPropertyValue(entry, seen));
+ seen.delete(value);
+ return normalized;
}
if (isPlainPatchPropertyObject(value)) {
- const record: { [key: string]: PropValue } = {};
+ if (seen.has(value)) {
+ throw new PatchError('Property value must be property-compatible data', {
+ code: 'E_PATCH_INVALID_PROPERTY_VALUE',
+ });
+ }
+ seen.add(value);
+ const record: { [key: string]: PropValue } = Object.create(null) as { [key: string]: PropValue };
for (const [key, entry] of Object.entries(value)) {
- record[key] = requirePatchPropertyValue(entry);
+ record[key] = requirePatchPropertyValue(entry, seen);
}
+ seen.delete(value);
return record;
}
throw new PatchError('Property value must be property-compatible data', {
code: 'E_PATCH_INVALID_PROPERTY_VALUE',
});
}📝 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.
Suggested change
| /** Validates public patch property values before intent construction. */ | |
| function requirePatchPropertyValue(value: unknown): PropValue { // nosemgrep: ts-no-unknown-outside-adapters -- public PatchBuilder boundary | |
| if (isScalarPatchPropertyValue(value)) { | |
| return value; | |
| } | |
| if (value instanceof Uint8Array) { | |
| return value; | |
| } | |
| if (Array.isArray(value)) { | |
| return value.map((entry) => requirePatchPropertyValue(entry)); | |
| } | |
| if (isPlainPatchPropertyObject(value)) { | |
| const record: { [key: string]: PropValue } = {}; | |
| for (const [key, entry] of Object.entries(value)) { | |
| record[key] = requirePatchPropertyValue(entry); | |
| } | |
| return record; | |
| } | |
| throw new PatchError('Property value must be property-compatible data', { | |
| code: 'E_PATCH_INVALID_PROPERTY_VALUE', | |
| }); | |
| } | |
| /** Returns true for scalar property values. */ | |
| function isScalarPatchPropertyValue( | |
| value: unknown, // nosemgrep: ts-no-unknown-outside-adapters -- public PatchBuilder boundary | |
| ): value is string | number | boolean | null { | |
| return value === null | |
| || typeof value === 'string' | |
| || typeof value === 'number' | |
| || typeof value === 'boolean'; | |
| } | |
| /** Returns true for plain recursive property objects. */ | |
| function isPlainPatchPropertyObject( | |
| value: unknown, // nosemgrep: ts-no-unknown-outside-adapters -- public PatchBuilder boundary | |
| ): value is { readonly [key: string]: unknown } { // nosemgrep: ts-no-unknown-outside-adapters -- public PatchBuilder boundary | |
| if (value === null || typeof value !== 'object') { | |
| return false; | |
| } | |
| if (Array.isArray(value) || value instanceof Uint8Array) { | |
| return false; | |
| } | |
| return Object.getPrototypeOf(value) === Object.prototype | |
| || Object.getPrototypeOf(value) === null; | |
| /** Validates public patch property values before intent construction. */ | |
| function requirePatchPropertyValue( | |
| value: unknown, | |
| seen: WeakSet<object> = new WeakSet(), | |
| ): PropValue { // nosemgrep: ts-no-unknown-outside-adapters -- public PatchBuilder boundary | |
| if (isScalarPatchPropertyValue(value)) { | |
| return value; | |
| } | |
| if (value instanceof Uint8Array) { | |
| return value; | |
| } | |
| if (Array.isArray(value)) { | |
| if (seen.has(value)) { | |
| throw new PatchError('Property value must be property-compatible data', { | |
| code: 'E_PATCH_INVALID_PROPERTY_VALUE', | |
| }); | |
| } | |
| seen.add(value); | |
| const normalized = value.map((entry) => requirePatchPropertyValue(entry, seen)); | |
| seen.delete(value); | |
| return normalized; | |
| } | |
| if (isPlainPatchPropertyObject(value)) { | |
| if (seen.has(value)) { | |
| throw new PatchError('Property value must be property-compatible data', { | |
| code: 'E_PATCH_INVALID_PROPERTY_VALUE', | |
| }); | |
| } | |
| seen.add(value); | |
| const record: { [key: string]: PropValue } = Object.create(null) as { [key: string]: PropValue }; | |
| for (const [key, entry] of Object.entries(value)) { | |
| record[key] = requirePatchPropertyValue(entry, seen); | |
| } | |
| seen.delete(value); | |
| return record; | |
| } | |
| throw new PatchError('Property value must be property-compatible data', { | |
| code: 'E_PATCH_INVALID_PROPERTY_VALUE', | |
| }); | |
| } | |
| /** Returns true for scalar property values. */ | |
| function isScalarPatchPropertyValue( | |
| value: unknown, // nosemgrep: ts-no-unknown-outside-adapters -- public PatchBuilder boundary | |
| ): value is string | number | boolean | null { | |
| return value === null | |
| || typeof value === 'string' | |
| || typeof value === 'number' | |
| || typeof value === 'boolean'; | |
| } | |
| /** Returns true for plain recursive property objects. */ | |
| function isPlainPatchPropertyObject( | |
| value: unknown, // nosemgrep: ts-no-unknown-outside-adapters -- public PatchBuilder boundary | |
| ): value is { readonly [key: string]: unknown } { // nosemgrep: ts-no-unknown-outside-adapters -- public PatchBuilder boundary | |
| if (value === null || typeof value !== 'object') { | |
| return false; | |
| } | |
| if (Array.isArray(value) || value instanceof Uint8Array) { | |
| return false; | |
| } | |
| return Object.getPrototypeOf(value) === Object.prototype | |
| || Object.getPrototypeOf(value) === null; | |
| } |
🤖 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 `@src/domain/services/PatchBuilder.ts` around lines 463 - 507,
requirePatchPropertyValue currently can recurse forever on cyclic inputs and can
be polluted via prototype keys; fix by adding cycle detection (pass a WeakSet
seen through recursive calls and throw PatchError with code
'E_PATCH_INVALID_PROPERTY_VALUE' if a non-scalar object/array is already in
seen) and by guarding against prototype-polluting keys when normalizing plain
objects (either reject keys like '__proto__' and 'constructor' with PatchError
or create records with Object.create(null) and still reject any key named
'__proto__' or 'constructor'); update requirePatchPropertyValue to accept an
internal seen: WeakSet<object> param, mark objects/arrays before recursing, and
keep isPlainPatchPropertyObject as-is for detection but ensure the object
key-check happens in requirePatchPropertyValue when building record/array to
deterministically fail on cycles and prototype-key pollution.
Comment on lines
+16
to
+63
| describe('PatchBuilder property intent lowering', () => { | ||
| it('lowers node property intents to the current PropSet shape', () => { | ||
| const builder = createBuilder(null); | ||
|
|
||
| builder.setProperty('node:1', 'status', 'ready'); | ||
|
|
||
| const patch = builder.build(); | ||
| expect(patch.ops).toHaveLength(1); | ||
| const op = requirePropSet(patch.ops[0]); | ||
| expect(op.node).toBe('node:1'); | ||
| expect(op.key).toBe('status'); | ||
| expect(op.value).toBe('ready'); | ||
| expect(patch.schema).toBe(2); | ||
| }); | ||
|
|
||
| it('lowers edge property intents to the current legacy edge PropSet shape', () => { | ||
| const state = stateWithEdge(); | ||
| const builder = createBuilder(state); | ||
|
|
||
| builder.setEdgeProperty('node:1', 'node:2', 'rel', 'weight', 3); | ||
|
|
||
| const patch = builder.build(); | ||
| expect(patch.ops).toHaveLength(1); | ||
| const op = requirePropSet(patch.ops[0]); | ||
| expect(op.node).toBe(encodeLegacyEdgePropNode('node:1', 'node:2', 'rel')); | ||
| expect(op.key).toBe('weight'); | ||
| expect(op.value).toBe(3); | ||
| expect(patch.schema).toBe(3); | ||
| }); | ||
|
|
||
| it('rejects invalid node property values before appending operations', () => { | ||
| const builder = createBuilder(null); | ||
|
|
||
| expect(() => { | ||
| builder.setProperty('node:1', 'status', new InvalidPropertyCarrier()); | ||
| }).toThrow(PatchError); | ||
| expect(builder.build().ops).toEqual([]); | ||
| }); | ||
|
|
||
| it('rejects malformed edge targets before appending operations', () => { | ||
| const builder = createBuilder(stateWithEdge()); | ||
|
|
||
| expect(() => { | ||
| builder.setEdgeProperty('', 'node:2', 'rel', 'weight', 3); | ||
| }).toThrow(/NodeId/); | ||
| expect(builder.build().ops).toEqual([]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add tests for the remaining property-value normalization branches.
This suite doesn’t explicitly cover accepted Uint8Array, recursive array/object values, or cyclic-value rejection paths introduced by property normalization. Add focused tests so this refactor slice is fully covered.
🧪 Suggested test additions
describe('PatchBuilder property intent lowering', () => {
+ it('accepts recursive property-compatible values (bytes, arrays, objects)', () => {
+ const builder = createBuilder(null);
+ const bytes = new Uint8Array([1, 2, 3]);
+ builder.setProperty('node:1', 'payload', { bytes, nested: [1, 'ok', { done: true }] });
+
+ const patch = builder.build();
+ const op = requirePropSet(patch.ops[0]);
+ expect(op.key).toBe('payload');
+ expect(op.value).toEqual({ bytes, nested: [1, 'ok', { done: true }] });
+ });
+
+ it('rejects cyclic property values before appending operations', () => {
+ const builder = createBuilder(null);
+ const cyclic: Record<string, unknown> = {};
+ cyclic.self = cyclic;
+
+ expect(() => {
+ builder.setProperty('node:1', 'payload', cyclic);
+ }).toThrow(PatchError);
+ expect(builder.build().ops).toEqual([]);
+ });As per coding guidelines, **/*.test.{ts,tsx}: Touched code in refactor slices must reach 100% test coverage before the slice is considered done.
📝 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.
Suggested change
| describe('PatchBuilder property intent lowering', () => { | |
| it('lowers node property intents to the current PropSet shape', () => { | |
| const builder = createBuilder(null); | |
| builder.setProperty('node:1', 'status', 'ready'); | |
| const patch = builder.build(); | |
| expect(patch.ops).toHaveLength(1); | |
| const op = requirePropSet(patch.ops[0]); | |
| expect(op.node).toBe('node:1'); | |
| expect(op.key).toBe('status'); | |
| expect(op.value).toBe('ready'); | |
| expect(patch.schema).toBe(2); | |
| }); | |
| it('lowers edge property intents to the current legacy edge PropSet shape', () => { | |
| const state = stateWithEdge(); | |
| const builder = createBuilder(state); | |
| builder.setEdgeProperty('node:1', 'node:2', 'rel', 'weight', 3); | |
| const patch = builder.build(); | |
| expect(patch.ops).toHaveLength(1); | |
| const op = requirePropSet(patch.ops[0]); | |
| expect(op.node).toBe(encodeLegacyEdgePropNode('node:1', 'node:2', 'rel')); | |
| expect(op.key).toBe('weight'); | |
| expect(op.value).toBe(3); | |
| expect(patch.schema).toBe(3); | |
| }); | |
| it('rejects invalid node property values before appending operations', () => { | |
| const builder = createBuilder(null); | |
| expect(() => { | |
| builder.setProperty('node:1', 'status', new InvalidPropertyCarrier()); | |
| }).toThrow(PatchError); | |
| expect(builder.build().ops).toEqual([]); | |
| }); | |
| it('rejects malformed edge targets before appending operations', () => { | |
| const builder = createBuilder(stateWithEdge()); | |
| expect(() => { | |
| builder.setEdgeProperty('', 'node:2', 'rel', 'weight', 3); | |
| }).toThrow(/NodeId/); | |
| expect(builder.build().ops).toEqual([]); | |
| }); | |
| }); | |
| describe('PatchBuilder property intent lowering', () => { | |
| it('lowers node property intents to the current PropSet shape', () => { | |
| const builder = createBuilder(null); | |
| builder.setProperty('node:1', 'status', 'ready'); | |
| const patch = builder.build(); | |
| expect(patch.ops).toHaveLength(1); | |
| const op = requirePropSet(patch.ops[0]); | |
| expect(op.node).toBe('node:1'); | |
| expect(op.key).toBe('status'); | |
| expect(op.value).toBe('ready'); | |
| expect(patch.schema).toBe(2); | |
| }); | |
| it('lowers edge property intents to the current legacy edge PropSet shape', () => { | |
| const state = stateWithEdge(); | |
| const builder = createBuilder(state); | |
| builder.setEdgeProperty('node:1', 'node:2', 'rel', 'weight', 3); | |
| const patch = builder.build(); | |
| expect(patch.ops).toHaveLength(1); | |
| const op = requirePropSet(patch.ops[0]); | |
| expect(op.node).toBe(encodeLegacyEdgePropNode('node:1', 'node:2', 'rel')); | |
| expect(op.key).toBe('weight'); | |
| expect(op.value).toBe(3); | |
| expect(patch.schema).toBe(3); | |
| }); | |
| it('rejects invalid node property values before appending operations', () => { | |
| const builder = createBuilder(null); | |
| expect(() => { | |
| builder.setProperty('node:1', 'status', new InvalidPropertyCarrier()); | |
| }).toThrow(PatchError); | |
| expect(builder.build().ops).toEqual([]); | |
| }); | |
| it('rejects malformed edge targets before appending operations', () => { | |
| const builder = createBuilder(stateWithEdge()); | |
| expect(() => { | |
| builder.setEdgeProperty('', 'node:2', 'rel', 'weight', 3); | |
| }).toThrow(/NodeId/); | |
| expect(builder.build().ops).toEqual([]); | |
| }); | |
| it('accepts recursive property-compatible values (bytes, arrays, objects)', () => { | |
| const builder = createBuilder(null); | |
| const bytes = new Uint8Array([1, 2, 3]); | |
| builder.setProperty('node:1', 'payload', { bytes, nested: [1, 'ok', { done: true }] }); | |
| const patch = builder.build(); | |
| const op = requirePropSet(patch.ops[0]); | |
| expect(op.key).toBe('payload'); | |
| expect(op.value).toEqual({ bytes, nested: [1, 'ok', { done: true }] }); | |
| }); | |
| it('rejects cyclic property values before appending operations', () => { | |
| const builder = createBuilder(null); | |
| const cyclic: Record<string, unknown> = {}; | |
| cyclic.self = cyclic; | |
| expect(() => { | |
| builder.setProperty('node:1', 'payload', cyclic); | |
| }).toThrow(PatchError); | |
| expect(builder.build().ops).toEqual([]); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unit/domain/services/PatchBuilderPropertyIntent.test.ts` around lines 16
- 63, Add unit tests in PatchBuilderPropertyIntent.test.ts that exercise the
remaining property normalization branches: add a test that calls
createBuilder(...) and builder.setProperty('node:1','bin', new
Uint8Array([1,2,3])) then build() and assert the op value accepts Uint8Array and
patch.schema is correct; add tests that pass nested/recursive structures via
builder.setProperty (e.g., arrays and objects) to ensure they are normalized and
appended to ops by build(); and add a test that supplies a cyclic value to
builder.setProperty and asserts it throws PatchError (or the same cyclic
rejection path) and that builder.build().ops remains empty. Reference the
existing helpers and symbols used in the file (createBuilder,
builder.setProperty, setEdgeProperty, InvalidPropertyCarrier, PatchError,
requirePropSet, encodeLegacyEdgePropNode) so tests align with current test
patterns.
Comment on lines
+26
to
+81
| describe('StateReader property projection routing', () => { | ||
| it('reads node and edge property bags through projection records', async () => { | ||
| const state = WarpState.empty(); | ||
| addLiveNode(state, 'node:1', 1); | ||
| addLiveNode(state, 'node:2', 2); | ||
| addLiveEdge(state, 'node:1', 'node:2', 'rel', 3); | ||
| state.prop.set(encodePropKey('node:1', 'status'), register(4, 'ready')); | ||
| state.prop.set('node:1\0bad\0extra', register(5, 'ignored')); | ||
| state.prop.set(encodeEdgePropKey('node:1', 'node:2', 'rel', 'weight'), register(6, 3)); | ||
| state.prop.set(`${EDGE_PROP_PREFIX}node:1\0node:2\0rel\0bad\0extra`, register(7, 'ignored')); | ||
|
|
||
| const reader = createStateReader(state); | ||
| const host = hostForState(state); | ||
|
|
||
| expect(reader.getNodeProps('node:1')).toEqual({ status: 'ready' }); | ||
| expect(reader.getEdgeProps('node:1', 'node:2', 'rel')).toEqual({ weight: 3 }); | ||
| expect(reader.getEdges()).toEqual([ | ||
| { from: 'node:1', to: 'node:2', label: 'rel', props: { weight: 3 } }, | ||
| ]); | ||
| expect(reader.project().props).toEqual([ | ||
| { node: 'node:1', key: 'status', value: 'ready' }, | ||
| ]); | ||
| await expect(getNodePropsImpl(host, 'node:1')).resolves.toEqual(reader.getNodeProps('node:1')); | ||
| await expect(getEdgePropsImpl(host, { | ||
| from: 'node:1', | ||
| to: 'node:2', | ||
| label: 'rel', | ||
| })).resolves.toEqual(reader.getEdgeProps('node:1', 'node:2', 'rel')); | ||
| }); | ||
|
|
||
| it('reads content metadata through the content attachment projection', () => { | ||
| const state = WarpState.empty(); | ||
| addLiveNode(state, 'node:1', 1); | ||
| addLiveNode(state, 'node:2', 2); | ||
| addLiveEdge(state, 'node:1', 'node:2', 'rel', 3); | ||
| state.prop.set(encodePropKey('node:1', CONTENT_PROPERTY_KEY), register(4, 'node-oid')); | ||
| state.prop.set(encodePropKey('node:1', CONTENT_MIME_PROPERTY_KEY), register(5, 'ignored/old')); | ||
| state.prop.set(encodePropKey('node:1', CONTENT_SIZE_PROPERTY_KEY), register(4, 512)); | ||
| state.prop.set(encodeEdgePropKey('node:1', 'node:2', 'rel', CONTENT_PROPERTY_KEY), register(6, 'edge-oid')); | ||
| state.prop.set(encodeEdgePropKey('node:1', 'node:2', 'rel', CONTENT_MIME_PROPERTY_KEY), register(6, 'text/plain')); | ||
| state.prop.set(encodeEdgePropKey('node:1', 'node:2', 'rel', CONTENT_SIZE_PROPERTY_KEY), register(7, 999)); | ||
|
|
||
| const reader = createStateReader(state); | ||
|
|
||
| expect(reader.getNodeContentMeta('node:1')).toEqual({ | ||
| oid: 'node-oid', | ||
| mime: null, | ||
| size: 512, | ||
| }); | ||
| expect(reader.getEdgeContentMeta('node:1', 'node:2', 'rel')).toEqual({ | ||
| oid: 'edge-oid', | ||
| mime: 'text/plain', | ||
| size: null, | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add coverage for the SnapshotWarpState reader path.
These cases only exercise the live WarpState branch. The new StateReaderSource / warpStateFromSnapshot path in src/domain/services/state/StateReaderContext.ts is still untested, so the main behavior added by this slice is missing direct assertions. Please add at least one parity case that feeds createStateReader a SnapshotWarpState and expects the same props/content metadata as the live-state reader.
As per coding guidelines "Touched code in refactor slices must reach 100% test coverage before the slice is considered done".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unit/domain/services/StateReaderPropertyProjection.test.ts` around lines
26 - 81, Add a parallel test that constructs a SnapshotWarpState via
warpStateFromSnapshot (or by calling StateReaderSource.snapshot path) containing
the same nodes/edges/prop entries as the existing WarpState test, then call
createStateReader with that SnapshotWarpState and assert the same results for
getNodeProps, getEdgeProps, getEdges, project().props and
getNodeContentMeta/getEdgeContentMeta as in the live-state cases; locate where
the existing tests create the live WarpState (functions addLiveNode,
addLiveEdge, encodePropKey, encodeEdgePropKey) and replicate those setup steps
but convert the final state into a SnapshotWarpState using warpStateFromSnapshot
(or the StateReaderSource API) before creating the reader so the snapshot code
path (SnapshotWarpState/StateReaderSource) is exercised and assertions remain
identical.
Member
Author
|
@codex self-review findings for confirmation.
|
Member
Author
|
Resolved the self-review findings in
Local verification before push:
|
Release Preflight
If you tag this commit as |
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 free
to 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.
Summary
StateQueryReadModeland property-count accounting.PatchBuilderproperty writes through those intents while preserving the existing wire shape and reserved-byte validation behavior.Verification
npm run typechecknpm run lintnpm run lint:sludgenpm run lint:quarantine-graduatenpm run test:local(488 files, 6956 tests)npx markdownlint-cli2 CHANGELOG.md docs/BEARING.md docs/method/backlog/v18.0.0/PROTO_legacy-props-as-projection.md docs/design/0183-v18-property-projection-closeout/v18-property-projection-closeout.mdgit diff --checkNotes
A follow-up commit preserves the consumer type surface after widening
createStateReaderto accept immutableSnapshotWarpStatesources in addition to liveWarpStatesources.Summary by CodeRabbit
Release Notes