[refactor] modular redirectors

[KCarretto]

Refactor our redirector package to be more modular, allowing multiple redirector transports to be registered. This registration system is modeled after Golang's db/sql driver registration. Also makes the docs a bit more clear.

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Prepares us to implement a gRPC redirector

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ 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]

Order redirector CLI arguments correctly

The redirector command documents its arguments as [upstream_address] [transport=http1] [listen_on=8080], but the implementation reads the second argument into listenOn and the third into transport. Anyone following the help text and running tavern redirector example.com grpc :9090 will instead try to start a redirector named ":9090" and bind to "grpc", which fails with "redirector ":9090" not found". Either the assignments should be swapped or the help text changed so that transport can actually be set via the second argument.

Useful? React with 👍 / 👎.

[hulto]

@claude Take a look?

[claude]

Claude finished @hulto's task —— View job

---

Code Review

I've reviewed this PR and overall the refactoring looks solid! The modular redirector design following Go's database/sql driver pattern is clean and well-executed. Here are my findings:

---

Positive Highlights

1. Excellent architecture - The registration pattern with Register(), List(), and Run() in tavern/internal/redirectors/redirector.go:24-68 is clean and extensible. This will make adding new transports straightforward.

2. Good separation of concerns - Moving HTTP/1.1 specific code into tavern/internal/redirectors/http1/ makes the codebase more maintainable.

3. Net reduction in code - 1172 deletions vs 392 additions shows this is truly simplifying things.

4. Comprehensive testing - The test coverage in tavern/internal/redirectors/http1/grpc_frame_test.go:1-247 is thorough with edge cases well tested.

5. Clean shared utilities - The ConnectToUpstream() function in tavern/internal/redirectors/grpc_upstream.go:16-63 centralizes upstream connection logic nicely.

---

Critical Issue: CLI Argument Mismatch ❗

The automated review correctly identified a serious bug in tavern/app.go:67-96. The code contradicts its own documentation:

// Line 67: Documentation says:
ArgsUsage: "[upstream_address]",

// Lines 69-78: Flags defined as --listen and --transport
cli.StringFlag{
    Name:  "listen",
    Usage: "Address to listen on for incoming redirector traffic (default: :8080)",
    Value: ":8080",
},
cli.StringFlag{
    Name:  "transport",
    Usage: "Transport protocol to use for redirector (default: http1)",
    Value: "http1",
},

The implementation correctly uses named flags (c.String("listen") and c.String("transport")), which is the right approach. The old ArgsUsage documentation appears to be a leftover from the previous implementation.

Impact: This is confusing for users, but not a runtime bug since the flags work correctly. The ArgsUsage should be updated to reflect that listen and transport are now proper flags, not positional arguments.

Recommendation: Update line 67 to:

ArgsUsage: "[upstream_address]",
Usage:     "Run a redirector connecting agents using a specific transport to the server (use --listen and --transport flags)",

---

Minor Issues

1. Typo in comment - tavern/internal/redirectors/redirector.go:25

   // "If Register is called twice with the same name or if driver is nil,it panics."
   // Should be: "... or if driver is nil, it panics." (add space after comma)

2. Inconsistent default port - I noticed commit 53858e3 changed the default listen address to 8000, but the flag default in tavern/app.go:72 is still :8080. Is this intentional?

3. Error message inconsistency - tavern/app.go:87 says "gRPC upstream address is required" but the upstream could be any protocol (HTTP, HTTPS). Consider: "upstream address is required (first argument)"

---

Code Quality Observations

1. Good use of mutex locking - The sync.RWMutex usage in tavern/internal/redirectors/redirector.go:15-42 correctly uses RLock for reads and Lock for writes.

2. Proper resource cleanup - The deferred upstream.Close() in tavern/internal/redirectors/redirector.go:60-63 ensures connections are cleaned up.

3. Well-structured HTTP handlers - The helper functions in tavern/internal/redirectors/http1/http.go:19-65 make the main handler logic much cleaner.

---

Testing Recommendations

1. Add integration tests - Consider adding tests that verify the full redirector registration and startup flow, not just unit tests for frame handling.

2. Test CLI flag parsing - Add tests to verify the CLI flags work as expected (this would have caught the documentation mismatch).

---

Documentation

The docs update in docs/_docs/user-guide/imix.md:99 correctly documents the HTTP/1.1 redirector feature flag. Good job keeping docs in sync!

The AGENTS.md update clarifies project structure nicely.

---

Summary

This is a high-quality refactoring that successfully makes the codebase more modular. The main issue is the CLI documentation mismatch which should be fixed before merge. Everything else is minor polish.

Recommendation: Approve after fixing the ArgsUsage documentation.

---

[hulto]

Update the docs that reference http://localhost
Update the agent to default to 8080 as well