Add completion spec: ip route - #317
Merged
Merged
Conversation
Add nested completions for `ip route`, covering the actions (add, change/chg, replace, prepend, append, del/delete, show/list/lst, flush, get, save, restore, showdump, help) and their attribute surface (via, dev/oif, src, metric, table, scope, proto, mtu, advmss, nexthop, onlink, route types, lockable metrics, etc.), modeled against upstream iproute2 ip/iproute.c and man/man8/ip-route.8.in. Bare (non-dash) keywords are modeled as `subcommands`, not `options`, per the fix landing in #311 -- Warp's completer only suggests dash-prefixed tokens from `options` arrays, so a bare keyword there is unreachable in the dropdown. show/flush/save share one option set (the SELECTOR filter grammar); add/change/replace/prepend/append/del share a different, larger set (the route attribute grammar); get has its own distinct set. These three grammars are verified as genuinely different parsers in upstream source, not copy-paste of one onto the others. dev/oif reuse the network_interfaces generator from #310 since they accept any interface. vrf and other narrower positions are left without a generator rather than reusing network_interfaces incorrectly. Co-Authored-By: Warp Agent <agent@warp.dev>
warp-agent-staging
Bot
force-pushed
the
factory/ip-route-completions
branch
from
August 13, 2026 01:40
97b238e to
97e9938
Compare
Reviewer found two gaps: 1. 'nexthop' was modeled as an opaque NH positional, so nothing completed after it. Model it as nested subcommands (via [FAMILY] ADDRESS, dev STRING, weight NUMBER, onlink, realms REALM, encap ENCAPTYPE, as [to] ADDRESS), matching parse_one_nh() in ip/iproute.c. Applied across all seven modify-grammar action blocks (add, change, replace, prepend, append, del, test). The inner 'dev' stays unbound from network_interfaces, as intended. 2. 'ip route test' was missing. It is undocumented in usage()/the man page, but do_iproute() dispatches it straight into the same iproute_modify() parser as add/change/replace/prepend/append/del, so it gets the identical grammar. Co-Authored-By: Warp Agent <agent@warp.dev>
Upstream's parse_features() (ip/iproute.c) consumes a space-separated sequence of feature tokens, e.g. 'features ecn tcp_usec_ts', not a single scalar value. Marks the FEATURES argument isVariadic across add/change/replace/prepend/append/del/test so completions keep offering the remaining feature name(s) after the first is typed. Co-Authored-By: Warp Agent <agent@warp.dev>
|
@warp-agent-staging[bot] I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR claims to add ip route completions in command-signatures/json/ip.json, but the attached annotated diff contains only Patch unavailable from GitHub for this file. No approved or repository spec context was provided.
Concerns
⚠️ [IMPORTANT] The patch content is unavailable inpr_diff.txt, so I cannot verify correctness, tests, comments, security implications, or implementation-vs-spec behavior against changed lines. Please regenerate the review context with the actual annotated diff.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
acarl005
approved these changes
Aug 13, 2026
Contributor
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
The PR description says this adds ip route completion metadata in command-signatures/json/ip.json, but the attached annotated diff contains no reviewable hunks or [NEW:n] / [OLD:n] line annotations; it only reports that the patch is unavailable for that file.
Concerns
⚠️ [IMPORTANT] The actual changed lines are missing from the review artifact, so I cannot audit correctness, security, comments, tests, or spec alignment, and I cannot attach inline feedback under the required diff-line rules. Please regenerate the review context with the annotated diff available or split the JSON change so the diff is reviewable.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Contributor
Author
|
/oz-review |
1 similar comment
Contributor
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR expands ip route completions with nested route-management, selector, lookup, restore, showdump, and help commands.
Concerns
- The modify/nexthop grammar omits the valid
pervasivenext-hop flag wherever onlyonlinkis modeled. ip route get asis modeled with the modify-routeas [to] ADDRESSgrammar, but the lookup form takesas ADDRESS; suggestingtohere would lead users toward an invalid invocation.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| } | ||
| }, | ||
| { | ||
| "name": "onlink", |
There was a problem hiding this comment.
NHFLAGS also accepts pervasive; add it alongside each copied onlink entry in the modify and nested nexthop grammars so Warp doesn't omit a valid route flag.
| "description": "Rewrite the destination address to ADDRESS before performing the lookup", | ||
| "args": [ | ||
| { | ||
| "name": "to", |
There was a problem hiding this comment.
ip route get as takes ADDRESS directly; as to ADDRESS is the modify-route grammar and would suggest an invalid to token here.
acarl005
pushed a commit
that referenced
this pull request
Aug 13, 2026
* ip route/tunnel: add pervasive nexthop flag, fix get as, add tunnel show generator Resolves the outstanding findings from the #317 and #310 re-reviews (main as of e8a33b4): - ip route add/change/prepend/replace/append/del/test: added the 'pervasive' NHFLAGS value (RTNH_F_PERVASIVE, 'do recursive gateway lookup') as a sibling of 'onlink' everywhere it was modeled -- both the route-level flag and the nested per-nexthop flag inside the repeatable 'nexthop' spec, for all 7 route-modifying verbs. Left 'ip nexthop add/replace''s own 'onlink' untouched: that command's own usage() grammar (ip/ipnexthop.c) does not include 'pervasive' at all, unlike ip/iproute.c's NHFLAGS. - ip route get ... as ADDRESS: dropped the optional leading 'to' keyword that only applies to the modify-route grammar's 'as [ to ] ADDRESS'; ip route get's own usage() and iproute_get's argument parsing take a bare 'as ADDRESS' with no 'to' variant, so suggesting 'to' there steered users into an invalid invocation. - ip tunnel show: added the network_interfaces generator to its optional NAME argument, matching the pattern already used elsewhere in this file (e.g. 'ip tunnel prl dev') for tunnel/interface names. Deliberately left alone per instruction: 'ip tunnel change' still omits the GRE seq/iseq/oseq/csum/icsum/ocsum options that 'ip tunnel add' has. That gap is intentionally reserved for #322, which already implements it on its own branch. Verified against main and the real iproute2 sources (ip/iproute.c, ip/iptunnel.c, ip/ip6tunnel.c) rather than the stale review text -- confirmed 'pervasive' is genuinely absent everywhere 'onlink' appears under ip route (0 prior occurrences), confirmed ip/ipnexthop.c's own grammar excludes 'pervasive', and confirmed via iproute.c's usage() that OPTIONS's 'as [ to ] ADDRESS' and route get's 'as ADDRESS' are genuinely different grammars. Validated: cargo test --workspace (137 passed), cargo fmt --check, cargo clippy --all-targets --all-features -D warnings, and npm run format:check (prettier) all clean. Co-Authored-By: Warp Agent <agent@warp.dev> * Revert pervasive nexthop flag and route get as; keep tunnel show generator only Both grammar changes from the previous commit were wrong, confirmed against upstream ip/iproute.c parse branches (not just usage() text, which is known to diverge from the parser in both directions): - 'pervasive' has no parse branch anywhere in iproute.c. It appears only in the usage string (NHFLAGS := [ onlink | pervasive ]) and in the route-printing path (a bare print_string call for RTNH_F_PERVASIVE, which the kernel/other means set, not the CLI). 'onlink' by contrast has real strcmp/matches branches in both the route-level modify loop and parse_one_nh. Completing to 'pervasive' would send users straight into a rejected command -- removed all 14 occurrences (route level and the nested per-nexthop spec, across all 7 modify verbs). - 'ip route get ... as ADDRESS' does accept an optional leading 'to': iproute_get()'s 'as' branch does 'if (strcmp(*argv, "to") == 0) NEXT_ARG();' before parsing the address, exactly like the modify-route grammar. The usage() synopsis just doesn't show it. Restored the optional 'to' argument to match what iproute_get() actually parses. 'ip nexthop add/replace' onlink (left untouched, unaffected) and 'ip tunnel show''s network_interfaces generator (unaffected, still correct) are the only two things this PR now touches -- the diff against main is now exactly the one-line tunnel show generator fix. Re-validated: cargo test --workspace (137 passed), cargo fmt --check, cargo clippy --all-targets --all-features -D warnings, and npm run format:check all clean. Co-Authored-By: Warp Agent <agent@warp.dev> --------- Co-authored-by: Warp Agent <agent@warp.dev>
This was referenced Aug 13, 2026
acarl005
pushed a commit
to warpdotdev/warp
that referenced
this pull request
Aug 13, 2026
## Description Updates `warp-command-signatures` from `fe352669` to `77c4a9a7`. This is the bump that actually delivers the `ip` completions work to users. Before it, every `ip` subcommand in the pinned spec was a bare leaf with no nested actions, options, or argument completions; after it, all 29 are filled in. ### Merged PRs - Add git read-tree completions (warpdotdev/command-signatures#296) - Add completions: ip rule, tunnel, xfrm, monitor (warpdotdev/command-signatures#310) - Fix ip rule/tunnel/xfrm/monitor completions: model bare keywords as subcommands (warpdotdev/command-signatures#311) - Add completions: ip neighbour (neigh) (warpdotdev/command-signatures#312) - Add completions: ip netns (warpdotdev/command-signatures#313) - Add completion spec: ip address (addr, a) (warpdotdev/command-signatures#315) - Add completion spec: ip route (warpdotdev/command-signatures#317) - Add completions: ip link (warpdotdev/command-signatures#318) - Add the neighbor (US spelling) alias to the ip neigh spec (warpdotdev/command-signatures#320) - Add completions for remaining ip subcommands (warpdotdev/command-signatures#321) - Fix ip neighbour completions: add 'neighbor' alias and full rt_protos vocabulary (warpdotdev/command-signatures#323) - Fix ip address completions: alias addr/a, local/dev keywords, nowhere scope, negated flags, accurate save (warpdotdev/command-signatures#324) - ip: fix macsec rx SCI chaining, ntable/tcp_metrics aliases, tuntap delete flags (warpdotdev/command-signatures#325) - ip tunnel: add interface generator to 'tunnel show NAME' (warpdotdev/command-signatures#326) - ip: add remaining nexthop/mrule/tunnel gaps, fix tunnel-show generator regression from #326 (warpdotdev/command-signatures#327) ### Worth knowing for review Two behaviors of this client's completer shaped the spec, and are worth being aware of when reading it: - Bare non-dash keywords must be modeled as `subcommands`, not `options`. `short_hand_flag_suggestions()` in `crates/warp_completer/src/completer/engine/flag/v2.rs` returns empty for any non-dash token, and only emits names satisfying `is_short_hand_flag_name()` or `is_long_hand_flag_name()`, both of which require a leading `-`. iproute2's grammar is almost entirely bare keywords, so an early version of this work was silently invisible until #311 remodeled it. - Only the first bare keyword after a verb completes. `deepest_matching_subcommand_signature()` in `crates/warp_completer/src/signatures/v2/lookup.rs` returns the matched subcommand alone, so its siblings stop being candidates — `ip rule add iif eth0 from <Tab>` offers nothing. That is an engine limitation rather than a spec one, tracked separately in #15048. It is not a regression: before this work none of these keywords completed at all. ## Linked Issue Delivers #9764, #9765, #9766, #9767, #9768, #9769. Follow-ups tracked in #15046 (IPv6 tunnel modes) and #15048 (chained-keyword completion). - [x] The linked issue is labeled `ready-to-implement`. - [x] Where appropriate, screenshots or a short video of the implementation are included below. ## Testing - [ ] I have manually tested my changes locally with `./script/run` This change is a dependency rev bump: two lines in `Cargo.toml` and `Cargo.lock`, no source changes. `cargo metadata` resolves cleanly against the new rev. The completions themselves were verified end to end against a locally built client earlier in this work, by pointing `warp-command-signatures` at a local checkout and exercising the dropdown. That is what caught the modeling bug fixed by #311. Evidence, including the before/after comparison on one build with only the spec crate swapped, is attached to warpdotdev/command-signatures#311. Being explicit about the gap: I did not run a full local `./script/run` or `cargo clippy` for this bump. The sandbox this was prepared in has 3 GB of RAM and the OOM killer terminates rustc partway through compiling the `warp` crate. Since the diff is a dependency rev with no source changes, CI is the meaningful gate here — but a reviewer who wants a local sanity check should run one before merging. ### Screenshots / Videos Completions dropdown verification is on warpdotdev/command-signatures#311. ## Agent Mode - [x] Warp Agent Mode - This PR was created via Warp's AI Agent Mode CHANGELOG-IMPROVEMENT: Added completions for `ip` — `address`, `link`, `route`, `neighbour`, `netns`, `rule`, `tunnel`, `xfrm`, `monitor` and the remaining subcommands — plus `git read-tree`. CHANGELOG-BUG-FIX: Fixed `ip` completions not appearing for iproute2's bare keyword arguments, along with missing `ip addr`/`ip a`/`ip neighbor` aliases and several suggestions that iproute2 rejects. Co-authored-by: Oz <oz-agent@warp.dev> Co-authored-by: Warp Agent <agent@warp.dev>
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
2 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.
Summary
Adds nested completions for
ip route— manage the kernel routing table (show, add, del, replace, etc.). Addresses warpdotdev/warp#9767 (APP-3980).Stacked conceptually on #310 (
ip rule,ip tunnel,ip xfrm,ip monitor), which has since merged tomain; this PR branches offmain(which now includes it) and reuses itsnetwork_interfacesgenerator. It only touches theroutesubtree ofcommand-signatures/json/ip.json.Important modeling note: bare (non-dash) keywords are modeled as
subcommands, notoptions, since Warp's completer only ever suggests dash-prefixed tokens fromoptionsarrays — a bare keyword placed there would never appear in the dropdown. Every bare keyword underroute(via,dev,table,scope,tos,metric, …) is modeled as asubcommandsentry instead, with its value carried on that entry's own positionalargs, matching the fix in #311 forrule/tunnel/xfrm/monitor.Changes
ip route add|change|chg|replace|prepend|append|del|delete|test: fullNODE_SPEC/INFO_SPECattribute grammar —via [FAMILY] ADDRESS,src,as [to] ADDRESS,from,tos/dsfield,expires,metric/priority/preference,scope, the lockable metrics (mtu,hoplimit,advmss,reordering,rtt,rto_min,window,cwnd,initcwnd,initrwnd,rttvar,ssthresh,congctl),features(ecn,tcp_usec_ts— iproute2 6.8+; the argument is variadic sinceparse_features()accepts multiple space-separated values in one invocation),quickack,realms,onlink,nhid,protocol,table,vrf,dev/oif,pref,encap,ttl-propagate,fastopen_no_cookie, and the routeTYPEenum (unicast,local,broadcast,multicast,throw,unreachable,prohibit,blackhole,nat).nexthop: modeled as its own nested clause (matchingparse_one_nh()), withvia [FAMILY] ADDRESS,dev STRING,weight NUMBER,onlink,realms REALM,encap ENCAPTYPE, andas [to] ADDRESSchildren, so completions continue past thenexthopkeyword when building a multipath (ECMP) route. Applied identically across all seven modify actions listed above.ip route test: undocumented (absent fromusage()/the man page) but present indo_iproute()'s dispatch table, sharing the identical grammar asadd/change/replace/prepend/append/delete; it validates a route request against the kernel without committing it (dispatches with only the exclusivity flag set).ip route show|list|lst/flush/save: the separateSELECTORfilter grammar (tableincl.all/cache,vrf,cached/cloned,tos,protocolincl.all,scopeincl.all,type,dev/oif,iif,mark,metric,via,src,realms,from [root|match|exact]) plus the[to] [root|match|exact] PREFIXpositional selector. This is a genuinely different parser (iproute_list_flush_or_save) from the modify grammar above — verified againstip/iproute.c, not copy-pasted.ip route get: its own distinct option set (tos,from,iif,mark,oif/dev,notify,connected,vrf,uid,fibmatch,as,sport,dport,ipproto,flowlabel— iproute2 6.14+).ip route restore/showdump: modeled as taking no arguments, periproute_restore()/iproute_showdump()reading only from stdin — deliberately not given the SELECTOR set thatsavegets, sincesaveandrestore/showdumpare different code paths.dev/oifreuse thenetwork_interfacesgenerator from Add completions: ip rule, tunnel, xfrm, monitor #310 wherever the position genuinely accepts any interface (route'sdev, the filter'sdev/iif,get'siif/oif).vrfandnexthop's innerdevare left without a generator since they're a narrower subset of interfaces, and a wrong-but-broad suggestion is worse than none.Sources verified against
ip/iproute.c(upstreamiproute2/iproute2,mainbranch) —usage(),iproute_modify(),iproute_list_flush_or_save(),iproute_get(),iproute_restore(),iproute_showdump(),do_iproute(),parse_features(),parse_one_nh().ip/iproute_lwtunnel.c— authoritativeENCAPTYPEkeyword list (mpls,ip,ip6,ila,bpf,seg6,seg6local,rpl,ioam6,xfrm); this differs from both theusage()string (missingbpf) and the man page (missingila/rpl), so the C parser was used as the tie-breaker.man/man8/ip-route.8.in— option descriptions,SELECTOR/root/match/exactsemantics,get's flag descriptions.flowlabel(ip route get, landed iproute2 6.14), fortcp_usec_ts(ip route add ... features, landed iproute2 6.8), and for theparse_features()fix that madefeaturesaccept multiple values in one invocation.Deliberately left out
weightoption at the top level ofadd/change/etc. — it's only valid inside anexthopclause for multipath routes, not as a route-level attribute; adding it at the top level would be a misattached-flag bug.encap's full per-type header syntax (MPLS labels, SRv6 segments, IOAM trace parameters, etc.) — kept at the same shallow-but-accurate depth asxfrm'sencap/offloadin Add completions: ip rule, tunnel, xfrm, monitor #310 (top-level keyword only), since the full grammar is its own multi-hundred-line sub-language.Follow-up required
warpdotdev/warp'sCargo.tomlpins thewarp-command-signaturesgit dependency at revfe3526693fe4ea3dc208ee5ef892b3aad2679af6. That pin must be bumped to a commit that includes this PR after it merges here, or these completions will never reach users. The bump can't be made as part of this PR since it can't point at a commit that hasn't merged yet.Verification
npm run format:check— passes.cargo fmt -p warp-command-signatures -p warp-completion-metadata --check— passes.cargo clippy -p warp-command-signatures -p warp-completion-metadata --all-targets --all-features -- -D warnings— passes.cargo test --verbose— 133 tests pass (spec deserialization,generatorNameexistence, no-unquoted-newlines invariants all included), pluswarp-completion-metadata's own suite.src/generators/ip.rs.