Conversation
Refactor snippet import handling to support import_N and args. Enhance parsing of named matchers, handle blocks, and transport configurations.
📝 WalkthroughWalkthroughAdds a snippet import/substitution system and expands route-building: named matcher and handle_N parsing, per-prefix transport/header extraction, handler builders (headers/respond), remote-IP parsing, per-handle route generation, and a GitHub Actions workflow to build/publish the container image. Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration
participant ImportParser as parse_imports()
participant SnippetStore as Snippet Registry
participant Substituter as substitute_import_args()
participant ImportApplier as apply_snippet_imports()
participant RouteBuilder as Route Builder (parse_container_labels / parse_handle_blocks)
participant HandlerBuilder as Handlers (header/respond / transport/header per-prefix)
Config->>ImportParser: extract import / import_N directives
ImportParser-->>Config: ordered snippet-name/args pairs
ImportApplier->>SnippetStore: lookup snippet definitions
SnippetStore-->>Substituter: snippet values
Substituter-->>ImportApplier: values with {args[N]} substituted
ImportApplier->>RouteBuilder: merge substituted directives into route config
RouteBuilder->>HandlerBuilder: construct per-handle handlers and apply per-prefix transport/headers
RouteBuilder-->>Config: emit final route entries
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@caddy-agent-watch.py`:
- Around line 445-474: The apply_snippet_imports function logs raw snippet args
and merged config which may contain secrets; update logging to avoid printing
sensitive data by (1) changing the logger.info call that currently includes args
to only log non-sensitive context (e.g., snippet_name and route_num) and remove
args from the message, (2) remove or replace the logger.debug that dumps the
full config/merged_config with either a redacted summary (e.g., log only keys,
not values) or a masked/hashed representation, and (3) ensure any warnings
(e.g., snippet not found) do not include args or config values; update
references in apply_snippet_imports to stop emitting sensitive variables
(snippet_name, args, merged_config, config) in logs and, if needed, implement a
small redact_keys helper to mask known secret keys before logging keys-only
summaries.
- Around line 827-829: The current loop sorting handle_blocks by
int(k.split('_')[1]) can raise ValueError for non-numeric suffixes; update the
sort key used in the for loop over handle_blocks to a safe extractor (e.g., a
small helper/inline lambda) that tries to parse the numeric suffix after the
underscore and returns a tuple such as (0, number) when numeric or (1,
original_suffix_or_key) when not numeric so non-numeric keys sort
deterministically instead of crashing; modify the sorting expression where
handle_key is computed (the for handle_key in sorted(handle_blocks.keys(), ... )
line) to use this safe key so block and block_directives processing continues
even with malformed labels.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@caddy-agent-watch.py`:
- Around line 562-582: In parse_respond_handler, the handler currently treats a
single numeric token as the body; change the parsing so that when parts has one
element and parts[0].isdigit() you set status_code = int(parts[0]) and body = ""
(empty string); otherwise preserve the existing behavior where body = parts[0]
and if parts[1].isdigit() set status_code from parts[1]. This fixes the
status-only case (e.g., "respond 404") while keeping shlex and the fallback
split logic intact.
🧹 Nitpick comments (3)
caddy-agent-watch.py (3)
436-448: Consider logging when arg placeholder index is out of bounds.When
{args[N]}references an index beyond the provided args, the function silently returns an empty string. This could hide configuration errors where a snippet expects more arguments than provided.💡 Optional: Add debug logging for missing args
def repl(match): idx = int(match.group(1)) - return args[idx] if idx < len(args) else "" + if idx < len(args): + return args[idx] + logger.debug(f"Arg placeholder {{args[{idx}]}} has no corresponding value (only {len(args)} args provided)") + return ""
619-668: Consider extracting shared logic withparse_header_config.This function duplicates ~50 lines of header parsing logic from
parse_header_config(lines 1019-1099). Both handle the sameheader_up/header_downparsing with identical delete/set logic.♻️ Suggested approach to reduce duplication
Extract a common helper that both functions can use:
def _parse_header_value(value, target_dict, direction): """Helper to parse header_up/header_down value into target dict.""" if value.startswith('-'): header_name = value[1:].strip() if 'delete' not in target_dict: target_dict['delete'] = [] target_dict['delete'].append(header_name) else: clean_value = value[1:].strip() if value.startswith('+') else value parts = clean_value.split(None, 1) if len(parts) == 2: header_name, header_value = parts[0], parts[1].strip('"') if 'set' not in target_dict: target_dict['set'] = {} target_dict['set'][header_name] = [header_value] def parse_header_config_for_prefix(config, prefix): headers_config = {} for key, value in config.items(): if key.startswith(f"{prefix}.header_up"): # ... use _parse_header_value for 'request' elif key.startswith(f"{prefix}.header_down"): # ... use _parse_header_value for 'response' return headers_configThen
parse_header_configcan delegate:return parse_header_config_for_prefix(config, 'reverse_proxy')
821-901: Nested function captures loop variables - consider refactoring.The
build_routes_for_domainsfunction captures multiple outer variables (https_domains,http_only_domains,handle_blocks,config,named_matchers,route_num,base_route_id,domain) which triggers B023 late-binding warnings. While this works correctly because the function is called immediately within the same loop iteration, it makes the code harder to reason about and fragile if refactored.Note: The
handle_sort_keysafe sorting (lines 831-835) correctly addresses the previous review concern about ValueError on malformed handle keys.♻️ Option 1: Pass captured variables as parameters
- def build_routes_for_domains(domains, http_only_flag): + def build_routes_for_domains(domains, http_only_flag, *, + handle_blocks, named_matchers, config, + base_route_id, route_num, domain, + https_domains, http_only_domains): if not domains: return # ... rest of function uses parameters instead of closures - build_routes_for_domains(http_only_domains, True) - build_routes_for_domains(https_domains, False) + build_routes_for_domains(http_only_domains, True, + handle_blocks=handle_blocks, named_matchers=named_matchers, + config=config, base_route_id=base_route_id, route_num=route_num, + domain=domain, https_domains=https_domains, http_only_domains=http_only_domains) + build_routes_for_domains(https_domains, False, ...)♻️ Option 2: Extract to module-level function
Move
build_routes_for_domainsoutsideparse_container_labelsas a module-level helper function, passing all required context as parameters. This improves testability and eliminates the closure issue entirely.
| def parse_respond_handler(value): | ||
| import shlex | ||
|
|
||
| try: | ||
| parts = shlex.split(value) | ||
| except Exception: | ||
| parts = value.split() | ||
|
|
||
| if not parts: | ||
| return None | ||
|
|
||
| body = parts[0] | ||
| status_code = 200 | ||
| if len(parts) > 1 and parts[1].isdigit(): | ||
| status_code = int(parts[1]) | ||
|
|
||
| return { | ||
| "handler": "static_response", | ||
| "body": body, | ||
| "status_code": status_code | ||
| } |
There was a problem hiding this comment.
Bug: Status-only respond directive parsed incorrectly.
When the respond value is a status code only (e.g., respond 404), the current logic incorrectly treats it as body="404" with status=200. In Caddyfile syntax, respond 404 should return status 404 with an empty body.
🐛 Proposed fix to handle status-only respond
def parse_respond_handler(value):
import shlex
try:
parts = shlex.split(value)
except Exception:
parts = value.split()
if not parts:
return None
- body = parts[0]
- status_code = 200
- if len(parts) > 1 and parts[1].isdigit():
- status_code = int(parts[1])
+ # Handle status-only: respond 404
+ if len(parts) == 1 and parts[0].isdigit():
+ return {
+ "handler": "static_response",
+ "status_code": int(parts[0])
+ }
+
+ body = parts[0]
+ status_code = 200
+ if len(parts) > 1 and parts[1].isdigit():
+ status_code = int(parts[1])
return {
"handler": "static_response",
"body": body,
"status_code": status_code
}🧰 Tools
🪛 Ruff (0.14.14)
[warning] 567-567: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@caddy-agent-watch.py` around lines 562 - 582, In parse_respond_handler, the
handler currently treats a single numeric token as the body; change the parsing
so that when parts has one element and parts[0].isdigit() you set status_code =
int(parts[0]) and body = "" (empty string); otherwise preserve the existing
behavior where body = parts[0] and if parts[1].isdigit() set status_code from
parts[1]. This fixes the status-only case (e.g., "respond 404") while keeping
shlex and the fallback split logic intact.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/publish-container-gh.yml:
- Line 19: Update the GitHub Actions step that uses actions/checkout@v3 to the
current stable release by replacing the usage reference with
actions/checkout@v6; locate the line containing "uses: actions/checkout@v3" in
the workflow and change it to "uses: actions/checkout@v6" to ensure the action
runs on supported GitHub Actions runtimes.
| packages: write # Required to push to GHCR | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v3 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
actions/checkout v3 deprecated GitHub Actions latest version 2025
💡 Result:
- Yes—
actions/checkout@v3is effectively deprecated/legacy because it was built for the Node.js 16 GitHub Actions runtime, and GitHub completed the platform transition away from Node 16. GitHub began enforcing Node 20 by default on June 3, 2024 [2], and removed Node 16 from the Actions runner on November 12, 2024, so Node 16–based actions can no longer run [3]. - Latest
actions/checkoutmajor version (as of Feb 2026):v6. GitHub’s official repo/Marketplace docs show Checkout v6 as the current major version [1]. - What to use in 2025+ workflows: switch to a maintained major tag, typically:
(Pinning to the major tag is the recommended pattern for GitHub-maintained actions.) [1]
- uses: actions/checkout@v6
Sources: official actions/checkout repo/Marketplace page [1], GitHub Actions changelog (Node20 enforcement + Node16 end-of-life) [2][3].
Update deprecated actions/checkout@v3 to latest version to fix workflow failure.
actions/checkout@v3 is deprecated because it relies on the Node.js 16 runtime, which GitHub removed from Actions runners on November 12, 2024. The workflow will fail at runtime without an update. The latest stable version is v6.
🧰 Tools
🪛 actionlint (1.7.10)
[error] 19-19: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In @.github/workflows/publish-container-gh.yml at line 19, Update the GitHub
Actions step that uses actions/checkout@v3 to the current stable release by
replacing the usage reference with actions/checkout@v6; locate the line
containing "uses: actions/checkout@v3" in the workflow and change it to "uses:
actions/checkout@v6" to ensure the action runs on supported GitHub Actions
runtimes.
1 participant
Refactor snippet import handling to support import_N and args. Enhance parsing of named matchers, handle blocks, and transport configurations.
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.