Conversation
gf_add_field threw "Unknown field type" for any type not in the static 46-key registry, rejecting third-party fields (Gravity Perks, Gravity Wiz, etc.) that Gravity Forms itself accepts. The registry is now an enhancement, not a gate: unknown types are created with graceful degradation plus a warning. Also stop leaking the internal _unknown flag to Gravity Forms on form create/update, and align validateFieldConfig to tolerate unknown types. Convert the orphan field-operations-integration harness to exercise the real field-operations layer. Bump 2.4.0 -> 2.4.1. Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo
|
Warning Review limit reached
More reviews will be available in 3 minutes and 36 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThe PR updates field validation, field creation, dependency handling, utility hardening, integration tests, and release metadata for version 2.4.1. ChangesField handling and validation hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Live-verified against real Gravity Forms (local
Result: ALL PASS. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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 `@src/field-operations/field-manager.js`:
- Around line 35-52: The addField flow in FieldManager still preserves
caller-supplied inputs for unknown types without updating their parent IDs after
generateFieldId assigns the new fieldId. Update addField to rebase any dotted
sub-inputs in the created field (especially for unknown types) so their IDs and
parent references are derived from the newly generated fieldId before saving,
while keeping generateSubInputs behavior for known compound types unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c853d00c-84e2-4b29-9ad0-d853f428860b
📒 Files selected for processing (12)
AGENTS.mdCHANGELOG.mdmcp.jsonpackage.jsonsrc/config/field-validation.jssrc/field-definitions/field-registry.jssrc/field-operations/field-manager.jstest/field-manager.test.jstest/field-operations-integration.test.jstest/field-registry.test.jstest/field-validation.test.jstest/forms.test.js
For unknown/third-party compound fields, gf_add_field preserved caller inputs verbatim, so dotted sub-input ids kept their assumed parent (e.g. 9.1) instead of the id generateFieldId assigns, orphaning the sub-inputs. Rebase dotted ids onto the generated field id (mirrors assignFieldIds); known compound types keep generateSubInputs unchanged. Live-verified on dev.test: 99.x inputs persist as 4.x. Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo
|
Addressed review feedback (sub-input id rebasing):
New unit test catches the mismatch (caller |
…rsistence Adversarial pass (50+ probes) surfaced 15 defects, all fixed test-first: gf_update_field saved a change before reporting it blocked (dependency guard now runs before the write, like deleteField); gf_add_field crashed on null properties/position and accepted empty/null/non-string field_type; gf_create_form fields:[null] threw an opaque TypeError instead of a clean error; null-input crashes in validateFieldConfig/detectFieldVariant/generateCompoundInputs/stripEntryMeta/DependencyTracker; stripEmpty/sanitize could stack-overflow on circular input; buildToolList emitted an undefined entry when gkReloadDef was omitted; validateFieldFilter silently stringified object values. Offline gate green (test:node 377, test:unit 100%, field-validation, publint, lint:docs); adversarial flags 13->3 (remaining 3 are non-bugs); live-verified on dev.test. Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo
Adversarial hardening pass (15 bugs fixed, test-first)Ran an adversarial probe harness (89 probes over two rounds) against the real modules; fixed 15 genuine defects RED→GREEN: Reachable (agent-controlled input):
Defensive (exported helpers crashed on null/cyclic input): Verification: offline gate green (test:node 377, test:unit 100%, field-validation, publint, lint:docs); harness flags 13→3 (the 3 remaining are non-bugs: pathological 100k-deep nesting + two params |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/sanitize.js (1)
46-65: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winShared references are now treated as cycles and lose data.
Because
seenis aWeakSet, any second visit is cut off, even for acyclic graphs that legitimately reuse the same object.{ a: child, b: child }now produces a sanitizedabut collapsesbto{}/[]. If you only want to break recursion, cache source→sanitized clone in aWeakMapand return the cached clone on revisit.Suggested fix
-export function sanitize(obj, seen = new WeakSet()) { +export function sanitize(obj, seen = new WeakMap()) { if (!obj || typeof obj !== 'object') return obj; - if (seen.has(obj)) return Array.isArray(obj) ? [] : {}; - seen.add(obj); + if (seen.has(obj)) return seen.get(obj); if (Array.isArray(obj)) { - return obj.map((v) => sanitize(v, seen)); + const out = []; + seen.set(obj, out); + out.push(...obj.map((v) => sanitize(v, seen))); + return out; } const result = {}; + seen.set(obj, result); for (const [key, value] of Object.entries(obj)) { const keyLower = key.toLowerCase(); const isSensitive = SENSITIVE_KEYS.some(k => keyLower.includes(k));🤖 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/utils/sanitize.js` around lines 46 - 65, The sanitize helper is treating any repeated object reference as a cycle, so shared acyclic objects get collapsed and lose data. Update sanitize to preserve shared references by replacing the current WeakSet-based revisit check with a source-to-clone cache (for example in sanitize and its recursive calls) so a previously seen object returns the same sanitized clone instead of an empty placeholder. Keep the existing recursion guard behavior for true cycles while ensuring repeated references under objects and arrays still serialize consistently.
🧹 Nitpick comments (1)
test/compact.test.js (1)
19-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the recursive edge too, not just the root fields.
This test still passes if
stripEmpty()returns the original object atout.self, which is the current regression. Add an assertion on the recursive branch as well, e.g.out.self.b === undefinedorout.self !== o, so the cycle fix is actually enforced.🤖 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/compact.test.js` around lines 19 - 25, The circular-reference test for stripEmpty only checks the root object fields, so it can still pass if the recursive branch incorrectly points back to the original object. Update the test in stripEmpty to also assert the cyclic path returned from stripEmpty(o) is fixed, using out.self and the stripEmpty symbol, by verifying the recursive branch no longer preserves the original object state (for example, that out.self.b is undefined or that out.self is not the original o).
🤖 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 `@src/field-operations/field-manager.js`:
- Around line 121-132: updateField in field-manager.js is only checking
dependencies.conditionalLogic, which misses other breaking dependencies handled
by deleteField. Update the dependency gate to use
this.dependencyTracker.hasBreakingDependencies(dependencies) in the updateField
flow so the same blocking rules apply consistently, and keep the existing force
behavior and error response structure intact.
In `@src/utils/compact.js`:
- Around line 15-27: The revisit handling in stripEmpty is returning the
original input node for already-seen arrays/objects, which can leak unstripped
values through circular references. Update stripEmpty to use a source-to-clone
cache so repeated visits return the already-built sanitized clone instead of the
source object/array. Keep the fix within stripEmpty and preserve the current
null/empty-string filtering while ensuring self-references and repeated
references resolve to the cleaned result.
---
Outside diff comments:
In `@src/utils/sanitize.js`:
- Around line 46-65: The sanitize helper is treating any repeated object
reference as a cycle, so shared acyclic objects get collapsed and lose data.
Update sanitize to preserve shared references by replacing the current
WeakSet-based revisit check with a source-to-clone cache (for example in
sanitize and its recursive calls) so a previously seen object returns the same
sanitized clone instead of an empty placeholder. Keep the existing recursion
guard behavior for true cycles while ensuring repeated references under objects
and arrays still serialize consistently.
---
Nitpick comments:
In `@test/compact.test.js`:
- Around line 19-25: The circular-reference test for stripEmpty only checks the
root object fields, so it can still pass if the recursive branch incorrectly
points back to the original object. Update the test in stripEmpty to also assert
the cyclic path returned from stripEmpty(o) is fixed, using out.self and the
stripEmpty symbol, by verifying the recursive branch no longer preserves the
original object state (for example, that out.self.b is undefined or that
out.self is not the original o).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9245f6d-5fa5-4d09-bfa4-3e16c4904922
📒 Files selected for processing (17)
CHANGELOG.mdsrc/config/field-validation.jssrc/config/validation.jssrc/field-definitions/field-registry.jssrc/field-operations/field-dependencies.jssrc/field-operations/field-manager.jssrc/field-operations/index.jssrc/server-runtime.jssrc/utils/compact.jssrc/utils/sanitize.jstest/compact.test.jstest/field-dependencies.test.jstest/field-manager.test.jstest/field-registry.test.jstest/sanitize-hardening.test.jstest/server-runtime.test.jstest/validation-hardening.test.js
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/config/field-validation.js
- test/field-registry.test.js
| const dependencies = this.dependencyTracker?.scanFormDependencies(form, fieldId) || {}; | ||
|
|
||
| const hasBreakingDeps = dependencies.conditionalLogic?.length > 0; | ||
|
|
||
| if (hasBreakingDeps && !force) { | ||
| return { | ||
| success: false, | ||
| error: 'Field has dependencies that may be affected', | ||
| field_id: fieldId, | ||
| dependencies, | ||
| suggestion: 'Use force=true to update anyway' | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect relevant file and nearby dependency tracker implementation
git ls-files 'src/field-operations/**' | sed -n '1,120p'
echo '--- field-manager outline ---'
ast-grep outline src/field-operations/field-manager.js --view expanded || true
echo '--- dependency tracker outline ---'
fd -a 'dependency-tracker.js|dependencyTracker.js|*dependency*' src/field-operations src || trueRepository: GravityKit/MCP
Length of output: 1405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant sections with line numbers
wc -l src/field-operations/field-manager.js
cat -n src/field-operations/field-manager.js | sed -n '1,240p'
echo '--- dependency tracker candidates ---'
fd -a 'dependency-tracker.js|dependencyTracker.js|*dependency*' srcRepository: GravityKit/MCP
Length of output: 10784
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect dependency tracker behavior precisely
fd -a 'dependency-tracker.js|dependencyTracker.js|*dependency*' src || true
rg -n "hasBreakingDependencies|scanFormDependencies|conditionalLogic|calculations|mergeTags|merge tag|merge-tag" src/field-operations src || trueRepository: GravityKit/MCP
Length of output: 8964
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant implementation and dependency handling
wc -l src/field-operations/field-manager.js
cat -n src/field-operations/field-manager.js | sed -n '1,240p'
echo '--- dependency tracker files ---'
fd -a 'dependency-tracker.js|dependencyTracker.js|*dependency*' src || true
echo '--- dependency-related symbols ---'
rg -n "hasBreakingDependencies|scanFormDependencies|conditionalLogic|calculations|mergeTags|merge tag|merge-tag" src/field-operations src || trueRepository: GravityKit/MCP
Length of output: 19248
updateField should use the same dependency gate as deleteField.
deleteField treats conditional logic, calculations, and merge tags as breaking dependencies, but updateField only blocks on dependencies.conditionalLogic?.length > 0. That still allows updates that can break calculations or merge-tag references. Reuse this.dependencyTracker.hasBreakingDependencies(dependencies) for consistency, or narrow the comment if conditional logic is the only intended blocker.
🤖 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/field-operations/field-manager.js` around lines 121 - 132, updateField in
field-manager.js is only checking dependencies.conditionalLogic, which misses
other breaking dependencies handled by deleteField. Update the dependency gate
to use this.dependencyTracker.hasBreakingDependencies(dependencies) in the
updateField flow so the same blocking rules apply consistently, and keep the
existing force behavior and error response structure intact.
| export function stripEmpty(obj, seen = new WeakSet()) { | ||
| if (Array.isArray(obj)) { | ||
| return obj.map(stripEmpty); | ||
| if (seen.has(obj)) return obj; | ||
| seen.add(obj); | ||
| return obj.map((v) => stripEmpty(v, seen)); | ||
| } | ||
| if (obj !== null && typeof obj === 'object') { | ||
| if (seen.has(obj)) return obj; | ||
| seen.add(obj); | ||
| const result = {}; | ||
| for (const [key, value] of Object.entries(obj)) { | ||
| if (value === null || value === '') continue; | ||
| result[key] = stripEmpty(value); | ||
| result[key] = stripEmpty(value, seen); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Returning the original node on revisit leaks unstripped data.
On Line 22 (and similarly Line 17), a repeated visit returns the original input object/array, so the output can still contain values this helper was supposed to remove. With a self-reference like { b: '', self: obj }, out.self.b remains '' because self points back to the source object. Use a source→clone cache so revisits return the already-built sanitized clone instead of the input node.
Suggested fix
-export function stripEmpty(obj, seen = new WeakSet()) {
+export function stripEmpty(obj, seen = new WeakMap()) {
if (Array.isArray(obj)) {
- if (seen.has(obj)) return obj;
- seen.add(obj);
- return obj.map((v) => stripEmpty(v, seen));
+ if (seen.has(obj)) return seen.get(obj);
+ const out = [];
+ seen.set(obj, out);
+ out.push(...obj.map((v) => stripEmpty(v, seen)));
+ return out;
}
if (obj !== null && typeof obj === 'object') {
- if (seen.has(obj)) return obj;
- seen.add(obj);
+ if (seen.has(obj)) return seen.get(obj);
const result = {};
+ seen.set(obj, result);
for (const [key, value] of Object.entries(obj)) {
if (value === null || value === '') continue;
result[key] = stripEmpty(value, seen);
}
return result;📝 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.
| export function stripEmpty(obj, seen = new WeakSet()) { | |
| if (Array.isArray(obj)) { | |
| return obj.map(stripEmpty); | |
| if (seen.has(obj)) return obj; | |
| seen.add(obj); | |
| return obj.map((v) => stripEmpty(v, seen)); | |
| } | |
| if (obj !== null && typeof obj === 'object') { | |
| if (seen.has(obj)) return obj; | |
| seen.add(obj); | |
| const result = {}; | |
| for (const [key, value] of Object.entries(obj)) { | |
| if (value === null || value === '') continue; | |
| result[key] = stripEmpty(value); | |
| result[key] = stripEmpty(value, seen); | |
| export function stripEmpty(obj, seen = new WeakMap()) { | |
| if (Array.isArray(obj)) { | |
| if (seen.has(obj)) return seen.get(obj); | |
| const out = []; | |
| seen.set(obj, out); | |
| out.push(...obj.map((v) => stripEmpty(v, seen))); | |
| return out; | |
| } | |
| if (obj !== null && typeof obj === 'object') { | |
| if (seen.has(obj)) return seen.get(obj); | |
| const result = {}; | |
| seen.set(obj, result); | |
| for (const [key, value] of Object.entries(obj)) { | |
| if (value === null || value === '') continue; | |
| result[key] = stripEmpty(value, seen); |
🤖 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/utils/compact.js` around lines 15 - 27, The revisit handling in
stripEmpty is returning the original input node for already-seen arrays/objects,
which can leak unstripped values through circular references. Update stripEmpty
to use a source-to-clone cache so repeated visits return the already-built
sanitized clone instead of the source object/array. Keep the fix within
stripEmpty and preserve the current null/empty-string filtering while ensuring
self-references and repeated references resolve to the cleaned result.
… validation
Five more genuine defects, fixed test-first: isLocalUrl treated remote 127.x domains (e.g. 127.example.com) as local, which could allow Basic auth over plain HTTP; sanitize/sanitizeHeaders never masked Cookie; validateURL rejected dotless hosts (localhost/intranet) for confirmation redirects; assignFieldIds let duplicate explicit field ids survive; formatErrorMessage only replaced the first {field} and interpreted $-sequences in values. Offline gate green (test:node 384, test:unit 100%, auth, field-validation, publint, lint:docs).
Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo
…N_WEBHOOK
Adds an "Announce release to Slack" step after "Create GitHub Release"
that POSTs the gktools {link,version,changelog,product} contract to
SLACK_RELEASE_NOTIFICATION_WEBHOOK, so GravityKit MCP shows up in
#release-announcements like every other GravityKit plugin (which get
this for free via gktools' "Notify Slack" block).
- product = "GravityKit MCP"; version reuses steps.guard.outputs.version;
link = .../releases/tag/v<version>; changelog = the CHANGELOG.md section
for that version (awk between "## [<version>]" and the next "## [").
- jq builds the JSON so changelog newlines/quotes are safely escaped.
- Non-fatal, mirroring gktools: a missing webhook secret skips cleanly,
and a response without "ok":true only warns. The release never fails on
Slack.
- Gated on a new steps.release.outputs.created output so it fires only when
a release was actually created this run (not on an idempotent skip).
Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo
In test mode resolveEnv() remaps GRAVITY_FORMS_TEST_TIMEOUT into this.config, but the GravityFormsClient constructor read the raw config object, so the test-site timeout override was silently ignored (stayed 30000). Read timeout and self-signed-cert settings from this.config like every other setting. Found by reading the constructor; verified test-first. Gate green (test:node 385). Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/field-definitions/field-registry.js (1)
1001-1024: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRebase dotted sub-input IDs even when the explicit field ID is kept.
Line 1002 returns the field unchanged for the first valid explicit
id, so a payload like{ id: 7, inputs: [{ id: '2.3' }] }keeps compound inputs anchored to2instead of the final parent id7. That can still orphan third-party compound inputs even though the field id itself is valid.Proposed fix
+const rebaseInputs = (fieldId, inputs) => inputs.map((input) => { + const hasDottedId = input && typeof input.id === 'string' && input.id.includes('.'); + if (!hasDottedId) { + return input; + } + const sub = input.id.slice(input.id.indexOf('.') + 1); + return { ...input, id: `${fieldId}.${sub}` }; +}); + export function assignFieldIds(fields) { if (!Array.isArray(fields)) { return fields; } @@ return fields.map((field) => { const id = Number(field?.id); const hasFreshExplicitId = Number.isInteger(id) && id > 0 && !claimed.has(id); if (hasFreshExplicitId) { claimed.add(id); - return field; + if (Array.isArray(field?.inputs)) { + return { ...field, inputs: rebaseInputs(id, field.inputs) }; + } + return field; } @@ // Re-base any provided compound sub-input ids onto the new field id. if (Array.isArray(field?.inputs)) { - const inputs = field.inputs.map((input) => { - const hasDottedId = input && typeof input.id === 'string' && input.id.includes('.'); - if (hasDottedId) { - const sub = input.id.slice(input.id.indexOf('.') + 1); - return { ...input, id: `${newId}.${sub}` }; - } - return input; - }); + const inputs = rebaseInputs(newId, field.inputs); return { ...field, id: newId, inputs }; }🤖 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/field-definitions/field-registry.js` around lines 1001 - 1024, The explicit-id fast path in field-registry’s field reassignment logic still leaves dotted compound input IDs attached to the old parent. Update the branch around the fresh explicit id check and the input rebasing logic so that whenever a field with inputs is accepted, its dotted sub-input ids are rewritten to use the final parent id, not just when a new id is allocated. Use the existing field/id handling in the same function to keep the rebase behavior consistent for both explicit and generated ids.
🧹 Nitpick comments (1)
.github/workflows/publish.yml (1)
117-118: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winBind
${{ }}expressions to env vars to avoid template injection into the shell.
steps.guard.outputs.versionis interpolated directly into the script body before bash parses it. Although the value originates from the repo's ownpackage.json, GitHub expands${{ }}textually before execution, so a crafted version string would land inside the shell script. The hardened pattern is to pass these through theenv:block and reference them as quoted shell variables.🔒 Proposed env-binding
env: SLACK_RELEASE_NOTIFICATION_WEBHOOK: ${{ secrets.SLACK_RELEASE_NOTIFICATION_WEBHOOK }} + VERSION: ${{ steps.guard.outputs.version }} + REPOSITORY: ${{ github.repository }} run: | if [ -z "${SLACK_RELEASE_NOTIFICATION_WEBHOOK:-}" ]; then echo "SLACK_RELEASE_NOTIFICATION_WEBHOOK not set — skipping Slack announcement." exit 0 fi - VERSION="${{ steps.guard.outputs.version }}" - LINK="https://github.com/${{ github.repository }}/releases/tag/v$VERSION" + LINK="https://github.com/$REPOSITORY/releases/tag/v$VERSION"🤖 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/publish.yml around lines 117 - 118, The publish workflow script is interpolating the guard version directly into the shell body, which can expose the job to template-to-shell injection. Update the publish step to bind steps.guard.outputs.version through the env block and reference it as a quoted shell variable when building VERSION and LINK, instead of using GitHub expression syntax inline. Keep the fix localized to the publish job/script section and ensure any later use of VERSION/LINK in that step reads from env-backed shell vars.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@src/field-definitions/field-registry.js`:
- Around line 1001-1024: The explicit-id fast path in field-registry’s field
reassignment logic still leaves dotted compound input IDs attached to the old
parent. Update the branch around the fresh explicit id check and the input
rebasing logic so that whenever a field with inputs is accepted, its dotted
sub-input ids are rewritten to use the final parent id, not just when a new id
is allocated. Use the existing field/id handling in the same function to keep
the rebase behavior consistent for both explicit and generated ids.
---
Nitpick comments:
In @.github/workflows/publish.yml:
- Around line 117-118: The publish workflow script is interpolating the guard
version directly into the shell body, which can expose the job to
template-to-shell injection. Update the publish step to bind
steps.guard.outputs.version through the env block and reference it as a quoted
shell variable when building VERSION and LINK, instead of using GitHub
expression syntax inline. Keep the fix localized to the publish job/script
section and ensure any later use of VERSION/LINK in that step reads from
env-backed shell vars.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0db1b32a-a55c-41af-8e9d-89126aa55c27
📒 Files selected for processing (9)
.github/workflows/publish.ymlCHANGELOG.mdsrc/config/auth.jssrc/config/validation-config.jssrc/field-definitions/field-registry.jssrc/utils/sanitize.jstest/field-registry.test.jstest/sanitize-hardening.test.jstest/validation-hardening.test.js
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- test/field-registry.test.js
- src/utils/sanitize.js
…en publish.yml Three more genuine bugs, test-first: (1) assignFieldIds infinite-loops on an out-of-range field id (1e308 etc.) because next++ can't advance past float precision — now only safe integers count as explicit ids (DoS, found via fuzzing). (2) assignFieldIds preserved-explicit-id fast path left dotted sub-inputs attached to a stale parent — now rebases inputs to the final id in both branches via a shared helper. (3) publish.yml Slack step interpolated steps.guard.outputs.version / github.repository directly into the shell body — now bound via env and referenced as quoted shell vars (template-to-shell injection). Gate green (test:node 387, test:unit 100%, publish.yml YAML/actionlint clean). Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo
methodForAbility(null) threw 'Cannot read properties of null (reading readonly)' — the = {} default only covers undefined. Now guarded with optional chaining (returns POST). Surfaced by a fuzz pass over the exported surface. Verified test-first.
Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo
…ge id fixes [ci skip] Reflect the net user-facing 2.4.1 changes since v2.4.0: compound sub-input ids stay consistent with the field id, and out-of-range/duplicate explicit field ids are corrected (an out-of-range id no longer stalls form creation). CI/release tooling (Slack announce), the test-only timeout fix, and the internal methodForAbility guard are intentionally omitted (not user-facing). Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo
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 `@src/gravity-forms-client.js`:
- Line 116: The TLS self-signed certificate check in the Gravity Forms client is
still using string truthiness, so one flag can mask the other. Update the
`rejectUnauthorized` logic in `src/gravity-forms-client.js` to evaluate
`this.config.GRAVITY_FORMS_ALLOW_SELF_SIGNED_CERTS` and
`this.config.MCP_ALLOW_SELF_SIGNED_CERTS` explicitly as booleans/strings, and
allow self-signed certs when either setting is enabled. Use the
`GravityFormsClient` config handling around `rejectUnauthorized` to locate and
fix the conditional.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8dbaca3-39d4-4463-abd0-6f81abe52bf1
📒 Files selected for processing (8)
.github/workflows/publish.ymlCHANGELOG.mdsrc/abilities/loader.jssrc/field-definitions/field-registry.jssrc/gravity-forms-client.jstest/abilities-loader.test.jstest/client-hardening.test.jstest/field-registry.test.js
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/publish.yml
- test/field-registry.test.js
- src/field-definitions/field-registry.js
rejectUnauthorized used (A || B) !== 'true', but 'false' is a truthy string, so GRAVITY_FORMS_ALLOW_SELF_SIGNED_CERTS='false' short-circuited and masked MCP_ALLOW_SELF_SIGNED_CERTS='true' (and vice versa). Now each flag is compared === 'true' separately, in both GravityFormsClient and WordPressClient (same bug). Test-first; gate green (test:node 389). Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo
Drop the internal-marker bullet (not customer-facing) ahead of the 2.4.1 release. Claude-Session: https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo
1 participant
Summary
gf_add_fieldrejected any field type not in the static 46-key registry, throwingUnknown field type. That blocked third-party field types (Gravity Perks, Gravity Wiz, other add-ons) and GravityKit/custom fields that Gravity Forms itself accepts on save. The other paths already tolerated unknown types, so the behavior was inconsistent.This makes the registry an enhancement source, not a gate, and aligns every field path on the same "tolerate unknown types" rule.
Release: 2.4.0 → 2.4.1 (patch).
Changes
gf_add_fieldgraceful degradation (src/field-operations/field-manager.js): unknown types are created from the caller's properties (caller-suppliedinputs/choicespreserved), skipping registry-derived defaults/sub-inputs, and the result carries a warning naming the unrecognized type._unknownflag (src/config/field-validation.js): it was written onto unknown-type fields and PUT verbatim to Gravity Forms viagf_create_form/gf_update_form, yet no code reads it.validateFieldConfig(src/field-definitions/field-registry.js): tolerate unknown types instead of returningisValid:false(the branch was dead via its only caller and contradicted the new decision).test/field-operations-integration.test.jsto exercise the realcreateFieldOperations(...)layer (removed ~118 lines of drifting mock that re-threw on unknown types and asserted a stalegf_list_field_typesshape).Testing (offline gate, all green)
npm run test:node— 357/357npm run test:unit— 289/289npm run test:field-validation,npm run test:views— passnpm run lint:docs,npm run lint:package(publint) — passAll behavior changes were written test-first (RED → GREEN) in
test/field-manager.test.js,test/field-validation.test.js,test/forms.test.js, andtest/field-registry.test.js.Notes
developwas 2 commits behindmain(the v2.4.0 release merge commits); content was identical, so this merges cleanly. After this lands, consider amain→developsync to reconcile history.v2.4.1tag +npm publishremain manual release steps.https://claude.ai/code/session_01LJHfTpQknHFs1j5ayj7fpo
Summary by CodeRabbit
Cookievalues; improved resilience for malformed/cyclic data.