Repository navigation
fix: harden utils/set path semantics and query conflict skip (#353) - #378
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📜 Recent review details🧰 Additional context used📓 Path-based instructions (4)**/*.js📄 CodeRabbit inference engine (Custom checks)
Files:
{lib,http}/**/*.js📄 CodeRabbit inference engine (Custom checks)
Files:
lib/**⚙️ CodeRabbit configuration file
Files:
utils/**⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (5)📚 Learning: 2026-06-07T00:21:31.900ZApplied to files:
📚 Learning: 2026-06-10T14:01:32.908ZApplied to files:
📚 Learning: 2026-06-27T16:53:37.400ZApplied to files:
📚 Learning: 2026-07-03T04:05:13.458ZApplied to files:
📚 Learning: 2026-07-01T20:00:40.539ZApplied to files:
🔀 Multi-repo context CentralPing/ergo-routerLinked repositories findingsCentralPing/ergo-router
🔇 Additional comments (5)
Summary by CodeRabbit
WalkthroughThe setter now validates path segments and intermediates, reports coded traversal conflicts, protects exotic ChangesPath traversal handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant QueryParser
participant Accumulator
participant trySet
participant set
QueryParser->>Accumulator: parse source-ordered query pairs
Accumulator->>Accumulator: check existing paths and shape conflicts
Accumulator->>trySet: attempt first-wins assignment
trySet->>set: validate path and intermediates
set-->>trySet: success or coded traversal conflict
trySet-->>Accumulator: true or false
Accumulator-->>QueryParser: return nested accumulator
Possibly related issues
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (7 passed)
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@utils/set.js`:
- Around line 46-47: Update the intermediate-value guard in set so function
values are treated as traversable objects rather than invalid primitives.
Preserve the existing null and primitive rejection behavior, while allowing
assignments through function intermediates such as handler.timeout.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b4e4a189-f89d-47bb-9975-c875d34e4504
📒 Files selected for processing (5)
CHANGELOG.mdlib/query.jslib/query.spec.unit.jsutils/set.jsutils/set.spec.unit.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
CentralPing/ergo-router(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: analyze
- GitHub Check: test (22)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.js
📄 CodeRabbit inference engine (Custom checks)
Changed JavaScript source files (excluding tests and benchmarks) must have complete JSDoc: new exported functions need both
@paramand@returns; use lowercase primitive types (string,number,boolean) even in compound types; use@returnsinstead of@return; and reference Node.js built-in types viaimport('node:...')(for example,import('node:stream').Readable).
Files:
lib/query.spec.unit.jsutils/set.spec.unit.jslib/query.jsutils/set.js
{lib,http}/**/*.js
📄 CodeRabbit inference engine (Custom checks)
In changed JavaScript files under
lib/andhttp/, any new code that creates objects from user-input-derived data (such as query parameters, cookie values, header values, or parsed body fields) must useObject.create(null)instead of{}ornew Object()to prevent prototype pollution.
Files:
lib/query.spec.unit.jslib/query.js
lib/**
⚙️ CodeRabbit configuration file
lib/**: Shared primitives — pure functions with no transport or framework dependencies. Must use null-prototype objects (Object.create(null)) for user-input-derived data. Use undefined (not null) for absent values. No HTTP or framework imports.
Files:
lib/query.spec.unit.jslib/query.js
utils/**
⚙️ CodeRabbit configuration file
utils/**: Low-level utilities. get.js defaults invoke to false — callers must opt in explicitly. No mutation of req objects. Prefer ?? over || for nullish defaults.
Files:
utils/set.spec.unit.jsutils/set.js
🧠 Learnings (5)
📚 Learning: 2026-06-07T00:21:31.900Z
Learnt from: JasonCust
Repo: CentralPing/ergo PR: 129
File: lib/response-time.js:44-45
Timestamp: 2026-06-07T00:21:31.900Z
Learning: In CentralPing/ergo’s `lib` code, treat developer-provided factory configuration options (e.g., options like `precision` in `applyResponseTiming`/`timing`, `ms` in `timeout`, `limit` in `body`) as fail-fast inputs: do not clamp/coerce values into range and do not silently default when they’re out-of-range or the wrong type. If an invalid value would break calculations (e.g., `precision: 'banana'` leading to a `RangeError` in `toFixed()`), it should throw immediately to surface the bug rather than being masked by a fallback. During review, avoid suggesting defensive clamping or silent defaults for these developer-controlled options.
Applied to files:
lib/query.spec.unit.jslib/query.js
📚 Learning: 2026-06-10T14:01:32.908Z
Learnt from: JasonCust
Repo: CentralPing/ergo PR: 145
File: lib/response-info.js:24-31
Timestamp: 2026-06-10T14:01:32.908Z
Learning: In CentralPing/ergo, only enforce the null-prototype policy (`Object.create(null)`) when attacker-controlled user input can determine the *object’s keys* (e.g., query-string parsing where parameters like `__proto__` could become keys, or cookie/header parsing where field names come from user input). If the object’s key set is fixed by developer-defined string literals (e.g., `{statusCode, headers, method, url, bodySize, duration}`), then it is acceptable even if the *values* come from user input (e.g., `req.url`, `req.method`), because prototype-pollution via user-controlled keys is not possible. During review, don’t flag fixed-key result objects in `lib/**` as null-prototype violations when user input affects only values, not keys (the sibling precedent is `lib/paginate.js` returning a fixed-key object without `Object.create(null)` while using user-derived values).
Applied to files:
lib/query.spec.unit.jslib/query.js
📚 Learning: 2026-06-27T16:53:37.400Z
Learnt from: JasonCust
Repo: CentralPing/ergo PR: 194
File: lib/response-info.js:22-28
Timestamp: 2026-06-27T16:53:37.400Z
Learning: In CentralPing/ergo, runtime type validation for developer-facing options should be done at the http layer factory boundaries (e.g., via a validateOptions helper under `http/**`), not inside shared pure helpers under `lib/**`. For `lib/**` modules like `lib/response-info.js`, assume callers such as `http/handler.js` and `http/logger.js` have already validated inputs; avoid adding redundant runtime guards (e.g., `instanceof Set` checks) inside `lib` helpers and keep them consistent with other `lib` modules (e.g., `lib/response-time.js`, `lib/attach-instance.js`, `lib/vary.js`, `lib/paginate.js`).
Applied to files:
lib/query.spec.unit.jslib/query.js
📚 Learning: 2026-07-03T04:05:13.458Z
Learnt from: JasonCust
Repo: CentralPing/ergo PR: 233
File: lib/idempotency.js:165-174
Timestamp: 2026-07-03T04:05:13.458Z
Learning: In CentralPing/ergo, shared primitives under `lib/**` should follow the fail-fast convention from `DECISIONS.md`: perform runtime input validation at the `http` layer boundary, not inside `lib` modules. In `lib/idempotency.js` (e.g., `IdempotencyStore.complete(key, response, generation)`), honor the documented contract instead of adding extra runtime guards—`response` is expected to be an object per JSDoc, and the implementation intentionally only applies a nullish check (do not add `typeof response === 'object'` validation); non-object `response` should be treated as developer misuse.
Applied to files:
lib/query.spec.unit.jslib/query.js
📚 Learning: 2026-07-01T20:00:40.539Z
Learnt from: JasonCust
Repo: CentralPing/ergo PR: 229
File: lib/idempotency.spec.unit.js:180-183
Timestamp: 2026-07-01T20:00:40.539Z
Learning: Follow CentralPing/ergo’s comment convention in .js files: add comments only for non-obvious intent, trade-offs, or constraints. Avoid comments that merely restate what the code/assertions/constants literally compute (e.g., explaining a hardcoded SHA-256 digest used by a test). Prefer making code/self-documenting via descriptive test names and tracing to the implementation (e.g., `body ?? ''` + `createHash('sha256')`) rather than redundant `// sha256(...)`-style comments.
Applied to files:
lib/query.spec.unit.jsutils/set.spec.unit.jslib/query.jsutils/set.js
🔇 Additional comments (4)
utils/set.spec.unit.js (1)
6-6: LGTM!Also applies to: 62-131
lib/query.js (1)
26-26: LGTM!Also applies to: 108-111
lib/query.spec.unit.js (1)
155-160: LGTM!Also applies to: 162-167
CHANGELOG.md (1)
7-24: LGTM!
Functions are objects in JS; reject only null and non-object primitives so assignments like handler.timeout remain valid. Addresses CodeRabbit review.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.15.
| Benchmark suite | Current: 4ebd797 | Previous: 6032f6c | Ratio |
|---|---|---|---|
compose: authorization (bearer) |
0.007 us/op |
0.006 us/op |
1.17 |
compose: full pipeline (negotiate + auth + execute) |
0.014 us/op |
0.012 us/op |
1.17 |
This comment was automatically generated by workflow using github-action-benchmark.
|
@coderabbitai review |
✅ Action performedReview finished.
|
JasonCust
left a comment
JasonCust
left a comment
There was a problem hiding this comment.
Lead Architect Review
Verdict: APPROVE
(GitHub prevents self-approve on this account; verdict is APPROVE in this review body.)
Pre-existing CodeRabbit feedback (function intermediates) was already addressed; branch is MERGEABLE with no conflicts. Fresh review finds the #353/#354/#355 design sound: strict /^\d+$/ index detection, coded path-conflict throw + trySet first-wins for the documented 500 path, and non-breaking return val. Quality gates pass locally and on CI.
Make query path-conflict first-wins bidirectional (#379), strengthen conflict/test oracles, and document function intermediates in set JSDoc.
|
@coderabbitai review |
✅ Action performedReview finished.
|
JasonCust
left a comment
JasonCust
left a comment
There was a problem hiding this comment.
Lead Architect Review
Verdict: REQUEST CHANGES
(GitHub prevents self-REQUEST_CHANGES on this account; verdict is REQUEST CHANGES in this review body.)
Fix cycle for prior findings completed (146a8e5); no merge conflicts. Fresh review finds the top-level bidirectional first-wins (#379) incomplete for nested/dotted path conflicts — claimed invariant does not hold (see #380).
- Blocking items: 1
- Non-blocking items: 2
Automated reviewers: INCOMPLETE — CodeRabbit has not reviewed head 146a8e5 (incremental skipped; @coderabbitai review reported finished without a matching-head review). Re-run Check 3b after a successful CR review of the current head before approving.
Fix blocking item and push. Use cping-lead-architect-fix-review for guided resolution.
Skip container overwrite at any dotted path and nesting under empty-bracket arrays; strengthen conflict test oracles.
|
@coderabbitai review |
✅ Action performedReview finished.
|
JasonCust
left a comment
JasonCust
left a comment
There was a problem hiding this comment.
Lead Architect Review
Verdict: REQUEST CHANGES
(GitHub prevents self-REQUEST_CHANGES on this account; verdict is REQUEST CHANGES in this review body.)
Prior findings (#380) were fixed in 5f36b60, but nestsUnderArray over-corrects: subsequent numeric bracket indices under an Array are dropped (role[0]=user&role[1]=admin → only user). See #381.
- Blocking items: 1
- Non-blocking items: 2
Automated reviewers: INCOMPLETE — CodeRabbit has not reviewed head 5f36b60 (incremental skipped; review command finished without a matching-head review). Re-run Check 3b before approving.
Fix blocking item and push. Use cping-lead-architect-fix-review for guided resolution.
Skip only non-index nests under Arrays; keep scalar↔[] first-wins and strengthen conflict test oracles.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 10 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
✅ Action performedReview finished.
|
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 `@utils/set.js`:
- Line 523: Update the Arguments-object detection at the leaf check to use the
non-spoofable types.isArgumentsObject() helper instead of
Object.prototype.toString.call(leaf). Add regression coverage for
Symbol.toStringTag spoofing and getters that throw during set(..., 'length',
...), ensuring these cases do not trigger the observable toString path.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fc591816-938c-4e11-814e-9a97ecb5854c
📒 Files selected for processing (5)
CHANGELOG.mdlib/query.jslib/query.spec.unit.jsutils/set.jsutils/set.spec.unit.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
CentralPing/ergo-router(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: test (22)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.js
📄 CodeRabbit inference engine (Custom checks)
Changed JavaScript source files (excluding tests and benchmarks) must have complete JSDoc: new exported functions need both
@paramand@returns; use lowercase primitive types (string,number,boolean) even in compound types; use@returnsinstead of@return; and reference Node.js built-in types viaimport('node:...')(for example,import('node:stream').Readable).
Files:
lib/query.spec.unit.jslib/query.jsutils/set.jsutils/set.spec.unit.js
{lib,http}/**/*.js
📄 CodeRabbit inference engine (Custom checks)
In changed JavaScript files under
lib/andhttp/, any new code that creates objects from user-input-derived data (such as query parameters, cookie values, header values, or parsed body fields) must useObject.create(null)instead of{}ornew Object()to prevent prototype pollution.
Files:
lib/query.spec.unit.jslib/query.js
lib/**
⚙️ CodeRabbit configuration file
lib/**: Shared primitives — pure functions with no transport or framework dependencies. Must use null-prototype objects (Object.create(null)) for user-input-derived data. Use undefined (not null) for absent values. No HTTP or framework imports.
Files:
lib/query.spec.unit.jslib/query.js
utils/**
⚙️ CodeRabbit configuration file
utils/**: Low-level utilities. get.js defaults invoke to false — callers must opt in explicitly. No mutation of req objects. Prefer ?? over || for nullish defaults.
Files:
utils/set.jsutils/set.spec.unit.js
🧠 Learnings (5)
📚 Learning: 2026-06-07T00:21:31.900Z
Learnt from: JasonCust
Repo: CentralPing/ergo PR: 129
File: lib/response-time.js:44-45
Timestamp: 2026-06-07T00:21:31.900Z
Learning: In CentralPing/ergo’s `lib` code, treat developer-provided factory configuration options (e.g., options like `precision` in `applyResponseTiming`/`timing`, `ms` in `timeout`, `limit` in `body`) as fail-fast inputs: do not clamp/coerce values into range and do not silently default when they’re out-of-range or the wrong type. If an invalid value would break calculations (e.g., `precision: 'banana'` leading to a `RangeError` in `toFixed()`), it should throw immediately to surface the bug rather than being masked by a fallback. During review, avoid suggesting defensive clamping or silent defaults for these developer-controlled options.
Applied to files:
lib/query.spec.unit.jslib/query.js
📚 Learning: 2026-06-10T14:01:32.908Z
Learnt from: JasonCust
Repo: CentralPing/ergo PR: 145
File: lib/response-info.js:24-31
Timestamp: 2026-06-10T14:01:32.908Z
Learning: In CentralPing/ergo, only enforce the null-prototype policy (`Object.create(null)`) when attacker-controlled user input can determine the *object’s keys* (e.g., query-string parsing where parameters like `__proto__` could become keys, or cookie/header parsing where field names come from user input). If the object’s key set is fixed by developer-defined string literals (e.g., `{statusCode, headers, method, url, bodySize, duration}`), then it is acceptable even if the *values* come from user input (e.g., `req.url`, `req.method`), because prototype-pollution via user-controlled keys is not possible. During review, don’t flag fixed-key result objects in `lib/**` as null-prototype violations when user input affects only values, not keys (the sibling precedent is `lib/paginate.js` returning a fixed-key object without `Object.create(null)` while using user-derived values).
Applied to files:
lib/query.spec.unit.jslib/query.js
📚 Learning: 2026-06-27T16:53:37.400Z
Learnt from: JasonCust
Repo: CentralPing/ergo PR: 194
File: lib/response-info.js:22-28
Timestamp: 2026-06-27T16:53:37.400Z
Learning: In CentralPing/ergo, runtime type validation for developer-facing options should be done at the http layer factory boundaries (e.g., via a validateOptions helper under `http/**`), not inside shared pure helpers under `lib/**`. For `lib/**` modules like `lib/response-info.js`, assume callers such as `http/handler.js` and `http/logger.js` have already validated inputs; avoid adding redundant runtime guards (e.g., `instanceof Set` checks) inside `lib` helpers and keep them consistent with other `lib` modules (e.g., `lib/response-time.js`, `lib/attach-instance.js`, `lib/vary.js`, `lib/paginate.js`).
Applied to files:
lib/query.spec.unit.jslib/query.js
📚 Learning: 2026-07-03T04:05:13.458Z
Learnt from: JasonCust
Repo: CentralPing/ergo PR: 233
File: lib/idempotency.js:165-174
Timestamp: 2026-07-03T04:05:13.458Z
Learning: In CentralPing/ergo, shared primitives under `lib/**` should follow the fail-fast convention from `DECISIONS.md`: perform runtime input validation at the `http` layer boundary, not inside `lib` modules. In `lib/idempotency.js` (e.g., `IdempotencyStore.complete(key, response, generation)`), honor the documented contract instead of adding extra runtime guards—`response` is expected to be an object per JSDoc, and the implementation intentionally only applies a nullish check (do not add `typeof response === 'object'` validation); non-object `response` should be treated as developer misuse.
Applied to files:
lib/query.spec.unit.jslib/query.js
📚 Learning: 2026-07-01T20:00:40.539Z
Learnt from: JasonCust
Repo: CentralPing/ergo PR: 229
File: lib/idempotency.spec.unit.js:180-183
Timestamp: 2026-07-01T20:00:40.539Z
Learning: Follow CentralPing/ergo’s comment convention in .js files: add comments only for non-obvious intent, trade-offs, or constraints. Avoid comments that merely restate what the code/assertions/constants literally compute (e.g., explaining a hardcoded SHA-256 digest used by a test). Prefer making code/self-documenting via descriptive test names and tracing to the implementation (e.g., `body ?? ''` + `createHash('sha256')`) rather than redundant `// sha256(...)`-style comments.
Applied to files:
lib/query.spec.unit.jslib/query.jsutils/set.jsutils/set.spec.unit.js
🔀 Multi-repo context CentralPing/ergo-router
Linked repositories findings
CentralPing/ergo-router
- No direct references to
utils/set,trySet, or the new query-parser exports were found.[::CentralPing/ergo-router::] - The router consumes parsed query data through its existing URL/query interface; no changed setter or parser API is part of its integration surface.
[::CentralPing/ergo-router::] - Existing Ergo dependency constraints remain
>=0.8.0 <0.9.0, so the changes do not require a dependency-range update.[::CentralPing/ergo-router::]
🔇 Additional comments (5)
CHANGELOG.md (1)
9-29: LGTM!Also applies to: 31-40, 42-59, 61-67
utils/set.js (1)
21-22: LGTM!Also applies to: 198-214, 216-243, 300-315, 471-505, 509-522, 536-546
utils/set.spec.unit.js (1)
11-13: LGTM!Also applies to: 19-35, 182-206, 331-331, 342-377, 533-563
lib/query.js (1)
28-40: LGTM!Also applies to: 58-100, 113-133, 178-194
lib/query.spec.unit.js (1)
112-140: LGTM!Also applies to: 186-191
JasonCust
left a comment
JasonCust
left a comment
There was a problem hiding this comment.
Lead Architect Review
Verdict: REQUEST CHANGES
(GitHub prevents self-REQUEST_CHANGES on this account; verdict is REQUEST CHANGES in this review body.)
Fix cycle completed (a3e4182 / ac521f5): Proxy ctor oracle, query own-property options (#392), exotic length forbid (#393). Required CI green; MERGEABLE. CodeRabbit CHANGES_REQUESTED on head ac521f5 — independently corroborated.
forbidsLengthAssignment Arguments detection via toString is bypassable with Symbol.toStringTag (verified length set to 1e6). Use types.isArgumentsObject.
- Blocking items: 1 (
types.isArgumentsObject) - Non-blocking items: 2 (test oracle strengthen;
codecov/patch→ #391) - Deferred still tracked (out of fix scope this cycle): #253 (comma-split/repeat composition)
Companion docs: dot-cursor#29 updated for #392/#393.
Fix blocking item and push. Say fix it for guided resolution.
Use types.isArgumentsObject instead of spoofable toString; strengthen
DataView/arguments oracles and pollution tests with provided {}.
|
@coderabbitai review |
✅ Action performedReview finished.
|
JasonCust
left a comment
JasonCust
left a comment
There was a problem hiding this comment.
Lead Architect Review
Verdict: APPROVE
(Posted as COMMENT — GitHub blocks self-APPROVE on own PRs.)
Prior blocking Arguments-oracle defect is fixed (types.isArgumentsObject). Security scanner clean; required CI green; CodeRabbit APPROVED on 469cb64. Remaining items are non-blocking polish / out-of-scope improvements — not merge blockers.
- Blocking items: 0
- Non-blocking items: 3
- Deferred / tracked: #391 (codecov/patch), #394 (query option value validation), #253 (comma-split + repeated keys — prior)
Falsification on HEAD: Arguments @@toStringTag spoof, TypedArray length, plain-object length, {} pollution caps, Proxy ctor — all PASS.
#353) Align call-boundary errors with the trySet contract and drop the vacuous DataView byteLength post-check.
|
@coderabbitai review |
✅ Action performedReview finished.
|
JasonCust
left a comment
JasonCust
left a comment
There was a problem hiding this comment.
Lead Architect Review
Verdict: APPROVE
(Posted as COMMENT — GitHub blocks self-APPROVE on own PRs.)
Prior non-blocking findings addressed in ace542c (branded null/primitive root + non-string path; DataView length oracle). Required CI green; branch MERGEABLE / CLEAN. Falsification on HEAD: null/primitive roots, non-string path, DataView length — all PASS.
- Blocking items: 0
- Non-blocking items: 1 (Improvement → #395 cross-realm host-graph residual)
- Deferred / tracked: #391 (codecov/patch), #394 (query option values), #395 (cross-realm), #253 (comma-split)
Automated reviewers: GAPS — CodeRabbit last APPROVED 469cb64; full review requested for ace542c but no matching-head review posted yet. No unresolved threads.
Document the identity-based security boundary so callers do not treat foreign-realm host objects as protected containers.
|
@coderabbitai review |
✅ Action performedReview finished.
|
JasonCust
left a comment
JasonCust
left a comment
There was a problem hiding this comment.
Lead Architect Review
Verdict: REQUEST CHANGES
(Posted as COMMENT — GitHub does not allow self-REQUEST_CHANGES on the author's PR.)
The same-realm caller-contract finding is resolved in 6032f6c, required CI is green, the branch is mergeable, and independent security scanning found no additional issues. Fresh falsification found one runtime bypass and two changed-test specification gaps that must be corrected before merge.
- Blocking items: 3
- Non-blocking items: 1 (legacy query test oracles → #396)
- Automated reviewers: INCOMPLETE — CodeRabbit remains in progress and has not posted a formal review tied to
6032f6c codecov/patch: known non-required residual tracked by #391
Fix blocking items and push to this branch. Use cping-lead-architect-fix-review for guided resolution.
TypedArray/Buffer/DataView expose non-own length, so planPath must forbid creating a length container before the leaf guard. Also strengthen null-proto and reverse empty-bracket first-wins regressions.
Fix Cycle CompleteAddressed the three blocking findings from the
Non-blocking legacy query oracles remain tracked as #396 (no in-PR code change). Companion decisions sync: CentralPing/dot-cursor#29 ( |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 37 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 23 minutes. |
JasonCust
left a comment
JasonCust
left a comment
There was a problem hiding this comment.
Lead Architect Review
Verdict: NEEDS DISCUSSION
(Posted as COMMENT — GitHub blocks self-APPROVE/REQUEST_CHANGES on the author's PR. Independent assessment would be APPROVE but Check 3b hard-gates approval.)
Fix cycle for the prior three blocking findings landed in 4ebd797. Branch is MERGEABLE (no conflicts). Required CI green (Node 22/24, Bun, Deno, CodeQL, analyze). Local falsification of the new invariants: PASS.
Blocking
None from independent Phase 1 + Phase 2 scanners.
Automated Reviewers: INCOMPLETE
CodeRabbit commit-status says "Review completed" on head 4ebd797, but the latest formal CodeRabbit review object remains on 469cb647 (APPROVED) — no review with commit_id == 4ebd797 after two @coderabbitai full review requests. Per Check 3b hard gate, overall verdict cannot be APPROVE until a head-matched CodeRabbit review exists. Re-run Check 3b after CR posts a matching-head review.
Non-blocking / tracked
| Topic | Disposition |
|---|---|
Query option value validation (maxLength/maxPairs coercion) |
Already tracked → #394 |
| Legacy weak query test oracles | Already tracked → #396 |
codecov/patch residual |
Non-required; tracked → #391 |
Reuse of a pre-existing own object on TypedArray/Buffer/DataView length |
Caller must already have shadowed exotic length outside set (leaf assign/create are forbidden). Residual defense-in-depth; not merge-blocking |
Checks
| Check | Result |
|---|---|
First-principles (exotic length intermediate) |
PASS — planPath rejects before create; TypedArray/Buffer/DataView falsified |
| First-principles (reverse empty-bracket first-wins) | PASS — a[0]=1&a[]=2 keeps ['1'] |
First-principles (null-proto under {} root) |
PASS |
| Security scanner | No findings |
| Defensive scanner | Option-value issues → #394; pre-shadow reuse → non-blocking residual |
| Test quality scanner | No findings |
| Quality gates | PASS locally + required CI |
| Docs | CHANGELOG/JSDoc updated; companion dot-cursor#29 synced |
No further code changes until you say fix it (or CR catches up and you want a re-check of 3b only).
|
@coderabbitai full review |
✅ Action performedFull review finished. |
1 participant
Summary
utils/setuses strict/^\d+$/array-index detection instead of permissiveNumber()coercion (rejects'',-1,Infinity,0x1,1e2).code: 'ERGO_SET_PATH_TRAVERSE'; newtrySet();lib/query.jsfirst-wins skip soa=42&a[b]=99no longer 500s viaurl().return val(public export — non-breaking).Closes #353
Closes #354
Closes #355
Test plan
utils/set.spec.unit.js— coercion false positives, path-conflict code,trySetsuccess/skip/rethrowlib/query.spec.unit.js—a=42&a[b]=99first-wins;fields[a..b]=xobject chainnpm test(ergo) exit 0 —set.js/query.js100% coveragenpm run check-typesexit 0npm test(ergo-router) against worktree ergo — 822 passCompanion docs
url.mdxconflict note (separate PR in centralping.github.io)dot-cursor/decisions/ergo.mdset/query contract updatesCompletion review
Verdict: PASS — conventions, correctness, docs currency, quality gates, deferred findings (#367/#374 N/A to this changeset).
Closes #384
Closes #385
Closes #386
Closes #387
Closes #388
Closes #389
Closes #392
Closes #393
Closes #395