feat(ip): add ip route show/get builtin (Linux only)

[AlexandreYang]

Summary

• Adds ip route [show|list] and ip route get ADDRESS subcommands to the existing ip builtin (Linux only)
• Routes are read from /proc/net/route via callCtx.OpenFile, respecting the AllowedPaths sandbox
• All write operations (add, del, flush, replace, change, save, restore) are blocked with exit 1
• Extracts /proc/net/route parsing into a dedicated builtins/internal/procnet package for reusability

Implementation details

• Little-endian uint32 hex decoding for /proc/net/route IP fields
• Longest-prefix-match lookup for ip route get
• Memory safety: MaxLineBytes (1 MiB) cap per line, maxRoutes (10 000) cap
• Linux-only guard (runtime.GOOS); other platforms return a clear error
• ProcNetRoutePath exported for testability (tests override with temp file)

Tests

• Linux-only Go tests (ip_linux_test.go) with synthetic /proc/net/route files
• Pentest tests (ip_pentest_linux_test.go) for adversarial inputs
• Fuzz tests (ip_route_fuzz_linux_test.go): FuzzIPRouteParse, FuzzIPRouteGetAddr
• YAML scenario tests for blocked write ops, error cases, and IPv6 rejection
• SHELL_FEATURES.md updated to document ip route

Generated with Claude Code

[AlexandreYang]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f9df211bc3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[AlexandreYang]

@codex review

[AlexandreYang]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 24aa04a7a0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[AlexandreYang]

@codex review

[AlexandreYang]

Code Review — feat(ip): add ip route show/get builtin

Reviewed commit 72fb2b2b. Overall: needs fixes — one open P1 (data race) and two style/coverage findings. Two other P1s were already fixed before this review landed.

---

Summary Table

#	Priority	File	Finding
1		builtins/tests/ip/ip_route_fuzz_linux_test.go:37	Data race on ProcNetRoutePath global in parallel fuzz workers
2		builtins/internal/procnet/procnet_linux.go	os.Open bypasses the AllowedPaths sandbox wrapper — intentional but undocumented
3		tests/scenarios/cmd/ip/errors/unknown_object.yaml	stderr_contains weakens the assertion — should use exact stderr match
4		tests/scenarios/cmd/ip/	Missing scenario test for ip route get output format

Already fixed before this review:

• strconv.FormatUint missing from "ip" entry in builtinPerCommandSymbols — fixed in the branch.
• Fuzz tests using runScriptCtx which calls t.Fatalf on interpreter panics — fixed by switching to cmdRunCtxFuzz + code == -1 guard.

---

Findings

Data race on ProcNetRoutePath in parallel fuzz workers

Severity: P1 — Concurrency
Location: builtins/tests/ip/ip_route_fuzz_linux_test.go:37–48

Description:
writeFuzzRoute writes to the package-level variable ipcmd.ProcNetRoutePath without any synchronization:

func writeFuzzRoute(t *testing.T, content []byte) (cleanup func()) {
    orig := ipcmd.ProcNetRoutePath
    ipcmd.ProcNetRoutePath = dir          // unsynchronized write
    return func() { ipcmd.ProcNetRoutePath = orig }
}

The Go fuzzing engine runs fuzz workers in parallel goroutines. Each worker independently calls writeFuzzRoute, all racing to read and write the same global. With -race enabled (which CI uses), this will be detected and fail. Even without the race detector, workers can read a path written by a different worker, causing spurious test failures or incorrect test state.

Evidence:
The same pattern in FuzzIPRouteParse and FuzzIPRouteGetAddr both invoke writeFuzzRoute in their fuzz functions. Since fuzz workers run concurrently, any two concurrent executions race.

Remediation:
Use a mutex to protect access to ProcNetRoutePath, or — better — give each fuzz worker its own temp directory and set the path only within that worker's goroutine scope. The cleanest fix is to refactor writeFuzzRoute to use a per-invocation path variable rather than the package-level global, passing it through context or via a function parameter:

var procNetRouteMu sync.Mutex

func writeFuzzRoute(t *testing.T, content []byte) (cleanup func()) {
    t.Helper()
    dir := t.TempDir()
    netDir := filepath.Join(dir, "net")
    require.NoError(t, os.MkdirAll(netDir, 0o755))
    if err := os.WriteFile(filepath.Join(netDir, "route"), content, 0o644); err != nil {
        return func() {}
    }
    procNetRouteMu.Lock()
    orig := ipcmd.ProcNetRoutePath
    ipcmd.ProcNetRoutePath = dir
    procNetRouteMu.Unlock()
    return func() {
        procNetRouteMu.Lock()
        ipcmd.ProcNetRoutePath = orig
        procNetRouteMu.Unlock()
    }
}

A more robust fix would be to make ProcNetRoutePath a per-invocation parameter to the route-reading function rather than a package-level global, but that requires a larger refactor.

---

os.Open bypasses the AllowedPaths sandbox wrapper

Severity: P2 — Security (intentional divergence, undocumented)
Location: builtins/internal/procnet/procnet_linux.go

Description:
procnet_linux.go calls os.Open directly to read /proc/net/route. This bypasses the shell's AllowedPaths sandbox wrapper, which is the mechanism that restricts file access in the restricted shell. This is the single most critical security invariant in this codebase per the code-review skill guidelines.

The usage is likely intentional — /proc/net/route is a kernel pseudo-file that should always be accessible regardless of sandbox configuration, and the ip route builtin is a network-information command analogous to reading /proc/net/tcp in the ss builtin.

However, there is no comment in the code explaining this deliberate sandbox bypass. Future reviewers (or the allowlist checker) may flag this as a vulnerability without the context.

Remediation:
Add an explicit comment explaining that the direct os.Open call is intentional:

// os.Open is used directly (bypassing the AllowedPaths sandbox wrapper) because
// /proc/net/route is a kernel-managed pseudo-file that must always be readable for
// network introspection commands, regardless of sandbox path restrictions.
// This matches the precedent set by the ss builtin (procnet/tcp reading).
f, err := os.Open(filepath.Join(procPath, "net", "route"))

---

stderr_contains weakens the assertion

Severity: P3 — Test Coverage
Location: tests/scenarios/cmd/ip/errors/unknown_object.yaml

Description:
The scenario uses stderr_contains rather than an exact stderr match. Per the project conventions in CLAUDE.md: "use expect.stderr when possible instead of stderr_contains". The current value is a partial string match, which allows the stderr output to change without the test catching it.

Remediation:
Replace stderr_contains with expect.stderr using the full expected error string.

---

Missing scenario test for ip route get output format

Severity: P3 — Test Coverage
Location: tests/scenarios/cmd/ip/

Description:
ip route get <ADDRESS> has output format coverage only in the Go pentest test (TestIPRoutePentestGetOutputFormat). There is no YAML scenario test. Per project conventions, scenario tests are preferred because they are automatically validated against both the shell and bash.

Note: Since ip route get output diverges from real bash ip route get (which has more fields like src, uid, cache), the scenario would need skip_assert_against_bash: true.

Remediation:
Add a scenario such as:

description: ip route get returns the nexthop for a given destination
skip_assert_against_bash: true  # output intentionally simpler than real ip-route(8)
input:
  script: |
    ip route get 8.8.8.8
expect:
  exit_code: 0
  stdout: "8.8.8.8 via 192.168.1.1 dev eth0\n"

---

Test Coverage Summary

Code path	Scenario test	Go test	Status
ip route show — default route	tests/scenarios/cmd/ip/	ip_linux_test.go	Covered
ip route show — empty file	—	ip_pentest_linux_test.go:82	Covered
ip route show — /dev/zero hang guard	skip (intentional)	ip_pentest_linux_test.go:99	Covered
ip route show — long lines	—	ip_pentest_linux_test.go:127	Covered
ip route show — metric overflow	—	ip_pentest_linux_test.go:218	Covered
ip route show — binary content	—	ip_pentest_linux_test.go:244	Covered
ip route get <addr> — valid address	—	ip_pentest_linux_test.go:287	Go only (missing scenario)
ip route get <addr> — invalid address	—	ip_pentest_linux_test.go:37	Covered
ip route — unknown subcommand	tests/scenarios/cmd/ip/errors/	—	Covered (weak assertion)
LongestPrefixMatch — metric tie-breaking	—	procnet_linux_test.go	Covered
Fuzz: arbitrary /proc/net/route content	—	ip_route_fuzz_linux_test.go:58	Covered
Fuzz: arbitrary ip route get address	—	ip_route_fuzz_linux_test.go:159	Covered (data race)

---

Positive Observations

• MaxLineBytes cap on the bufio.Scanner correctly protects against /dev/zero-style infinite reads — the pentest test confirms this.
• parseIPv4 validates all four octets fit in [0,255] before assembling the uint32, preventing integer overflow.
• LongestPrefixMatch metric tie-breaking (r.Metric < best.Metric) is correct — lower metric wins, matching Linux routing table semantics.
• parseRouteEntry validates field count before any indexing, preventing out-of-bounds panics on malformed lines.
• The procnet internal package cleanly separates /proc/net/route parsing from the ip command logic, and ProcNetRoutePath makes the path injectable for tests.
• The fuzz seed corpus is thorough: it covers all CVE-class inputs (null bytes, CRLF, binary magic, long lines) and all existing test inputs as required by the seed corpus comment header.

🤖 Generated with Claude Code

[AlexandreYang]

Found 1 review item.

• builtins/ip/ip.go:121 still leaves Cmd.Description as "show network interface information". That description is what the top-level help builtin prints, so after adding ip route the shell still advertises ip as interface-only and hides the new routing support from users. I verified this against the PR head with env GOCACHE=/tmp/go-build-cache go run ./cmd/rshell --allow-all-commands -c 'help', which still prints ip show network interface information. Please update the builtin description to include routing as well.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1c91e7ee9b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[AlexandreYang]

@codex review

[AlexandreYang]

Fixed in ea57fc9: updated Cmd.Description to "show network interface and routing information".

[AlexandreYang]

Review Summary

Reviewed feat(ip): add ip route show/get builtin (branch alex/ip_route vs main).

This PR adds ip route [show|list] and ip route get ADDRESS subcommands backed by a new builtins/internal/procnet package that reads /proc/net/route directly. All write subcommands are properly blocked. The implementation is thorough, with functional tests, pentest tests, and fuzz tests.

Overall assessment: safe to merge. No P0/P1 issues found. Three minor P3 observations below.

#	Priority	File	Finding
1		builtins/ip/ip.go	-o/--brief global flags silently ignored for route output
2		builtins/tests/ip/ip_route_fuzz_linux_test.go	writeProcNetRoute lacks mutex used in writeFuzzRoute
3		tests/scenarios/cmd/ip/	No scenario test for ip route (no-subcommand default)

---

Positive Observations

• Resource exhaustion protection: MaxRoutes (10 000) and MaxLineBytes (1 MiB) caps prevent unbounded allocation; /dev/zero is covered by a pentest test.
• Context cancellation: checked at every iteration in both the scanner loop (readRoutes) and the output loop (routeShow).
• /proc access documented: os.Open bypass of the AllowedPaths sandbox is explicitly justified (matches ss precedent) and noted in the package-level doc comment.
• Write ops comprehensively blocked: all of add, del, delete, change, replace, flush, save, restore return exit 1; each has its own scenario test.
• Safe path construction: filepath.Join(procPath, "net", "route") — no string concatenation.
• parseIPv4 validation: rejects non-decimal, overflow octets, too few/many parts, and IPv6 addresses cleanly.
• Correct LPM tie-breaking: lower metric wins on equal prefix length; covered by TestIPRouteGetMetricTieBreak.
• formatRoute default-route detection: only Dest==0 && Mask==0 prints "default"; the TestIPRouteShowZeroDestNonZeroMaskNotDefault test pins this.
• Fuzz corpus: seeds cover implementation boundaries, CVE-class inputs (null bytes, CRLF, binary magic, near-MaxLineBytes), and all existing test variants.

[AlexandreYang]

Review: feat(ip): add ip route show/get builtin (Linux only)

Overall Assessment: safe to merge (no P0/P1/P2 findings)

This PR adds ip route [show|list] and ip route get ADDRESS to the existing ip builtin. The implementation is well-structured and security-conscious. I reviewed:

• builtins/ip/ip.go — command dispatch, subcommand parsing, output formatting
• builtins/internal/procnetroute/ — /proc/net/route parsing, LPM, path validation
• allowedsymbols/symbols_builtins.go + symbols_internal.go — symbol allowlist additions
• All test files: ip_linux_test.go, ip_pentest_linux_test.go, ip_route_fuzz_linux_test.go
• All YAML scenario tests in tests/scenarios/cmd/ip/
• SHELL_FEATURES.md and README.md documentation updates

Security Assessment

Sandbox integrity: os.Open is called directly in procnetroute.readRoutes, bypassing AllowedPaths. This is intentional and correctly documented in AGENTS.md, the package doc comment, and README.md. The path is always derived from ProcNetRoutePath which is hardcoded to /proc in production and never controlled by user input. The .. component check before filepath.Clean correctly blocks traversal sequences.

No user input reaches os.Open. ip route get ADDR passes user input only through parseIPv4 which converts it to a uint32; the result never touches the filesystem path.

Write operations: all of add, del, delete, change, replace, flush, save, restore are blocked with exit 1. -6 (IPv6), --oneline, --brief are rejected for route. ip netns, -b/-B/-batch, --force, -n/--netns are already blocked from the pre-existing implementation.

Memory safety: MaxRoutes (10 000) caps entries in memory; MaxTotalLines (100 000) caps scan time; MaxLineBytes (1 MiB) per line caps allocations. All caps produce hard errors rather than silent truncation, so callers know when LPM results may be unreliable.

DoS prevention: all read loops check ctx.Err() at each iteration; /dev/zero protection verified in TestIPRoutePentestDevZero.

Concurrency: ProcNetRoutePath is read-only in production; test-only mutations are serialised with procNetRouteMu. Fuzz goroutines also hold this mutex for the duration of each iteration.

Bash Compatibility

ip route is a Linux-only command reading /proc/net/route. The output format (no proto/scope/src fields — not available from /proc/net/route alone) is documented. All scenario tests that intentionally diverge from bash are correctly marked skip_assert_against_bash: true.

Test Coverage

Code path	Scenario test	Go test	Status
ip route show happy path	— (Linux only, per AGENTS.md)	ip_linux_test.go	Covered
ip route list alias	—	TestIPRouteListAliasForShow	Covered
ip route (bare, no subcommand)	—	TestIPRouteShowDefaultRouteAlias	Covered
ip route get ADDRESS happy path	—	TestIPRouteGetDefaultRoute, TestIPRouteGetNetworkRoute	Covered
LPM tie-break by metric	—	TestIPRouteGetMetricTieBreak	Covered
RTF_REJECT routes in show/get	—	TestIPRouteShowRejectRoute, TestIPRouteGetRejectRouteUnreachable	Covered
Host route /32 (no /32 suffix)	—	TestIPRouteShowHostRouteNoSlash32	Covered
Non-octet CIDR masks (/25, /17, /28)	—	TestIPRouteShowNonOctetMasks	Covered
MaxRoutes cap	—	TestIPRouteMaxRoutesCap	Covered
MaxLineBytes long-line protection	—	TestIPRoutePentestLongLines	Covered
/dev/zero hang protection	—	TestIPRoutePentestDevZero	Covered
Binary/adversarial file content	—	TestIPRoutePentestBinaryContent	Covered
Write ops blocked (add/del/etc.)	route_blocked/*.yaml	TestIPRouteAddBlocked, etc.	Covered
Case-sensitive subcommands	route_case_insensitive/show_uppercase.yaml	TestIPRouteSubcmdCaseSensitive	Covered
-6 route blocked	errors/ipv6_route_not_supported.yaml	TestIPIPv6RouteBlocked	Covered
--oneline/--brief route blocked	errors/route_brief_not_supported.yaml, route_oneline_not_supported.yaml	—	Covered
ip route get missing addr	errors/route_get_missing_addr.yaml	TestIPRouteGetMissingAddr	Covered
ip route get leading-zero octet	errors/route_get_leading_zero_octet.yaml	—	Covered
Fuzz: parser + addr parser	—	FuzzIPRouteParse, FuzzIPRouteGetAddr	Covered

Coverage: Adequate. All new code paths are covered by Linux Go tests. Scenario tests cover all error/blocked paths that are portable.

Findings

#	Priority	File	Finding
1		tests/scenarios/cmd/ip/route_case_insensitive/show_uppercase.yaml	Filename says "show_uppercase" but scenario tests "GET" uppercase — minor naming inconsistency

Positive Observations

• The .. component check is applied to the original path before filepath.Clean, which correctly catches traversal sequences like /proc/../etc/passwd that Clean would hide. This is a well-known footgun and it's handled correctly.
• Returning ErrMaxRoutes/ErrMaxTotalLines as hard errors rather than silently truncating is the right design: it prevents LPM from operating on incomplete data and producing a wrong next-hop.
• The IsContiguousMask function correctly handles the little-endian /proc byte order before validating CIDR contiguity — a subtle but important detail.
• Fuzz seed corpus covers CVE-class inputs (ELF/PE/ZIP magic bytes, null bytes, CRLF, very long lines) in addition to the normal test cases.
• AGENTS.md was updated with the AllowedPaths bypass rationale, providing a permanent audit trail for the security trade-off.

[AlexandreYang]

@codex review this PR

[AlexandreYang]

Iteration 2 self-review result: 1 finding (P3/minor). Summary: scenario test file show_uppercase.yaml tests ip route GET (not SHOW) — minor naming inconsistency. No P0/P1/P2 findings.

[AlexandreYang]

Review Summary

This PR adds ip route [show|list] and ip route get ADDRESS subcommands to the existing ip builtin, reading the IPv4 routing table from /proc/net/route via a new builtins/internal/procnetroute package. It also refactors the ss builtin to delegate /proc/net/ I/O to a new builtins/internal/procnetsocket package, and extracts a shared builtins/internal/procpath constant.

Overall assessment: safe to merge. The implementation is well-structured, security-conscious, and thoroughly tested. No P0 or P1 findings.

Summary Table

#	Priority	File	Finding
1		builtins/internal/procnetroute/procnetroute.go:107	.. substring check documents a theoretical false-positive (/proc..backup) but no runtime enforcement of the /proc-prefix invariant
2		builtins/ip/ip.go:558-616	ip route show with a single trailing argument (e.g. ip route show table main) produces a terse unsupported argument error rather than the more informative real ip error; minor UX divergence, intentional but undocumented
3		builtins/tests/ip/ip_linux_test.go	TestIPRouteShowContextCancellation and TestIPRouteGetContextCancellation accept exit 0 or 1 but don't actually cancel the context early — they are timeout tests in name only

Coverage Summary

Code path	Scenario test	Go test	Status
ip route show — default route	—	ip_linux_test.go:TestIPRouteShowDefault	Covered
ip route show — down routes skipped	—	ip_linux_test.go:TestIPRouteShowDownRouteSkipped	Covered
ip route show — reject (unreachable) route	—	ip_linux_test.go:TestIPRouteShowRejectRoute	Covered
ip route show — non-octet CIDR masks (/25, /17, /20, /28)	—	ip_linux_test.go:TestIPRouteShowNonOctetMasks	Covered
ip route show — host route /32 no "/32" suffix	—	ip_linux_test.go:TestIPRouteShowHostRouteNoSlash32	Covered
ip route get — longest-prefix-match	—	ip_linux_test.go:TestIPRouteGetLongestPrefixMatch	Covered
ip route get — metric tie-break	—	ip_linux_test.go:TestIPRouteGetMetricTieBreak	Covered
ip route get — reject route returns unreachable	—	ip_linux_test.go:TestIPRouteGetRejectRouteUnreachable	Covered
Write ops blocked	route_blocked/*.yaml	ip_linux_test.go:TestIPRoute{Add,Del,...}Blocked	Covered
ip -6 route rejected	errors/ipv6_route_not_supported.yaml	ip_linux_test.go:TestIPIPv6RouteBlocked	Covered
ip route SHOW (case-sensitive)	route_case_insensitive/get_uppercase.yaml	ip_linux_test.go:TestIPRouteSubcmdCaseSensitive	Covered
MaxRoutes cap	—	ip_linux_test.go:TestIPRouteMaxRoutesCap	Covered
MaxLineBytes cap (long lines)	—	ip_pentest_linux_test.go:TestIPRoutePentestLongLines	Covered
/dev/zero no-hang	—	ip_pentest_linux_test.go:TestIPRoutePentestDevZero	Covered
Fuzz: parser	—	ip_route_fuzz_linux_test.go:FuzzIPRouteParse	Covered
Fuzz: address parser	—	ip_route_fuzz_linux_test.go:FuzzIPRouteGetAddr	Covered
MaxTotalLines cap	—	—	Not directly tested (P3)

---

Findings

P3 Findings

Finding 1: .. check documents but does not enforce /proc-prefix invariant

Severity: P3 — Hardening / documentation gap

Location: builtins/internal/procnetroute/procnetroute.go:107 and the safety invariant comment at lines 93–105

Description: The ReadRoutes doc-comment notes:

(a) /proc-prefix requirement: all callers MUST pass a path that starts with /proc. No runtime assertion enforces this — tests override procPath with a temp-directory tree...

The .. substring check (line 107) catches traversal sequences but does not enforce the /proc-prefix invariant it documents. In practice, ProcNetRoutePath is a package-level constant initialized to procpath.Default ("/proc") and only overridden in tests, so there is no real attack surface. However, a future caller who passes an arbitrary non-/proc path would bypass the stated invariant silently. This is P3 because the variable is not accessible from user scripts.

Remediation (optional): Consider adding a debug-mode assertion or a //nolint comment linking to the security design decision in AGENTS.md. No urgent action needed.

---

Finding 2: Trailing argument handling for ip route show produces a terse error

Severity: P3 — Minor UX divergence

Location: builtins/ip/ip.go:595-597

Description: ip route show table main (or any unsupported trailing token after show/list) produces ip: route: show: unsupported argument "table" and exits 1, which is safe and correct. The real ip command also rejects this with an error. The divergence is acceptable but is not documented in the scenario tests as an intentional divergence (the scenario test route_brief_not_supported.yaml covers brief/oneline but there is no test for unsupported trailing arguments to show that exercises the exact error message in a bash-comparable way).

Remediation: No action required — the AGENTS.md already documents that ip route happy-path scenario tests cannot be added due to the platform-skip limitation. The error path is already tested in ip_linux_test.go:TestIPRouteShowTrailingArgRejected.

---

Finding 3: Context cancellation tests are timeout-guards, not actual cancellation tests

Severity: P3 — Test quality

Location: builtins/tests/ip/ip_linux_test.go:627-643

Description: TestIPRouteShowContextCancellation and TestIPRouteGetContextCancellation set a 5-second timeout but never cancel the context early. They effectively just verify the operation completes within 5 seconds. A true context cancellation test would cancel the context immediately after starting the command and verify the builtin returns promptly. The TestIPRoutePentestDevZero test provides stronger coverage of the hang-prevention invariant.

Remediation (optional): Consider adding a test that cancels a context mid-execution (e.g. using a large synthetic route file and a goroutine that cancels after a short delay). Not blocking.

---

Positive Observations

• Strong security posture: ProcNetRoutePath is a compile-time constant (procpath.Default = "/proc") only overridden in tests. The sandbox bypass is clearly documented in AGENTS.md, SHELL_FEATURES.md, and inline comments. The .. path guard in ReadRoutes provides defence-in-depth.
• Memory safety: Both MaxRoutes (10 000) and MaxTotalLines (100 000) caps are well-chosen. The MaxLineBytes (1 MiB) per-line cap on the scanner prevents OOM from pathologically long lines. The /dev/zero test verifies the cap terminates correctly.
• No user-controlled paths: ProcNetRoutePath is never derived from shell script input — it flows through builtins/ip/ip.go → procnetroute.ReadRoutes(ctx, ProcNetRoutePath) with ProcNetRoutePath being the fixed /proc constant in production.
• Write operations correctly blocked: All write subcommands (add, del, delete, change, replace, flush, save, restore) are matched in the first switch in routeCmd before any I/O, and covered by scenario tests with skip_assert_against_bash: true.
• Case-sensitive subcommand matching: The PR intentionally makes ip route subcommands case-sensitive to match real iproute2 behavior, with both a Go test and a scenario test.
• Well-structured refactoring: Moving socket-table parsing from ss_linux.go into builtins/internal/procnetsocket is a clean separation of concerns that also lets ip route reuse the procpath constant.
• Good fuzz coverage: FuzzIPRouteParse and FuzzIPRouteGetAddr are registered in the fuzz.yml CI matrix under ./builtins/tests/ip/, ensuring they run on PRs.
• Reject-route handling: The RTF_REJECT flag handling is correct — reject routes are included in ip route show with an unreachable prefix, no dev field, and ip route get returns exit 1 with network unreachable when the best-match route is a reject route rather than silently falling back to a less-specific route.

[AlexandreYang]

@codex review this PR

[AlexandreYang]

Iteration 3 self-review result: 3 findings (all P3/minor). (1) dotdot guard is defence-in-depth only, (2) terse error on unsupported table arg, (3) context cancellation tests are timeout-guards only. No P0/P1/P2 findings. Safe to merge.

[AlexandreYang]

Review Summary

Reviewed the ip route show/get builtin and associated refactoring: procnetroute package creation, procpath centralisation, procnetsocket restructure, and ss table-cap hardening. The implementation is solid overall — all write operations are blocked, the /proc/net/route path is hardcoded and never user-derived, input validation is thorough, and the test suite (unit, pentest, fuzz) is comprehensive.

Overall assessment: safe to merge with one minor correctness fix (P2) and two P3 suggestions.

---

Summary Table

#	Priority	File	Finding
1		builtins/ip/ip.go:629	Context cancellation in routeShow output loop returns exit 1; bash ip route show exits 0 after partial output
2		builtins/ip/ip.go:596	Error message format inconsistency between route show trailing-arg error and route get trailing-arg error
3		ip_linux_test.go	ErrMaxTotalLines code path has no direct test

---

Findings

F1 — Context cancellation in routeShow returns exit 1 (bash divergence)

Severity: P2 — Bash Compatibility
Location: builtins/ip/ip.go:626-631

Description: When the shell execution context is cancelled (timeout) while routeShow is iterating through already-read route entries, it returns Code: 1. Real ip route show exits 0 regardless of how much output was written before interruption. This diverges from bash semantics.

The test TestIPRouteShowContextCancellation asserts code == 0 || code == 1 — a wide assertion that masks the divergence.

// Current (diverges from bash):
if ctx.Err() != nil {
    return builtins.Result{Code: 1}
}

Note: the ReadRoutes inner loop also checks ctx.Err() and returns an error if cancelled during reading — that correctly becomes exit 1 (I/O incomplete). The divergence is only in the output loop after routes are already loaded.

Remediation: Change Code: 1 to Code: 0 (i.e. return builtins.Result{}) in the output loop's cancellation guard. This matches how runAddr/runLink handle ctx.Err() — they break and return empty Result{}.

---

F2 — Error message format inconsistency for trailing-arg rejection

Severity: P3 — Cosmetic
Location: builtins/ip/ip.go:596

Description: route show trailing-arg errors use "ip: route: %s: unsupported argument" while route get uses "ip: route get: unsupported argument". Minor inconsistency in the error namespace prefix.

Remediation: Change to "ip: route %s: unsupported argument %q\n", sub, args[1] for a uniform "ip: route show:" / "ip: route list:" / "ip: route get:" prefix pattern.

---

F3 — ErrMaxTotalLines path untested

Severity: P3 — Test Coverage
Location: builtins/tests/ip/ip_linux_test.go

Description: The existing TestIPRouteMaxRoutesCap uses 15,000 identical UP-route rows, which hits ErrMaxRoutes before ErrMaxTotalLines. The MaxTotalLines scan-time guard (triggered when >100,000 non-UP/malformed lines appear before reaching MaxRoutes UP entries) is untested. A file with >100,000 non-UP rows and zero UP rows would trigger it.

Remediation: Add a test that writes MaxTotalLines + 1 non-UP rows (flags=0x0000) and asserts exit 1 with "MaxTotalLines" in stderr.

---

Coverage Summary

Code path	Scenario test	Go test	Status
route show / route list happy path	— (Linux-only, no platform skip in scenario fw)	ip_linux_test.go	Covered
route get ADDRESS LPM	—	TestIPRouteGetLongestPrefixMatch, MetricTieBreak, HostRoute	Covered
Write ops blocked	route_blocked/*.yaml	ip_linux_test.go	Covered
Unknown subcommand	errors/route_unknown_subcmd.yaml	✓	Covered
IPv6 route rejected	errors/ipv6_route_not_supported.yaml	✓	Covered
RTF_REJECT route in show	—	TestIPRouteShowRejectRoute	Covered
RTF_REJECT route in get → unreachable	—	TestIPRouteGetRejectRouteUnreachable	Covered
MaxRoutes cap	—	TestIPRouteMaxRoutesCap	Covered
MaxTotalLines cap	—	—	Missing (P3)
Non-octet-aligned CIDR masks (/25, /17, etc.)	—	TestIPRouteShowNonOctetMasks	Covered
/dev/zero hang protection	—	TestIPRoutePentestDevZero	Covered
Fuzz: parser + address	—	FuzzIPRouteParse, FuzzIPRouteGetAddr	Covered
Context cancel → exit code	—	TestIPRouteShowContextCancellation (asserts 0 OR 1)	Partially covered (P2, F1)

---

Positive Observations

• Sandbox bypass is fully documented at every layer: package doc, ReadRoutes godoc, AGENTS.md Security Design Decisions section, and inline comments. Auditable and intentional.
• Two-layer path safety: strings.Contains(procPath, "..") check on the original path prevents traversal before filepath.Clean normalises it. The doc correctly explains why Clean is applied after, not before, the check.
• All write operations blocked with precise error messages, covered by dedicated scenario tests (route_blocked/*.yaml, all with skip_assert_against_bash: true).
• Double-bounded resource exhaustion: MaxRoutes (memory guard) + MaxTotalLines (scan-time guard), both resulting in hard errors — correct choice since silent truncation would produce incorrect LPM results.
• IsContiguousMask correctness: the little-endian → network-byte-order byte-swap before CIDR validation is correct. TestIPRouteShowNonOctetMasks validates /25, /17, /20, /28 edge cases.
• ProcNetRoutePath concurrency contract is clearly documented with mutex, warning against t.Parallel(), and comments explaining the production-immutability invariant.
• GTFOBins vectors blocked and tested: -b/-B/-batch, --force, -n/--netns, ip netns all exit 1 with clear messages, covered at both unit and integration test levels.

[AlexandreYang]

@codex review this PR

[AlexandreYang]

Iteration 4 self-review result: 3 findings — 1 P2 (routeShow returns Code:1 on context cancel instead of 0), 2 P3 (error message inconsistency, ErrMaxTotalLines lacks dedicated test). Safe to merge.

[AlexandreYang]

Review Summary

Scope: ip route [show|list] and ip route get ADDRESS subcommands, builtins/internal/procnetroute package, refactored builtins/internal/procnetsocket package, associated tests, and documentation.

Overall assessment: safe to merge. The implementation is carefully designed with multiple defence-in-depth layers: the ..-traversal guard on procPath, a per-line memory cap (MaxLineBytes), a route-table memory cap (MaxRoutes), a CPU-time guard (MaxTotalLines), context-cancellation checks in every loop, and explicit hard-errors (rather than silent truncation) when caps are hit. All write operations are blocked. GTFOBins vectors are rejected. The AllowedPaths sandbox bypass is intentional, correctly constrained, and documented in both the package doc and AGENTS.md.

No P0, P1, or P2 findings. Three P3 observations (minor hardening / clarity) are left as inline comments.

#	Priority	File	Finding
1		builtins/internal/procnetroute/procnetroute.go:160	Equal-cost multipath (equal prefix length + equal metric): first-match-wins is undocumented
2		builtins/tests/ip/ip_route_fuzz_linux_test.go:44	writeFuzzRoute: early-return path skips locking, but the returned no-op cleanup is never called — minor clarity issue
3		builtins/ip/ip.go:558	routeCmd: `do.oneline

Positive Observations

• Sandbox bypass is constrained and documented. ReadRoutes calls os.Open directly but procPath is hardcoded to /proc in production, the .. guard fires on the original (pre-filepath.Clean) path to catch traversal sequences, and the intent is documented in the package doc, inline, AGENTS.md, and SHELL_FEATURES.md.
• Hard-error on cap violations. Returning ErrMaxRoutes / ErrMaxTotalLines instead of silently truncating ensures callers (and users) know that LPM results may be incorrect when the route table is abnormally large.
• Context cancellation at every loop iteration. Both readRoutes and routeShow check ctx.Err() in their scan loops, preventing hangs on a slow or infinite /proc/net/route.
• /dev/zero protection. MaxLineBytes (1 MiB) causes bufio.ErrTooLong before unbounded memory can be allocated, and the pentest test (TestIPRoutePentestDevZero) verifies this.
• Comprehensive test coverage. Unit tests, pentest tests, fuzz tests, and YAML scenario tests cover the happy path, all error paths, boundary conditions, adversarial inputs, and context cancellation.
• Write operations all blocked with clear errors. add, del, delete, change, replace, flush, save, restore all exit 1 with a descriptive message.

[AlexandreYang]

@codex review this PR

[AlexandreYang]

Iteration 5 self-review result: 3 findings (all P3/low). (1) equal-cost multipath first-match-wins undocumented, (2) fuzz helper lacks clarifying comment, (3) oneline/brief rejection on default subcommand path is uncommented. No P0/P1/P2 findings. Safe to merge.

[AlexandreYang]

Review-Fix Loop Summary

• PR: feat(ip): add ip route show/get builtin (Linux only) #135 (feat(ip): add ip route show/get builtin (Linux only) #135)
• Iterations completed: 5
• Final status: CLEAN

Iteration log

#	Unresolved threads	Fixes applied	CI status
1	0	4 fixed (error colon, dead allowlist entry, clarifying comments, scenario test)	Passing (34 SUCCESS)
2	0	1 fixed (renamed show_uppercase.yaml → get_uppercase.yaml)	Passing (34 SUCCESS)
3	0	1 fixed (	+ block scalars in 18 scenario files) + 6 codex threads resolved
4	0	2 fixed (routeShow ctx cancel exit code, route show/list error prefix)	Passing (34 SUCCESS)
5	0	3 fixed (clarifying comments for tie-break, fuzz cleanup, oneline flag)	Passing (34 SUCCESS)

Final state

• Unresolved threads: 0
• CI: All 34 checks SUCCESS

Remaining issues

None.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 866e5fb50a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Remove bash-skip from case-sensitive route scenario

This scenario is marked skip_assert_against_bash: true even though its own description says the behavior should match real iproute2 (uppercase GET rejected). Per /workspace/rshell/AGENTS.md, skip_assert_against_bash: true should only be used for intentional bash divergences; keeping the skip here disables the required byte-for-byte bash check and can let future compatibility regressions in ip route subcommand parsing slip through unnoticed.

Useful? React with 👍 / 👎.

[AlexandreYang]

[Claude Sonnet 4.6] Fixed. The skip was needed because our error message ("ip: route: GET: unknown subcommand", exit 1) diverged from iproute2 ("Command "GET" is unknown, try "ip route help".", exit 255). Rather than keep the skip, I updated the implementation to match iproute2 exactly: unknown route subcommands now emit the iproute2-format error and exit 255. Updated get_uppercase.yaml and route_unknown_subcmd.yaml to use the new expected output, removed both skips, and updated the Go unit tests accordingly. Also updated route_token_as_subcmd.yaml which tests the same code path.