Add specs for IPAM search closest match prefix#8467
Merged
Conversation
Contributor
|
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:
WalkthroughTwo new specification documents were added for IPAM Parent Prefix Lookup (INFP-431): a requirements quality checklist (all checklist items marked complete) at dev/specs/infp-431-ipam-closest-prefix/checklists/requirements.md, and a feature specification at dev/specs/infp-431-ipam-closest-prefix/spec.md. The feature spec defines user stories, acceptance criteria, functional requirements for IP/prefix detection and containment lookups across namespaces, specificity-ordered results, a "Parent Prefixes" UI section, IPv6 normalization, fallback to text search, cross-namespace visibility within the active branch, navigation to prefix detail pages, edge cases, and assumptions. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dev/specs/infp-431-ipam-closest-prefix/checklists/requirements.md`:
- Around line 32-34: The readiness note in requirements.md ("All items pass.
Spec is ready for `/speckit.clarify` or `/speckit.plan`") conflicts with the
spec status being "Draft"; reconcile by either (A) updating the note in
requirements.md to clearly state the spec is still Draft and list the exact next
action (e.g., "Draft — not yet approved for /speckit.plan; submit to
/speckit.clarify first"), or (B) if planning is approved, change the spec's
status metadata from "Draft" to the approved state (e.g., "Ready for Planning")
so it matches the note. Edit the "Notes" paragraph in requirements.md or the
spec's status field accordingly to make the readiness unambiguous.
In `@dev/specs/infp-431-ipam-closest-prefix/spec.md`:
- Around line 82-93: Add a new functional requirement (e.g., FR-012) that
parent-prefix search results must be filtered by the invoking user's
authorization/visibility rules so only prefixes the user is allowed to see are
returned; update acceptance criteria to state that results respect existing
ACLs/branch visibility (consistent with FR-011), that unauthorized prefixes are
excluded from both count and display, and include a test case verifying a user
with limited permissions cannot see restricted prefixes when performing an IP or
CIDR lookup; reference the parent-prefix lookup behavior and existing search
permission logic when implementing this constraint.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dev/specs/infp-431-ipam-closest-prefix/checklists/requirements.mddev/specs/infp-431-ipam-closest-prefix/spec.md
wvandeun
reviewed
Feb 26, 2026
wvandeun
reviewed
Feb 26, 2026
| - **FR-005**: System MUST fall back to existing text search behavior when the query is not a valid complete IP address or CIDR prefix (e.g., partial IPs, hostnames, or general text). | ||
| - **FR-006**: System MUST normalize IPv6 input before performing the lookup to ensure consistent matching regardless of input formatting variations. | ||
| - **FR-007**: System MUST support both IPv4 and IPv6 addresses and prefixes. | ||
| - **FR-008**: System MUST display parent prefix lookup results in a dedicated "Parent Prefixes" section within the search anywhere UI, visually separated from regular text search results, so users can clearly identify containment-based results. |
Contributor
There was a problem hiding this comment.
@bilalabbad @pa-lem this does imply frontend work
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
dev/specs/infp-431-ipam-closest-prefix/spec.md (1)
84-94:⚠️ Potential issue | 🟠 MajorAdd explicit user authorization filtering to parent-prefix results.
The spec still defines branch visibility but not explicit per-user authorization/ACL filtering for returned prefixes. Please require that parent-prefix results include only prefixes the current user is allowed to view (and that unauthorized prefixes are excluded from both count and display).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/specs/infp-431-ipam-closest-prefix/spec.md` around lines 84 - 94, Update the parent-prefix lookup to apply per-user authorization filtering so only prefixes the current user can view are returned and counted: in the logic that builds Parent Prefixes results (the "Parent Prefixes" UI section and the code paths implementing FR-008/FR-011/FR-012), call the existing authorization check (e.g., authorizePrefixView(user, prefix) or filterVisiblePrefixesForUser(user, prefixes)) and exclude any unauthorized prefixes from the result set and total counts; ensure the UI rendering for the Parent Prefixes section and the click-to-detail behavior (FR-009) only exposes authorized prefixes and that unauthorized matches do not affect other search results, ordering, or branch-visibility rules.
🧹 Nitpick comments (1)
dev/specs/infp-431-ipam-closest-prefix/spec.md (1)
109-109: Make SC-003 measurable instead of absolute.“Zero performance regression” / “unchanged” is not practically testable. Define a concrete SLO-style threshold (e.g., p95 latency delta under a fixed dataset/load).
Proposed wording
-- **SC-003**: Existing text search queries (non-IP) experience zero performance regression; response times remain unchanged. +- **SC-003**: For non-IP queries, p95 search latency must not regress by more than 5% versus baseline under the agreed benchmark dataset and load profile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/specs/infp-431-ipam-closest-prefix/spec.md` at line 109, Update SC-003 to make it measurable by replacing the absolute phrase "zero performance regression; response times remain unchanged" with an SLO-style threshold; for example, specify a concrete metric and limit such as "p95 latency delta for non-IP text search queries must be <= 5% under the standard test dataset and synthetic load" and include the test conditions (dataset version, query mix, and load profile) and measurement window so the requirement on SC-003 is verifiable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@dev/specs/infp-431-ipam-closest-prefix/spec.md`:
- Around line 84-94: Update the parent-prefix lookup to apply per-user
authorization filtering so only prefixes the current user can view are returned
and counted: in the logic that builds Parent Prefixes results (the "Parent
Prefixes" UI section and the code paths implementing FR-008/FR-011/FR-012), call
the existing authorization check (e.g., authorizePrefixView(user, prefix) or
filterVisiblePrefixesForUser(user, prefixes)) and exclude any unauthorized
prefixes from the result set and total counts; ensure the UI rendering for the
Parent Prefixes section and the click-to-detail behavior (FR-009) only exposes
authorized prefixes and that unauthorized matches do not affect other search
results, ordering, or branch-visibility rules.
---
Nitpick comments:
In `@dev/specs/infp-431-ipam-closest-prefix/spec.md`:
- Line 109: Update SC-003 to make it measurable by replacing the absolute phrase
"zero performance regression; response times remain unchanged" with an SLO-style
threshold; for example, specify a concrete metric and limit such as "p95 latency
delta for non-IP text search queries must be <= 5% under the standard test
dataset and synthetic load" and include the test conditions (dataset version,
query mix, and load profile) and measurement window so the requirement on SC-003
is verifiable.
gmazoyer
force-pushed
the
gma-20260224-infp431-spec
branch
from
6b1a002 to
d643a6e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dev/specs/infp-431-ipam-closest-prefix/spec.md`:
- Line 93: FR-011 currently requires branch visibility but omits explicit user
authorization filtering; update FR-011 (or add a new FR-012) to require that
parent prefix lookup also enforces the current user's authorization and
access-control rules so results include only prefixes the user is permitted to
view, and update the acceptance criteria to state permission-filtering must be
verified (align behavior with existing search permission checks and include a
test case verifying unauthorized prefixes are excluded).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dev/specs/infp-431-ipam-closest-prefix/checklists/requirements.mddev/specs/infp-431-ipam-closest-prefix/spec.md
🚧 Files skipped from review as they are similar to previous changes (1)
- dev/specs/infp-431-ipam-closest-prefix/checklists/requirements.md
gmazoyer
force-pushed
the
gma-20260224-infp431-spec
branch
from
d643a6e to
e3cd3f1
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
dev/specs/infp-431-ipam-closest-prefix/spec.md (1)
93-95:⚠️ Potential issue | 🟠 MajorAdd explicit user-level authorization filtering here.
FR-011only constrains branch visibility. As written, the spec still permits returning prefixes from the active branch that the current user is not allowed to see, which creates an information-disclosure gap. Please extend this requirement, and the related acceptance coverage, to state that parent-prefix results must also respect existing ACL/authorization rules so unauthorized prefixes are excluded from both display and counts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/specs/infp-431-ipam-closest-prefix/spec.md` around lines 93 - 95, Update FR-011 to require that parent-prefix lookup not only respect the user's active branch context but also apply the system's existing ACL/authorization filters so that any prefixes the current user is not permitted to view are excluded from results and counts; update related acceptance criteria and test cases (references: FR-011, FR-012, FR-013 and their "parent prefix lookup" acceptance bullets) to assert that unauthorized prefixes are omitted from both the displayed result set and the result counts, and add explicit test steps verifying both display and count behavior for users with limited permissions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@dev/specs/infp-431-ipam-closest-prefix/spec.md`:
- Around line 93-95: Update FR-011 to require that parent-prefix lookup not only
respect the user's active branch context but also apply the system's existing
ACL/authorization filters so that any prefixes the current user is not permitted
to view are excluded from results and counts; update related acceptance criteria
and test cases (references: FR-011, FR-012, FR-013 and their "parent prefix
lookup" acceptance bullets) to assert that unauthorized prefixes are omitted
from both the displayed result set and the result counts, and add explicit test
steps verifying both display and count behavior for users with limited
permissions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ecfa987-89c6-4c40-87d9-245fd7af7ab6
📒 Files selected for processing (2)
dev/specs/infp-431-ipam-closest-prefix/checklists/requirements.mddev/specs/infp-431-ipam-closest-prefix/spec.md
✅ Files skipped from review due to trivial changes (1)
- dev/specs/infp-431-ipam-closest-prefix/checklists/requirements.md
ogenstad
added
the
type/spec
wvandeun
approved these changes
Mar 13, 2026
gmazoyer
force-pushed
the
gma-20260224-infp431-spec
branch
from
e3cd3f1 to
ca66396
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
dev/specs/infp-431-ipam-closest-prefix/spec.md (1)
93-93:⚠️ Potential issue | 🟠 MajorAdd explicit user authorization filtering to parent-prefix results.
Line 93 scopes visibility to active branch, but it still doesn’t explicitly require enforcing user ACL/permissions. Please add a requirement (or extend FR-011) that only prefixes the current user is authorized to view are returned.
Suggested spec patch
-- **FR-011**: The parent prefix lookup MUST respect the user's currently active branch context, returning only prefixes visible on that branch (consistent with existing search behavior). +- **FR-011**: The parent prefix lookup MUST respect the user's currently active branch context, returning only prefixes visible on that branch (consistent with existing search behavior). +- **FR-012**: The parent prefix lookup MUST enforce existing user authorization/ACL rules, returning only prefixes the current user is permitted to view. -- **FR-012**: All existing search anywhere behavior MUST remain unchanged. The parent prefix lookup is purely additive — it adds a new results section when an IP/prefix is detected but MUST NOT alter, remove, or interfere with any other search results, result ordering, or UI behavior. +- **FR-013**: All existing search-anywhere behavior MUST remain unchanged. The parent prefix lookup is purely additive — it adds a new results section when an IP/prefix is detected but MUST NOT alter, remove, or interfere with any other search results, result ordering, or UI behavior. -- **FR-013**: When the search query is a valid IP address or CIDR prefix, the system MUST return exact-match IP address or prefix objects (if they exist in the system) in the search results, in addition to the parent prefix lookup results. This ensures that searching for an existing IP address or prefix always surfaces that object. +- **FR-014**: When the search query is a valid IP address or CIDR prefix, the system MUST return exact-match IP address or prefix objects (if they exist in the system) in the search results, in addition to the parent prefix lookup results. This ensures that searching for an existing IP address or prefix always surfaces that object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/specs/infp-431-ipam-closest-prefix/spec.md` at line 93, Extend FR-011 (parent prefix lookup) to require that results are filtered by the requesting user's authorization/ACLs in addition to the active branch context: when resolving parent prefixes (parent-prefix lookup) return only prefixes that are both visible on the user's active branch and that the current user is authorized to view per the system's permission model (e.g., role-based ACLs, ownership, or explicit read permissions); reference the parent-prefix lookup behavior and FR-011 in the spec update and state that authorization checks must be applied server-side before returning results.dev/specs/infp-431-ipam-closest-prefix/checklists/requirements.md (1)
34-34:⚠️ Potential issue | 🟡 MinorReconcile readiness note with current spec status.
Line 34 says the spec is ready for planning, but
dev/specs/infp-431-ipam-closest-prefix/spec.md(Line 5) is still marked Draft. Please align these to avoid process ambiguity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/specs/infp-431-ipam-closest-prefix/checklists/requirements.md` at line 34, Update the readiness statement in requirements.md (the “All items pass. Spec is ready for `/speckit.clarify` or `/speckit.plan`” line) to match the spec status in spec.md (which currently shows “Draft”) by either: a) changing the readiness note to indicate the spec is still Draft and not yet ready for planning, or b) promoting the spec status in spec.md from Draft to Ready and adding a brief changelog entry; ensure the two places (requirements.md readiness sentence and spec.md top-level status) are consistent so there is no ambiguity in the review workflow.
🧹 Nitpick comments (1)
dev/specs/infp-431-ipam-closest-prefix/spec.md (1)
92-92: Make performance criteria measurable instead of absolute.“Must not degrade” and “zero regression/unchanged” are hard to verify in practice. Define a measurable bound (for example, p95 latency delta threshold under a fixed dataset/query mix).
Also applies to: 110-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/specs/infp-431-ipam-closest-prefix/spec.md` at line 92, Change the non-measurable acceptance criteria in FR-010 from "MUST NOT degrade" to a quantifiable bound: specify the metric (e.g., p95 query latency), the allowed delta (e.g., <= X ms or <= Y% increase), the workload (fixed dataset and query mix), and the test conditions (concurrency, hardware baseline), and apply the same change to FR-110 so both use the identical measurable performance threshold and test procedure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@dev/specs/infp-431-ipam-closest-prefix/checklists/requirements.md`:
- Line 34: Update the readiness statement in requirements.md (the “All items
pass. Spec is ready for `/speckit.clarify` or `/speckit.plan`” line) to match
the spec status in spec.md (which currently shows “Draft”) by either: a)
changing the readiness note to indicate the spec is still Draft and not yet
ready for planning, or b) promoting the spec status in spec.md from Draft to
Ready and adding a brief changelog entry; ensure the two places (requirements.md
readiness sentence and spec.md top-level status) are consistent so there is no
ambiguity in the review workflow.
In `@dev/specs/infp-431-ipam-closest-prefix/spec.md`:
- Line 93: Extend FR-011 (parent prefix lookup) to require that results are
filtered by the requesting user's authorization/ACLs in addition to the active
branch context: when resolving parent prefixes (parent-prefix lookup) return
only prefixes that are both visible on the user's active branch and that the
current user is authorized to view per the system's permission model (e.g.,
role-based ACLs, ownership, or explicit read permissions); reference the
parent-prefix lookup behavior and FR-011 in the spec update and state that
authorization checks must be applied server-side before returning results.
---
Nitpick comments:
In `@dev/specs/infp-431-ipam-closest-prefix/spec.md`:
- Line 92: Change the non-measurable acceptance criteria in FR-010 from "MUST
NOT degrade" to a quantifiable bound: specify the metric (e.g., p95 query
latency), the allowed delta (e.g., <= X ms or <= Y% increase), the workload
(fixed dataset and query mix), and the test conditions (concurrency, hardware
baseline), and apply the same change to FR-110 so both use the identical
measurable performance threshold and test procedure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b93a2d8-3c53-4799-aab9-3d43269560b9
📒 Files selected for processing (2)
dev/specs/infp-431-ipam-closest-prefix/checklists/requirements.mddev/specs/infp-431-ipam-closest-prefix/spec.md
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
3 participants
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.
Why
Adds specifications for Spec Kit for IPAM search closest matched prefix for non-existing IP INFP-431
What changed
How to review
Summary by CodeRabbit