Implement basic runes and services installation - #3
Conversation
- `prepare`: minimal needed packages for NullForge to work - `base`: basic system configuration - `containers`: container management - `dns`: DoH/DoT/DoU resolver configuration - `haproxy`: HAProxt installation - `monitoring`: monitoring system configuration - `netsec`: network security policies - `profiles`: user profiles management - `users`: user management and SSH keys management - `telemt`: Telemt proxy installation - `tor`: Tor proxy installation - `warp`: Cloudflare Warp installation - `xray`: Xray-Core installation - `zerotrust`: Cloudflare ZeroTrust proxy installation
Release-As: 0.1.0
|
Warning Review limit reached
Next review available in: 24 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 (99)
📝 WalkthroughWalkthroughNullForge adds a pyinfra-based infrastructure-as-code framework with validated configuration molds, deployment runes, reusable provisioning helpers, systemd and shell templates, example inventory data, and project documentation. ChangesNullForge framework
Estimated code review effort: 5 (Critical) | ~180 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
github-actions
Bot
added
docs
4ccf111 to
d44ecad
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (18)
nullforge/molds/profiles.py-11-14 (1)
11-14: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake
for_rootindependent of managed-user creation.
nullforge/runes/profiles.py:48-61currently requiresuser_opts.managebefore adding("root", "/root"). Thusfor_root=Truedoes nothing when the regular user is unmanaged, despite this field’s contract. Change that condition toif profiles_opts.for_root:.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/molds/profiles.py` around lines 11 - 14, Update the profile installation condition in the profiles handling logic around user_opts.manage so the root profile entry is added whenever profiles_opts.for_root is true, independently of managed-user creation; preserve the existing managed-user handling separately.nullforge/molds/haproxy.py-13-16 (1)
13-16: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor
versionon RHEL or reject unsupported version selection.
nullforge/runes/haproxy.py:25-103usespackages=["haproxy"]on RHEL, soversion="3.2"is ignored there. Either provision the requested version on RHEL or validate/document that version pinning is Debian-only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/molds/haproxy.py` around lines 13 - 16, Update the HAProxy version handling across the model field `version` and the provisioning logic in `haproxy` so RHEL does not silently ignore a requested version: either install the selected version on RHEL or validate the platform and reject/document version pinning as Debian-only, while preserving the existing Debian behavior.nullforge/molds/user.py-21-24 (1)
21-24: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact
passwordfrom debug serialization.
UserMoldinheritsBaseMold.to_json()but does not declarepasswordsensitive, exposing a configured password in pyinfra debug inventory. Add_sensitive_fields: ClassVar[tuple[str, ...]] = ("password",).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/molds/user.py` around lines 21 - 24, Update UserMold to declare the inherited sensitivity metadata by adding _sensitive_fields as a ClassVar tuple containing "password", so BaseMold.to_json() redacts the password during debug serialization.nullforge/molds/containers.py-15-18 (1)
15-18: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject configuration values without a deployment implementation. Both molds accept feature values whose corresponding runes immediately raise, allowing validation to succeed before the cast fails.
nullforge/molds/containers.py#L15-L18: rejectContainersBackendType.CRIOwhileinstall=True, or implement CRI-O deployment.nullforge/molds/dns.py#L16-L19: rejectDnsMode.DOU, or implement the DNS-over-UDP deployment path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/molds/containers.py` around lines 15 - 18, Reject unsupported deployment configurations in the mold validators: in nullforge/molds/containers.py:15-18, disallow ContainersBackendType.CRIO when install=True unless its deployment rune is implemented; in nullforge/molds/dns.py:16-19, disallow DnsMode.DOU unless the DNS-over-UDP deployment path is implemented. Ensure validation fails before casting reaches the unsupported runes.nullforge/molds/monitoring/nezha.py-83-89 (1)
83-89: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not allow dashboard PATs over HTTP.
Line 87 accepts
http://, while the deployment path sendsapi_tokentodashboard_urlfor API calls. Require HTTPS so a valid remote configuration cannot transmit the PAT in cleartext.Proposed fix
- if v and not v.startswith(("https://", "http://")): - raise ValueError("dashboard_url must start with http:// or https://") + if v and not v.startswith("https://"): + raise ValueError("dashboard_url must start with https://")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/molds/monitoring/nezha.py` around lines 83 - 89, Update _validate_dashboard_url to reject any non-empty dashboard_url that does not use https://, removing http:// from the accepted schemes while preserving trailing-slash normalization and empty-value handling.Source: Linters/SAST tools
nullforge/templates/systemd/dns-internal.service.j2-10-10 (1)
10-10: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRender IPv6-aware DNS listen endpoints.
listen_addressacceptsIPvAnyAddress, sodns-internal.service.j2:10can renderip addr add <IPv6>/32 ..., andblocky.yaml.j2:8can emit invalidipv6:53instead of a bracketed endpoint. Either enforce IPv4-only or make the templates family-aware for/32vs IPv6 prefix andaddr:53vs[addr]:53.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/templates/systemd/dns-internal.service.j2` at line 10, Update the DNS endpoint templates to handle both IPv4 and IPv6 listen addresses: use the appropriate address prefix length in the systemd service command and bracket IPv6 addresses when rendering the Blocky DNS endpoint. Preserve the existing IPv4 output and base the family-aware formatting on the configured listen_address value.nullforge/templates/dns/resolved.conf.j2-3-4 (1)
3-4: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRender DoT upstreams for
resolved.conf
_deploy_dot_resolvedresolves upstreams but only passesDOT/DOHtodns/resolved.conf.j2, soDNS=stays hard-coded to Cloudflare even whenupstream_provideris Google or Quad9. Pass a formatted upstream list into the template (e.g.,tcp-tls:[host]:port/host#sniequivalents) so the selected DNS provider and IPv6 setting affects this path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/templates/dns/resolved.conf.j2` around lines 3 - 4, Update _deploy_dot_resolved to pass the resolved upstream list to dns/resolved.conf.j2, formatting each DoT server as the template’s expected host#SNI or tcp-tls:[host]:port representation and preserving IPv4/IPv6 handling. Replace the hard-coded Cloudflare DNS= value in the template with the provided upstream variable so upstream_provider and IPv6 configuration determine the rendered resolvers.nullforge/templates/systemd/cloudflare-tunnel.service.j2-12-14 (1)
12-14: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPass the configured WARP interface to
zt-tunnel-warp.sh.
WarpMold.ifaceis renderable fromwarp.py, but the Zero Trust systemd hooks invoke the script with onlyup/down, sozt-tunnel-warp.shfalls back towarpeven when the deployed WARP interface is customized. Add and pass the iface argument here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/templates/systemd/cloudflare-tunnel.service.j2` around lines 12 - 14, Update the ROUTE_THROUGH_WARP systemd hooks in the service template to pass the configured WarpMold.iface value to zt-tunnel-warp.sh for both the up and down actions, preserving the existing WORKDIR and action arguments.nullforge/templates/cloudflared/tunnel.yml.j2-1-3 (1)
1-3: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRender the supplied
POST_QUANTUMoption.
_deploy_tunnel()passesPOST_QUANTUM=opts.post_quantum, butnullforge/templates/cloudflared/tunnel.yml.j2only emitstoken,protocol, andha-connections, so enabling strict post-quantum mode is silently ignored. Cloudflare supports this incloudflared tunnel runconfiguration aspost-quantum: true.Proposed fix
token: {{ TOKEN }} protocol: {{ PROTOCOL }} ha-connections: {{ HA_CONNECTIONS }} +post-quantum: {{ POST_QUANTUM }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/templates/cloudflared/tunnel.yml.j2` around lines 1 - 3, Update the cloudflared tunnel template to emit the supplied POST_QUANTUM value using the post-quantum configuration key, alongside token, protocol, and ha-connections, so _deploy_tunnel()’s opts.post_quantum setting is applied.nullforge/templates/systemd/cloudflare-tunnel.service.j2-3-4 (1)
3-4: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd systemd ordering/dependency for the WARP service.
When
ROUTE_THROUGH_WARPis enabled,cloudflare-tunnel.servicerunszt-tunnel-warp.shimmediately aftercloudflaredstarts, but that script only replaces routes via its WARP interface and does not wait for/start the WARP unit. Add the appropriate WARP unit to bothRequires=/Wants=andAfter=incloudflare-tunnel.service.j2, or make the hook wait/retry until the configured interface is ready.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/templates/systemd/cloudflare-tunnel.service.j2` around lines 3 - 4, Add the configured WARP systemd unit to the cloudflare-tunnel service dependencies by including it in both Wants/Requires and After alongside network-online.target. Preserve the existing network dependency and ensure the WARP unit is started and ordered before cloudflared runs zt-tunnel-warp.sh.nullforge/templates/systemd/telemt.service.j2-17-30 (1)
17-30: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not grant
CAP_NET_ADMINto the proxy daemon.The privileged
ExecStartPre=+helpers configure routing and iptables rules before the daemon starts; the long-running Telemt process should inherit onlyCAP_NET_BIND_SERVICEunless Telemt itself changes host network state after startup.Proposed fix
-AmbientCapabilities=CAP_NET_ADMIN CAP_NET_BIND_SERVICE -CapabilityBoundingSet=CAP_NET_ADMIN CAP_NET_BIND_SERVICE +AmbientCapabilities=CAP_NET_BIND_SERVICE +CapabilityBoundingSet=CAP_NET_BIND_SERVICE🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/templates/systemd/telemt.service.j2` around lines 17 - 30, Remove CAP_NET_ADMIN from the telemt.service.j2 AmbientCapabilities and CapabilityBoundingSet declarations, retaining only CAP_NET_BIND_SERVICE for the long-running proxy daemon. Leave the privileged ExecStartPre and ExecStopPost helper commands unchanged.nullforge/templates/scripts/warp-v6-policy.sh-16-38 (1)
16-38: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPrevent generated table ID and rule preference collisions.
_slot()maps interface names into 100 slots, so different interfaces can get the sameTID/TABLEand rulePRIO. In theuppath this can let one interface add rules/routes into another interface’s shared routing-table ID, anddowncan flush that table withip -6 route flush table "$TABLE"even though the name differs. Use unique persisted IDs/prefs or fail before applying rules when either value is already owned by another interface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/templates/scripts/warp-v6-policy.sh` around lines 16 - 38, Replace the 100-slot hash-based allocation in _slot, TID, and PRIO with collision-safe persisted ownership or validation before applying rules. Ensure each interface’s table ID and rule preference are unique, and make the up path fail before adding rules/routes when either value is owned by another interface; preserve explicit TID/PRIO handling while validating those values as well.nullforge/molds/utils.py-26-46 (1)
26-46: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPartial-dict layers should not reset unspecified fields to model defaults.
In
_to_features_fragmentand_to_system_dict, a dict layer is validated against the full mold, so missing fields are populated with class defaults.merge_*then deep-merges that fully populated fragment, overwriting existing values for fields the caller did not set in that layer.🛠️ Proposed fix
case Mapping(): prepared = {k: (v.model_dump() if isinstance(v, ALLOWED_FEATURES_LAYERS) else v) for k, v in value.items()} - return FeaturesMold.model_validate(prepared).model_dump() + return FeaturesMold.model_validate(prepared).model_dump(exclude_unset=True)case Mapping(): - return SystemMold.model_validate(value).model_dump() + return SystemMold.model_validate(value).model_dump(exclude_unset=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/molds/utils.py` around lines 26 - 46, Update _to_features_fragment and _to_system_dict so mapping layers preserve only fields explicitly supplied by the caller. Avoid validating partial dictionaries through the full mold in a way that populates default values; validate supplied values while retaining the partial shape, then let merge_* deep-merge without overwriting unspecified existing fields.nullforge/models/dns.py-15-19 (1)
15-19: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not expose a mode that always aborts deployment.
DnsMode.DOUis accepted here, butnullforge/runes/dns.py:45-46unconditionally raises for it. Remove it until implemented, or reject it during mold validation before a host run begins.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/models/dns.py` around lines 15 - 19, Remove DnsMode.DOU from the DnsMode enum until its deployment behavior is implemented, or add mold validation that rejects it before any host run begins; ensure the unsupported mode cannot reach the unconditional failure in the DNS rune.nullforge/models/monitoring/__init__.py-3-12 (1)
3-12: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the discriminator from the single-variant alias.
MonitoringLayoutaliases one concrete model, butField(discriminator="type")makes Pydantic treat it like the root of a discriminated union. If it is used in a Pydantic model, schema generation can fail before construction; extend this to a union when another layout is added.Proposed fix
-from typing import Annotated - -from pydantic import Field - from .base import MonitoringBackendType, _MonitoringLayoutBase from .nezha import NezhaLayout -MonitoringLayout = Annotated[NezhaLayout, Field(discriminator="type")] +MonitoringLayout = NezhaLayout🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/models/monitoring/__init__.py` around lines 3 - 12, Update the MonitoringLayout alias to reference NezhaLayout directly without Annotated or Field(discriminator="type"), while preserving the existing alias name and its future-union intent. Remove any now-unused typing or Pydantic imports from this module.nullforge/smithy/install.py-123-148 (1)
123-148: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPredictable
/tmppaths for root-privileged operations (symlink/TOCTOU, CWE-377) in two places. Both sites build fixed, guessable filenames under world-writable/tmpand then perform root-privileged filesystem operations against them, letting a local unprivileged user pre-plant a symlink to redirect root's writes/deletes/execution.
nullforge/smithy/install.py#L123-L148:download_path/workdirare static (/tmp/{name},/tmp/nullforge-extract/{binary_name}); root thenrm -rf/mkdir -p/extracts/installs through them. Add a random per-run token to both paths (e.g.secrets.token_hex(8)) or generate them viamktemp -d.nullforge/smithy/monitoring/nezha/agent.py#L21-L54:script_path = "/tmp/nezha-agent-install.sh"is downloaded viacurl -oand then directly executed as root — worse than the install.py case since the redirected content is executed, not just moved. Replace with amktemp-generated path inline in the returned shell one-liner.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/smithy/install.py` around lines 123 - 148, The predictable temporary paths in nullforge/smithy/install.py lines 123-148 must be replaced with per-run unpredictable paths for both download_path and workdir, using a secure random token or mktemp while preserving extraction, installation, and cleanup behavior. In nullforge/smithy/monitoring/nezha/agent.py lines 21-54, replace the fixed script_path with a path generated by mktemp inline in the returned root shell command, ensuring the downloaded script is executed and cleaned up through that unique path.Source: Linters/SAST tools
nullforge/runes/users.py-57-68 (1)
57-68: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDon’t pass the user password as a shell command argument.
The password is embedded in the
printf ... | chpasswdstring passed throughserver.shell(), so it can appear in process command-line arguments while the command runs. Avoid calling_set_user_password()this way; use a safer input path or pre-computed hashed password support rather than passing the plaintext value as an argument.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/runes/users.py` around lines 57 - 68, Update _set_user_password so the plaintext password is not embedded in the shell command string or exposed as a process argument. Use a safe input mechanism or existing precomputed-hash support to provide the password to chpasswd, while preserving the user-targeting behavior and password-setting operation.nullforge/runes/xray.py-63-71 (1)
63-71: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd
_sudo=Trueto the GeoIP/GeoSite downloads.
files.downloadwrites into/usr/local/share/xray, but without_sudo=Truethe operation does not use privilege escalation. Run these downloads with the elevated install flow like the gVisor binaries so they don’t fail as the connecting user.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/runes/xray.py` around lines 63 - 71, Update the files.download call in the GEO_DATS loop to pass _sudo=True, matching the elevated installation flow used for gVisor binaries while preserving the existing download arguments.
🟡 Minor comments (8)
nullforge/molds/tor.py-13-20 (1)
13-20: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate Tor listener port ranges.
These values are written directly to
torrc, but invalid ports (for example-1or65536) are accepted until deployment. Add appropriate bounds, such asge=1, le=65535, unless port0is intentionally supported as a disable mechanism.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/molds/tor.py` around lines 13 - 20, Add validation bounds to the socks_port and dns_port fields in the Tor configuration model, limiting both values to the valid listener range of 1 through 65535. Preserve their existing defaults and descriptions, and do not allow zero unless the surrounding configuration explicitly uses it to disable the listener.nullforge/molds/telemt.py-117-128 (1)
117-128: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire a full username match.
_USERNAME_RE.match(name)accepts names with a trailing newline ("alice\n"passes), but usernames are rendered under[access.users]and break the TOML key. Use_USERNAME_RE.fullmatch(name)or\Zin the pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/molds/telemt.py` around lines 117 - 128, The _validate_users validator currently permits usernames with trailing newlines because _USERNAME_RE.match only checks the prefix. Replace this check with a full-name validation using _USERNAME_RE.fullmatch, while preserving the existing invalid-username error and normalization behavior.nullforge/molds/zerotrust.py-15-18 (1)
15-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize the tunnel token before validating it.
A whitespace-only token passes
not self.tokenand reaches the generated tunnel configuration, where authentication fails. Strip the value before this validator runs.Proposed fix
-from pydantic import Field, model_validator +from pydantic import Field, field_validator, model_validator class ZeroTrustTunnelMold(BaseMold): token: str | None = Field( default=None, description="Tunnel token for authentication", ) + `@field_validator`("token") + `@classmethod` + def _strip_token(cls, value: str | None) -> str | None: + return value.strip() if value is not None else None + `@model_validator`(mode="after")Also applies to: 42-45
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/molds/zerotrust.py` around lines 15 - 18, Normalize the token value in the zerotrust model before validation so whitespace-only input is treated as empty. Update the token field validation flow around the token field and its validator to strip surrounding whitespace before applying the existing required-token check, preserving valid non-whitespace token values.nullforge/templates/scripts/zt-tunnel-warp.sh-19-23 (1)
19-23: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake route cleanup idempotent.
ExecStopPostinvokesdownduring service stops/restarts, butip route delexits non-zero when a route is already absent. Withset -e, the first missing route aborts the loop and later routes are not processed. systemd runsExecStopPostduring service shutdown and restart paths, so this hook must tolerate already-clean state. (man7.org)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/templates/scripts/zt-tunnel-warp.sh` around lines 19 - 23, Update the down) cleanup loop in zt-tunnel-warp.sh so an absent route does not cause ip route del to fail the script under set -e. Allow each deletion to tolerate an already-missing route while continuing to process all remaining CIDRS and retain the existing removal logging.nullforge/templates/profiles/tmux.conf-2-2 (1)
2-2: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the escaped quotes from
terminal-overrides.The backslash escapes make the quotes literal at runtime, so tmux does not see the standard
",xterm*":Tc" value and thexterm*` terminal pattern is not matched.Proposed fix
-set-option -sa terminal-overrides \",xterm*:Tc\" +set-option -sa terminal-overrides ",xterm*:Tc"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/templates/profiles/tmux.conf` at line 2, Update the terminal-overrides set-option in the tmux configuration to remove the backslash escapes around the quoted value, preserving the standard xterm* true-color override format so tmux parses and matches the terminal pattern correctly.nullforge/runes/profiles.py-24-46 (1)
24-46: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
_install_starshipruns once per profile target instead of once per host.
_install_starshipprovisions a system-wide binary, but it's called from_install_user_profiles, whichdeploy_shell_profilesinvokes once per target returned by_get_profile_targets(root and/or the named user). When both targets are enabled, starship's install ops are emitted twice for the same host — wasted duplicate work, and withreinstall=Trueit re-runs the install script twice. Every other system-wide installer (_install_eza,_install_tmux,_install_nvim,_install_zoxide,_install_direnv) is correctly called once outside the loop indeploy_shell_profiles.🔧 Proposed fix
_install_zoxide(reinstall) _install_direnv(reinstall) + + _install_starship(reinstall) for user, home_dir in host.loop(_get_profile_targets(features)): _install_user_profiles(user, home_dir, reinstall, profiles_opts.font)def _install_user_profiles(user: str, home_dir: str, reinstall: bool, font: NerdFont | None) -> None: """Configure user profile.""" _configure_user_oh_my_zsh(user, home_dir, reinstall) _configure_user_shell_profiles(user, home_dir) if font: _install_nerd_font(user, home_dir, font, reinstall) _install_user_tmux(user, home_dir, reinstall) _install_user_nvim(user, home_dir, reinstall) _install_atuin(user, home_dir, reinstall) - _install_starship(reinstall)Also applies to: 64-75, 350-361
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/runes/profiles.py` around lines 24 - 46, Move the _install_starship invocation out of _install_user_profiles and call it once from deploy_shell_profiles alongside the other system-wide installers, before iterating over _get_profile_targets(features). Remove the per-target call while preserving user-specific profile setup for each loop target.nullforge/smithy/versions.py-256-272 (1)
256-272: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
is_pinned_version_installedcan implicitly returnNoneinstead ofbool.If
suppress(FactError, FactProcessError)swallows an exception, execution falls off the end of the function with noreturn, givingNonedespite the-> boolannotation. Harmless in today'sif/notcall sites (falsy), but violates the type contract and is a latent footgun for future callers doing strict boolean checks.🐛 Proposed fix
with suppress(FactError, FactProcessError): output = _ctx_host.get_fact(Command, f"{command.format(bin=binary_path)} 2>&1 || true") or "" return pinned.removeprefix("v") in output + return False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/smithy/versions.py` around lines 256 - 272, Update is_pinned_version_installed so exceptions swallowed by the suppress(FactError, FactProcessError) block produce an explicit False result instead of falling through. Preserve the existing version-match return behavior for successful command lookups and ensure every execution path returns a bool.nullforge/smithy/admin.py-22-30 (1)
22-30: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPer-file ACLs are skipped whenever the directory ACL already exists.
The
user:{username}:rwx not in acl_outputguard gates the entirecmdslist, including therw_filesloop. On a re-run where the directory ACL is already present but a file inrw_fileswas newly added or recreated (e.g.haproxy.cfgregenerated), its ACL won't be (re)applied, so the configured user silently loses access. Consider evaluating file ACLs independently of the directory-ACL check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/smithy/admin.py` around lines 22 - 30, The ACL guard in the smithy ACL setup currently suppresses the rw_files loop when the directory ACL already exists. Separate directory ACL commands from per-file ACL commands so each file in rw_files is evaluated and its user:username:rw ACL is applied independently, while preserving the existing directory ACL check and sudo shell execution.
🧹 Nitpick comments (2)
nullforge/smithy/install.py (1)
107-107: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSilent checksum-verification downgrade isn't logged.
When
verify=Truebutsha256_for_download_urlcan't resolve a checksum (API unreachable/rate-limited),checksumbecomesNoneand the download proceeds unverified with no warning. Consider logging when integrity verification is skipped so operators notice the degraded security posture.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/smithy/install.py` at line 107, Update the checksum selection flow around sha256_for_download_url so that when verify is true but no explicit sha256 is provided and checksum resolution returns None, log a warning that integrity verification is being skipped before proceeding. Preserve existing checksum behavior for resolved or explicitly supplied checksums.nullforge/runes/profiles.py (1)
122-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify
open()+io.StringIO+files.putto passing the template path directly.Elsewhere in this codebase (e.g.
nullforge/runes/telemt.py's_deploy_teleproxy_script,nullforge/runes/zerotrust.py's WARP-routing script deploy),files.put(src=get_template_path(...), ...)is passed the path string directly. Here, three call sites open the file, read it into memory, and wrap it inio.StringIObefore handing it tofiles.put/files.block— unnecessary indirection for static (non-Jinja) files.♻️ Example simplification (starship config)
- with open(get_template_path("profiles/starship.toml"), encoding="utf-8") as f: - starship_config = f.read() - files.put( name=f"Configure starship prompt for {user}", - src=io.StringIO(starship_config), + src=get_template_path("profiles/starship.toml"), dest=f"{home_dir}/.config/starship.toml", mode="0644", _sudo=True, _sudo_user=user, )The same simplification applies to the
direnv.tomlput (lines 139-150) and thetmux.confput (lines 285-296). (Thenvim_patch.lua.j2read at lines 328-330 is a different case sincefiles.blockonly acceptscontent=, notsrc=, so that one must stay as-is.)Also applies to: 285-296, 328-330
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/runes/profiles.py` around lines 122 - 150, Update _configure_user_shell_profiles and the tmux configuration deployment to pass each static template path directly as files.put’s src, removing the open(), read(), and io.StringIO wrappers and their now-unused imports. Leave the nvim_patch.lua.j2 handling unchanged because files.block requires content rather than src.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nullforge/runes/base.py`:
- Around line 272-283: Initialize grub_contents to an empty collection before
the suppress block in _ensure_ipv6_stack, then retain the existing fact-fetch
assignment and subsequent any check. This ensures FactError or FactProcessError
leaves the function with no matching lines instead of referencing an unbound
local.
In `@nullforge/runes/netsec.py`:
- Around line 37-42: Initialize contents to an empty list before the suppress
block in _read_checksum_file, so suppressed FactError or FactProcessError
exceptions return an empty string instead of reaching the final expression with
contents unbound.
In `@nullforge/runes/users.py`:
- Around line 21-43: Update the password configuration branch in
deploy_user_management so _configure_passwordless_sudo is called only when
user_opts.sudo is true and no password is provided; users with sudo disabled
must not receive passwordless sudo configuration.
---
Major comments:
In `@nullforge/models/dns.py`:
- Around line 15-19: Remove DnsMode.DOU from the DnsMode enum until its
deployment behavior is implemented, or add mold validation that rejects it
before any host run begins; ensure the unsupported mode cannot reach the
unconditional failure in the DNS rune.
In `@nullforge/models/monitoring/__init__.py`:
- Around line 3-12: Update the MonitoringLayout alias to reference NezhaLayout
directly without Annotated or Field(discriminator="type"), while preserving the
existing alias name and its future-union intent. Remove any now-unused typing or
Pydantic imports from this module.
In `@nullforge/molds/containers.py`:
- Around line 15-18: Reject unsupported deployment configurations in the mold
validators: in nullforge/molds/containers.py:15-18, disallow
ContainersBackendType.CRIO when install=True unless its deployment rune is
implemented; in nullforge/molds/dns.py:16-19, disallow DnsMode.DOU unless the
DNS-over-UDP deployment path is implemented. Ensure validation fails before
casting reaches the unsupported runes.
In `@nullforge/molds/haproxy.py`:
- Around line 13-16: Update the HAProxy version handling across the model field
`version` and the provisioning logic in `haproxy` so RHEL does not silently
ignore a requested version: either install the selected version on RHEL or
validate the platform and reject/document version pinning as Debian-only, while
preserving the existing Debian behavior.
In `@nullforge/molds/monitoring/nezha.py`:
- Around line 83-89: Update _validate_dashboard_url to reject any non-empty
dashboard_url that does not use https://, removing http:// from the accepted
schemes while preserving trailing-slash normalization and empty-value handling.
In `@nullforge/molds/profiles.py`:
- Around line 11-14: Update the profile installation condition in the profiles
handling logic around user_opts.manage so the root profile entry is added
whenever profiles_opts.for_root is true, independently of managed-user creation;
preserve the existing managed-user handling separately.
In `@nullforge/molds/user.py`:
- Around line 21-24: Update UserMold to declare the inherited sensitivity
metadata by adding _sensitive_fields as a ClassVar tuple containing "password",
so BaseMold.to_json() redacts the password during debug serialization.
In `@nullforge/molds/utils.py`:
- Around line 26-46: Update _to_features_fragment and _to_system_dict so mapping
layers preserve only fields explicitly supplied by the caller. Avoid validating
partial dictionaries through the full mold in a way that populates default
values; validate supplied values while retaining the partial shape, then let
merge_* deep-merge without overwriting unspecified existing fields.
In `@nullforge/runes/users.py`:
- Around line 57-68: Update _set_user_password so the plaintext password is not
embedded in the shell command string or exposed as a process argument. Use a
safe input mechanism or existing precomputed-hash support to provide the
password to chpasswd, while preserving the user-targeting behavior and
password-setting operation.
In `@nullforge/runes/xray.py`:
- Around line 63-71: Update the files.download call in the GEO_DATS loop to pass
_sudo=True, matching the elevated installation flow used for gVisor binaries
while preserving the existing download arguments.
In `@nullforge/smithy/install.py`:
- Around line 123-148: The predictable temporary paths in
nullforge/smithy/install.py lines 123-148 must be replaced with per-run
unpredictable paths for both download_path and workdir, using a secure random
token or mktemp while preserving extraction, installation, and cleanup behavior.
In nullforge/smithy/monitoring/nezha/agent.py lines 21-54, replace the fixed
script_path with a path generated by mktemp inline in the returned root shell
command, ensuring the downloaded script is executed and cleaned up through that
unique path.
In `@nullforge/templates/cloudflared/tunnel.yml.j2`:
- Around line 1-3: Update the cloudflared tunnel template to emit the supplied
POST_QUANTUM value using the post-quantum configuration key, alongside token,
protocol, and ha-connections, so _deploy_tunnel()’s opts.post_quantum setting is
applied.
In `@nullforge/templates/dns/resolved.conf.j2`:
- Around line 3-4: Update _deploy_dot_resolved to pass the resolved upstream
list to dns/resolved.conf.j2, formatting each DoT server as the template’s
expected host#SNI or tcp-tls:[host]:port representation and preserving IPv4/IPv6
handling. Replace the hard-coded Cloudflare DNS= value in the template with the
provided upstream variable so upstream_provider and IPv6 configuration determine
the rendered resolvers.
In `@nullforge/templates/scripts/warp-v6-policy.sh`:
- Around line 16-38: Replace the 100-slot hash-based allocation in _slot, TID,
and PRIO with collision-safe persisted ownership or validation before applying
rules. Ensure each interface’s table ID and rule preference are unique, and make
the up path fail before adding rules/routes when either value is owned by
another interface; preserve explicit TID/PRIO handling while validating those
values as well.
In `@nullforge/templates/systemd/cloudflare-tunnel.service.j2`:
- Around line 12-14: Update the ROUTE_THROUGH_WARP systemd hooks in the service
template to pass the configured WarpMold.iface value to zt-tunnel-warp.sh for
both the up and down actions, preserving the existing WORKDIR and action
arguments.
- Around line 3-4: Add the configured WARP systemd unit to the cloudflare-tunnel
service dependencies by including it in both Wants/Requires and After alongside
network-online.target. Preserve the existing network dependency and ensure the
WARP unit is started and ordered before cloudflared runs zt-tunnel-warp.sh.
In `@nullforge/templates/systemd/dns-internal.service.j2`:
- Line 10: Update the DNS endpoint templates to handle both IPv4 and IPv6 listen
addresses: use the appropriate address prefix length in the systemd service
command and bracket IPv6 addresses when rendering the Blocky DNS endpoint.
Preserve the existing IPv4 output and base the family-aware formatting on the
configured listen_address value.
In `@nullforge/templates/systemd/telemt.service.j2`:
- Around line 17-30: Remove CAP_NET_ADMIN from the telemt.service.j2
AmbientCapabilities and CapabilityBoundingSet declarations, retaining only
CAP_NET_BIND_SERVICE for the long-running proxy daemon. Leave the privileged
ExecStartPre and ExecStopPost helper commands unchanged.
---
Minor comments:
In `@nullforge/molds/telemt.py`:
- Around line 117-128: The _validate_users validator currently permits usernames
with trailing newlines because _USERNAME_RE.match only checks the prefix.
Replace this check with a full-name validation using _USERNAME_RE.fullmatch,
while preserving the existing invalid-username error and normalization behavior.
In `@nullforge/molds/tor.py`:
- Around line 13-20: Add validation bounds to the socks_port and dns_port fields
in the Tor configuration model, limiting both values to the valid listener range
of 1 through 65535. Preserve their existing defaults and descriptions, and do
not allow zero unless the surrounding configuration explicitly uses it to
disable the listener.
In `@nullforge/molds/zerotrust.py`:
- Around line 15-18: Normalize the token value in the zerotrust model before
validation so whitespace-only input is treated as empty. Update the token field
validation flow around the token field and its validator to strip surrounding
whitespace before applying the existing required-token check, preserving valid
non-whitespace token values.
In `@nullforge/runes/profiles.py`:
- Around line 24-46: Move the _install_starship invocation out of
_install_user_profiles and call it once from deploy_shell_profiles alongside the
other system-wide installers, before iterating over
_get_profile_targets(features). Remove the per-target call while preserving
user-specific profile setup for each loop target.
In `@nullforge/smithy/admin.py`:
- Around line 22-30: The ACL guard in the smithy ACL setup currently suppresses
the rw_files loop when the directory ACL already exists. Separate directory ACL
commands from per-file ACL commands so each file in rw_files is evaluated and
its user:username:rw ACL is applied independently, while preserving the existing
directory ACL check and sudo shell execution.
In `@nullforge/smithy/versions.py`:
- Around line 256-272: Update is_pinned_version_installed so exceptions
swallowed by the suppress(FactError, FactProcessError) block produce an explicit
False result instead of falling through. Preserve the existing version-match
return behavior for successful command lookups and ensure every execution path
returns a bool.
In `@nullforge/templates/profiles/tmux.conf`:
- Line 2: Update the terminal-overrides set-option in the tmux configuration to
remove the backslash escapes around the quoted value, preserving the standard
xterm* true-color override format so tmux parses and matches the terminal
pattern correctly.
In `@nullforge/templates/scripts/zt-tunnel-warp.sh`:
- Around line 19-23: Update the down) cleanup loop in zt-tunnel-warp.sh so an
absent route does not cause ip route del to fail the script under set -e. Allow
each deletion to tolerate an already-missing route while continuing to process
all remaining CIDRS and retain the existing removal logging.
---
Nitpick comments:
In `@nullforge/runes/profiles.py`:
- Around line 122-150: Update _configure_user_shell_profiles and the tmux
configuration deployment to pass each static template path directly as
files.put’s src, removing the open(), read(), and io.StringIO wrappers and their
now-unused imports. Leave the nvim_patch.lua.j2 handling unchanged because
files.block requires content rather than src.
In `@nullforge/smithy/install.py`:
- Line 107: Update the checksum selection flow around sha256_for_download_url so
that when verify is true but no explicit sha256 is provided and checksum
resolution returns None, log a warning that integrity verification is being
skipped before proceeding. Preserve existing checksum behavior for resolved or
explicitly supplied checksums.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b01edc5-9e56-4298-b0bc-f07281e9dffc
📒 Files selected for processing (97)
.gitignoreREADME.mdnullforge/foundry/README.mdnullforge/foundry/__init__.pynullforge/foundry/full_cast.pynullforge/inventories/README.mdnullforge/inventories/example.pynullforge/models/__init__.pynullforge/models/containers.pynullforge/models/dns.pynullforge/models/monitoring/__init__.pynullforge/models/monitoring/base.pynullforge/models/monitoring/nezha.pynullforge/models/netsec.pynullforge/models/profiles.pynullforge/models/system.pynullforge/models/users.pynullforge/models/warp.pynullforge/models/zerotrust.pynullforge/molds/__init__.pynullforge/molds/base_mold.pynullforge/molds/containers.pynullforge/molds/defaults.pynullforge/molds/dns.pynullforge/molds/features.pynullforge/molds/haproxy.pynullforge/molds/monitoring/__init__.pynullforge/molds/monitoring/nezha.pynullforge/molds/netsec.pynullforge/molds/profiles.pynullforge/molds/system.pynullforge/molds/telemt.pynullforge/molds/tor.pynullforge/molds/user.pynullforge/molds/utils.pynullforge/molds/warp.pynullforge/molds/xray.pynullforge/molds/zerotrust.pynullforge/runes/__init__.pynullforge/runes/base.pynullforge/runes/containers.pynullforge/runes/dns.pynullforge/runes/haproxy.pynullforge/runes/monitoring.pynullforge/runes/netsec.pynullforge/runes/prepare.pynullforge/runes/profiles.pynullforge/runes/telemt.pynullforge/runes/tor.pynullforge/runes/users.pynullforge/runes/warp.pynullforge/runes/xray.pynullforge/runes/zerotrust.pynullforge/smithy/__init__.pynullforge/smithy/admin.pynullforge/smithy/arch.pynullforge/smithy/blocky.pynullforge/smithy/cloudflare.pynullforge/smithy/github.pynullforge/smithy/http.pynullforge/smithy/install.pynullforge/smithy/monitoring/__init__.pynullforge/smithy/monitoring/nezha/__init__.pynullforge/smithy/monitoring/nezha/agent.pynullforge/smithy/monitoring/nezha/dashboard.pynullforge/smithy/monitoring/nezha/deploy.pynullforge/smithy/network.pynullforge/smithy/packages.pynullforge/smithy/service.pynullforge/smithy/sni.pynullforge/smithy/swap.pynullforge/smithy/system.pynullforge/smithy/versions.pynullforge/templates/__init__.pynullforge/templates/cloudflared/tunnel.yml.j2nullforge/templates/dns/blocky.yaml.j2nullforge/templates/dns/dns.yaml.j2nullforge/templates/dns/resolv.conf.j2nullforge/templates/dns/resolved.conf.j2nullforge/templates/etc/default/zramswap.j2nullforge/templates/nvim/nvim_patch.lua.j2nullforge/templates/profiles/direnv.tomlnullforge/templates/profiles/starship.tomlnullforge/templates/profiles/tmux.confnullforge/templates/profiles/zshrc.j2nullforge/templates/scripts/telemt-synfix.shnullforge/templates/scripts/teleproxy-warp.shnullforge/templates/scripts/warp-v6-policy.shnullforge/templates/scripts/zt-tunnel-warp.shnullforge/templates/systemd/blocky.service.j2nullforge/templates/systemd/cloudflare-tunnel.service.j2nullforge/templates/systemd/cloudflare-warp.service.j2nullforge/templates/systemd/dns-internal.service.j2nullforge/templates/systemd/telemt.service.j2nullforge/templates/telemt/telemt.toml.j2nullforge/templates/tor/torrc.j2pyproject.toml
d44ecad to
2077b96
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nullforge/smithy/system.py (2)
45-75: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTreat whitespace-only
preferredvalues as unset.A truthy value such as
" "has no first token, so direct first-token extraction can raise instead of falling through to the default locale order. Strip first and guard the empty result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/smithy/system.py` around lines 45 - 75, Update detect_best_locale to normalize preferred with surrounding whitespace removed before checking or extracting its locale name. Treat an empty normalized value as unset, avoiding split()[0] and allowing the existing default locale selection to run.
11-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the memory cache key account for
default.When the fact is unavailable, the first call caches its fallback under a fixed key; a later call with another
defaultreturns the earlier value. Includedefaultin the cache key or cache only actual fact values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/smithy/system.py` around lines 11 - 18, Update the memory caching logic around the cache_key and get_fact(Memory) flow so calls with different default values cannot reuse a fallback cached for another default. Include default in the cache key, while preserving reuse of the computed total_mb and existing behavior when the Memory fact is available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nullforge/molds/zerotrust.py`:
- Around line 57-58: Update the post-quantum validation in the relevant
ZeroTrust configuration initializer to require self.protocol ==
ZeroTrustTunnelProtocol.QUIC, rejecting AUTO and all other protocols while
preserving the existing ValueError message and behavior for valid QUIC
configuration.
In `@nullforge/smithy/system.py`:
- Around line 34-39: Update the locale parsing logic around the parts handling
to validate the locale and charmap tokens before appending to locales. Ensure
non-locale comments such as “This file ...” are rejected, while valid locale
entries continue to be appended unchanged.
---
Outside diff comments:
In `@nullforge/smithy/system.py`:
- Around line 45-75: Update detect_best_locale to normalize preferred with
surrounding whitespace removed before checking or extracting its locale name.
Treat an empty normalized value as unset, avoiding split()[0] and allowing the
existing default locale selection to run.
- Around line 11-18: Update the memory caching logic around the cache_key and
get_fact(Memory) flow so calls with different default values cannot reuse a
fallback cached for another default. Include default in the cache key, while
preserving reuse of the computed total_mb and existing behavior when the Memory
fact is available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b2d5003-19bb-46f1-90f9-ba5e1120d7cd
📒 Files selected for processing (31)
README.mdnullforge/molds/haproxy.pynullforge/molds/monitoring/nezha.pynullforge/molds/telemt.pynullforge/molds/tor.pynullforge/molds/user.pynullforge/molds/utils.pynullforge/molds/zerotrust.pynullforge/runes/base.pynullforge/runes/dns.pynullforge/runes/haproxy.pynullforge/runes/netsec.pynullforge/runes/profiles.pynullforge/runes/users.pynullforge/runes/xray.pynullforge/runes/zerotrust.pynullforge/smithy/admin.pynullforge/smithy/install.pynullforge/smithy/monitoring/nezha/agent.pynullforge/smithy/network.pynullforge/smithy/system.pynullforge/smithy/versions.pynullforge/templates/cloudflared/tunnel.yml.j2nullforge/templates/dns/blocky.yaml.j2nullforge/templates/dns/resolved.conf.j2nullforge/templates/profiles/tmux.confnullforge/templates/scripts/zt-tunnel-warp.shnullforge/templates/systemd/cloudflare-tunnel.service.j2nullforge/templates/systemd/dns-internal.service.j2nullforge/templates/systemd/telemt.service.j2pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (26)
- nullforge/templates/cloudflared/tunnel.yml.j2
- nullforge/molds/haproxy.py
- nullforge/templates/dns/blocky.yaml.j2
- nullforge/templates/dns/resolved.conf.j2
- nullforge/templates/scripts/zt-tunnel-warp.sh
- nullforge/templates/profiles/tmux.conf
- nullforge/smithy/monitoring/nezha/agent.py
- nullforge/templates/systemd/telemt.service.j2
- nullforge/templates/systemd/cloudflare-tunnel.service.j2
- nullforge/templates/systemd/dns-internal.service.j2
- nullforge/runes/users.py
- nullforge/smithy/network.py
- nullforge/runes/haproxy.py
- nullforge/smithy/admin.py
- nullforge/runes/xray.py
- nullforge/runes/dns.py
- nullforge/molds/telemt.py
- nullforge/runes/zerotrust.py
- nullforge/molds/user.py
- nullforge/smithy/versions.py
- nullforge/smithy/install.py
- nullforge/molds/monitoring/nezha.py
- nullforge/runes/netsec.py
- nullforge/runes/base.py
- nullforge/runes/profiles.py
- nullforge/molds/utils.py
bbd9314 to
96d6115
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
nullforge/smithy/monitoring/nezha/dashboard.py (1)
28-43: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider validating scheme inside
_api_requestrather than relying solely on the caller.The
# noqa: S310 - scheme validated by callercomment assumes upstream (mold) validation ofdashboard_url's scheme. Since this is a reusable smithy helper, not gated to one call site, adding the samehttps://-only guard used insmithy/http.py'sfetch_textwould be a cheap, self-contained hardening that doesn't depend on that external assumption holding.🛡️ Suggested guard
def _api_request( url: str, token: str, *, method: str = "GET", payload: dict[str, object] | None = None, ) -> dict[str, Any]: """Issue authenticated dashboard API request and return parsed envelope.""" + + if not url.startswith("https://"): + raise ValueError(f"Refusing non-HTTPS dashboard URL: {url}")Please confirm whether
dashboard_url's mold field already enforces an http(s)-only type, which would make this optional rather than necessary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/smithy/monitoring/nezha/dashboard.py` around lines 28 - 43, Validate the URL scheme inside `_api_request` before constructing the `urllib.request.Request`, enforcing the same HTTPS-only guard used by `fetch_text` in `smithy/http.py`. Remove the reliance on caller-side validation and update the security suppression comment to reflect the local validation.nullforge/runes/profiles.py (1)
365-365: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePredictable
/tmpfilenames for root-privileged downloads across multiple installers. All six sites download/install to a fixed, guessable path under/tmp, run with_sudo=True; on a host with other local users this is susceptible to symlink pre-creation/TOCTOU races (CWE-377). Same fix applies everywhere: generate the path viamktemp/mktemp -dinstead of a hardcoded literal.
nullforge/runes/profiles.py#L365-L365: replacestarship_install_path = "/tmp/starship.sh"with amktemp-generated path.nullforge/runes/profiles.py#L391-L391: replaceatuin_install_path = "/tmp/atuin.sh"with amktemp-generated path.nullforge/runes/profiles.py#L417-L417: replacezoxide_install_path = "/tmp/zoxide.sh"with amktemp-generated path.nullforge/runes/profiles.py#L472-L473: replacetmux_tar_path/tmux_src_dirwithmktemp/mktemp -d-generated paths.nullforge/runes/profiles.py#L529-L529: replacenvim_appimage_path = "/tmp/nvim.appimage"with amktemp-generated path.nullforge/runes/containers.py#L148-L148: replaceget_docker_path = "/tmp/get-docker.sh"with amktemp-generated path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullforge/runes/profiles.py` at line 365, Replace the predictable root-privileged temporary paths with securely generated temporary paths. In nullforge/runes/profiles.py lines 365-365, 391-391, 417-417, 529-529, use mktemp-generated file paths for the installer variables; at lines 472-473, use mktemp for tmux_tar_path and mktemp -d for tmux_src_dir; in nullforge/runes/containers.py line 148, apply the same mktemp-based path generation to get_docker_path.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nullforge/runes/netsec.py`:
- Around line 191-236: Update the firewalld rule builder around action_map to
reject rules with action="limit" before any port or rich-rule commands are
generated, raising a clear ValueError like the existing direction="out"
rejection. Remove the limit-to-accept mapping so unsupported limit rules cannot
emit unrestricted allow commands; preserve behavior for allow, deny, and reject.
In `@nullforge/smithy/system.py`:
- Around line 25-26: Update the memory lookup in the surrounding system resource
function to tolerate Memory fact failures by requesting the fact with
_ignore_errors=True, while preserving the existing default fallback when no
value is returned; alternatively, replace the unreliable Memory lookup with
portable /proc/meminfo parsing.
---
Nitpick comments:
In `@nullforge/runes/profiles.py`:
- Line 365: Replace the predictable root-privileged temporary paths with
securely generated temporary paths. In nullforge/runes/profiles.py lines
365-365, 391-391, 417-417, 529-529, use mktemp-generated file paths for the
installer variables; at lines 472-473, use mktemp for tmux_tar_path and mktemp
-d for tmux_src_dir; in nullforge/runes/containers.py line 148, apply the same
mktemp-based path generation to get_docker_path.
In `@nullforge/smithy/monitoring/nezha/dashboard.py`:
- Around line 28-43: Validate the URL scheme inside `_api_request` before
constructing the `urllib.request.Request`, enforcing the same HTTPS-only guard
used by `fetch_text` in `smithy/http.py`. Remove the reliance on caller-side
validation and update the security suppression comment to reflect the local
validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1592e65d-2ecd-4abe-bc2d-8228698af966
📒 Files selected for processing (97)
.gitignoreREADME.mdnullforge/foundry/README.mdnullforge/foundry/__init__.pynullforge/foundry/full_cast.pynullforge/inventories/README.mdnullforge/inventories/example.pynullforge/models/__init__.pynullforge/models/containers.pynullforge/models/dns.pynullforge/models/monitoring/__init__.pynullforge/models/monitoring/base.pynullforge/models/monitoring/nezha.pynullforge/models/netsec.pynullforge/models/profiles.pynullforge/models/system.pynullforge/models/users.pynullforge/models/warp.pynullforge/models/zerotrust.pynullforge/molds/__init__.pynullforge/molds/base_mold.pynullforge/molds/containers.pynullforge/molds/defaults.pynullforge/molds/dns.pynullforge/molds/features.pynullforge/molds/haproxy.pynullforge/molds/monitoring/__init__.pynullforge/molds/monitoring/nezha.pynullforge/molds/netsec.pynullforge/molds/profiles.pynullforge/molds/system.pynullforge/molds/telemt.pynullforge/molds/tor.pynullforge/molds/user.pynullforge/molds/utils.pynullforge/molds/warp.pynullforge/molds/xray.pynullforge/molds/zerotrust.pynullforge/runes/__init__.pynullforge/runes/base.pynullforge/runes/containers.pynullforge/runes/dns.pynullforge/runes/haproxy.pynullforge/runes/monitoring.pynullforge/runes/netsec.pynullforge/runes/prepare.pynullforge/runes/profiles.pynullforge/runes/telemt.pynullforge/runes/tor.pynullforge/runes/users.pynullforge/runes/warp.pynullforge/runes/xray.pynullforge/runes/zerotrust.pynullforge/smithy/__init__.pynullforge/smithy/admin.pynullforge/smithy/arch.pynullforge/smithy/blocky.pynullforge/smithy/cloudflare.pynullforge/smithy/github.pynullforge/smithy/http.pynullforge/smithy/install.pynullforge/smithy/monitoring/__init__.pynullforge/smithy/monitoring/nezha/__init__.pynullforge/smithy/monitoring/nezha/agent.pynullforge/smithy/monitoring/nezha/dashboard.pynullforge/smithy/monitoring/nezha/deploy.pynullforge/smithy/network.pynullforge/smithy/packages.pynullforge/smithy/service.pynullforge/smithy/sni.pynullforge/smithy/swap.pynullforge/smithy/system.pynullforge/smithy/versions.pynullforge/templates/__init__.pynullforge/templates/cloudflared/tunnel.yml.j2nullforge/templates/dns/blocky.yaml.j2nullforge/templates/dns/dns.yaml.j2nullforge/templates/dns/resolv.conf.j2nullforge/templates/dns/resolved.conf.j2nullforge/templates/etc/default/zramswap.j2nullforge/templates/nvim/nvim_patch.lua.j2nullforge/templates/profiles/direnv.tomlnullforge/templates/profiles/starship.tomlnullforge/templates/profiles/tmux.confnullforge/templates/profiles/zshrc.j2nullforge/templates/scripts/telemt-synfix.shnullforge/templates/scripts/teleproxy-warp.shnullforge/templates/scripts/warp-v6-policy.shnullforge/templates/scripts/zt-tunnel-warp.shnullforge/templates/systemd/blocky.service.j2nullforge/templates/systemd/cloudflare-tunnel.service.j2nullforge/templates/systemd/cloudflare-warp.service.j2nullforge/templates/systemd/dns-internal.service.j2nullforge/templates/systemd/telemt.service.j2nullforge/templates/telemt/telemt.toml.j2nullforge/templates/tor/torrc.j2pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (82)
- nullforge/templates/dns/resolv.conf.j2
- nullforge/models/init.py
- nullforge/templates/systemd/cloudflare-warp.service.j2
- nullforge/models/monitoring/nezha.py
- nullforge/smithy/init.py
- nullforge/foundry/init.py
- nullforge/models/users.py
- nullforge/molds/haproxy.py
- nullforge/templates/systemd/cloudflare-tunnel.service.j2
- nullforge/templates/profiles/direnv.toml
- nullforge/models/zerotrust.py
- nullforge/smithy/monitoring/init.py
- nullforge/models/netsec.py
- nullforge/molds/tor.py
- pyproject.toml
- nullforge/runes/monitoring.py
- nullforge/molds/containers.py
- nullforge/smithy/service.py
- nullforge/molds/xray.py
- nullforge/runes/tor.py
- nullforge/models/containers.py
- nullforge/templates/systemd/dns-internal.service.j2
- nullforge/foundry/full_cast.py
- .gitignore
- nullforge/molds/profiles.py
- nullforge/molds/defaults.py
- nullforge/templates/init.py
- nullforge/models/monitoring/base.py
- nullforge/models/system.py
- nullforge/smithy/arch.py
- nullforge/templates/tor/torrc.j2
- nullforge/templates/cloudflared/tunnel.yml.j2
- nullforge/smithy/cloudflare.py
- nullforge/templates/dns/dns.yaml.j2
- nullforge/molds/monitoring/init.py
- nullforge/templates/systemd/blocky.service.j2
- nullforge/runes/init.py
- nullforge/models/profiles.py
- nullforge/inventories/example.py
- nullforge/templates/systemd/telemt.service.j2
- nullforge/runes/xray.py
- nullforge/smithy/blocky.py
- nullforge/molds/zerotrust.py
- nullforge/runes/prepare.py
- nullforge/molds/dns.py
- nullforge/templates/scripts/zt-tunnel-warp.sh
- nullforge/templates/dns/resolved.conf.j2
- nullforge/molds/init.py
- nullforge/molds/system.py
- nullforge/templates/profiles/tmux.conf
- nullforge/models/warp.py
- nullforge/templates/telemt/telemt.toml.j2
- nullforge/runes/haproxy.py
- nullforge/smithy/swap.py
- nullforge/runes/users.py
- nullforge/molds/features.py
- nullforge/templates/profiles/zshrc.j2
- nullforge/molds/user.py
- nullforge/templates/etc/default/zramswap.j2
- nullforge/models/dns.py
- nullforge/smithy/monitoring/nezha/agent.py
- nullforge/smithy/monitoring/nezha/deploy.py
- nullforge/smithy/admin.py
- nullforge/templates/profiles/starship.toml
- nullforge/runes/zerotrust.py
- nullforge/runes/telemt.py
- nullforge/smithy/install.py
- nullforge/molds/warp.py
- nullforge/molds/netsec.py
- nullforge/smithy/packages.py
- nullforge/runes/dns.py
- nullforge/molds/telemt.py
- nullforge/molds/monitoring/nezha.py
- nullforge/molds/utils.py
- nullforge/smithy/versions.py
- nullforge/molds/base_mold.py
- nullforge/smithy/network.py
- nullforge/runes/warp.py
- nullforge/smithy/github.py
- nullforge/smithy/sni.py
- nullforge/templates/nvim/nvim_patch.lua.j2
- nullforge/runes/base.py
96d6115 to
951f6d5
Compare
1 participant
Type of change
Description
Why is this change needed?
Related Issues
Testing
uv run poe tests)Checklist
uv run poe check)Summary by CodeRabbit