Enable Nmbd by default, install wsdd2, fully rework smb file handling#52
Open
chrismin13 wants to merge 4 commits into
Open
Enable Nmbd by default, install wsdd2, fully rework smb file handling#52chrismin13 wants to merge 4 commits into
chrismin13 wants to merge 4 commits into
Conversation
…fault, install wsdd2 when available
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR moves SimpleSaferServer-managed Samba configuration from inline smb.conf markers to two owned include files, adds SambaLayoutService to create/validate/include them, shifts SMBManager to use the owned shares file with effective-config inspection for unmanaged shares, tracks three services (smbd/nmbd/wsdd2), and updates installer, uninstaller, UI, routes, runtime, and tests. ChangesSamba Discovery & Configuration Ownership Redesign
Sequence Diagram(s)sequenceDiagram
participant Installer
participant SambaLayout as SambaLayoutService
participant SMBManager
participant CommandAdapter
participant Systemd
Installer->>SambaLayout: ensure_layout()
SambaLayout->>CommandAdapter: validate_config(candidate smb.conf)
alt validation succeeds
SambaLayout->>SMBManager: publish include files
Installer->>Systemd: enable/start smbd
Systemd->>Systemd: report smbd active/inactive
Installer->>Systemd: try enable/start nmbd, wsdd2 (best-effort)
else validation fails
SambaLayout->>Installer: raise SambaLayoutError (rollback)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR modernizes SimpleSaferServer’s Samba integration by moving managed share ownership from inline smb.conf markers to SimpleSaferServer-owned include files, adds optional wsdd2 support, and updates install/uninstall + UI to reflect a 3-service (smbd/nmbd/wsdd2) model.
Changes:
- Replace marker-wrapped inline share management with owned include files (
simple_safer_server_globals.conf/simple_safer_server_shares.conf) plus a newSambaLayoutServiceto ensure wiring + validate/rollback. - Add optional
wsdd2install and expand service status/restart flows to handlesmbd,nmbd, andwsdd2(including “unavailable” semantics in the UI). - Rework uninstall Samba cleanup to remove include blocks + owned files, warn on malformed markers, and restart
smbdbest-effort.
Reviewed changes
Copilot reviewed 58 out of 58 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| uninstall.sh | Removes SSS-owned include blocks/files, adds malformed-marker warnings, and best-effort smbd restart. |
| install.sh | Anchors rsync excludes, optionally installs wsdd2, prepares Samba layout, and configures discovery services with smbd as required. |
| index.html | Updates public install/uninstall copy to reflect wsdd2 + include-file ownership and smbd restart impact. |
| docs/install.md | Documents installer behavior for Samba layout + discovery services (required smbd, optional wsdd2). |
| docs/manual_install.md | Adds manual Samba layout preparation and optional wsdd2 guidance. |
| docs/uninstall.md | Documents include-file cleanup, malformed-marker behavior, and smbd restart on uninstall. |
| docs/network_file_sharing.md | Updates ownership model, unmanaged-share definition (effective config), and 3-service semantics. |
| docs/dashboard.md | Updates dashboard semantics to include wsdd2 and treat “unavailable” discovery as non-degrading. |
| docs/drive_health.md | Corrects managed share file location references for recovery guidance. |
| simple_safer_server/services/samba_layout.py | New service to create/repair owned Samba layout with validation + rollback. |
| simple_safer_server/services/templates/simple_safer_server_globals.conf | New globals template deployed by SambaLayoutService. |
| simple_safer_server/services/smb_manager.py | Reworks managed share CRUD onto owned shares file and adds effective-config inspection/rollback behaviors. |
| simple_safer_server/services/runtime.py | Removes legacy samba backup dir field; expands fake-state SMB services to include wsdd2. |
| simple_safer_server/adapters/smb_commands.py | Adds cwd support for config validation and introduces unit availability detection via systemctl cat. |
| simple_safer_server/routes/smb.py | Surfaces unmanaged-share verification state and distinguishes validation vs operation failures. |
| simple_safer_server/services/drive_health.py | Adds asserts to satisfy static analysis for optional ML dependencies. |
| templates/network_file_sharing.html | Adds wsdd2 status row, three-tier discovery badges, and unmanaged verification failure UI state. |
| templates/dashboard.html | Updates dashboard file-sharing summary logic for wsdd2 + “unavailable” semantics. |
| tests/test_samba_layout.py | New tests for Samba layout helper idempotency, validation, rollback, and file creation. |
| tests/test_smb_manager.py | Major test updates for owned-shares-file model, effective-config inspection, rollback, and wsdd2 status/restart. |
| tests/test_smb_command_adapter.py | Adds unit-status tests and validates cwd forwarding for config validation. |
| tests/test_uninstall.py | Updates uninstall tests for include-block removal, owned-file deletion, best-effort restarts, and legacy backup dir handling. |
| tests/test_install_preflight.py | Adds coverage for optional wsdd2 install, anchored rsync excludes, and required smbd gating. |
| tests/test_app_factory_routes.py | Adds UI/API assertions for wsdd2 status fields, discovery logic, and unmanaged verification warning state. |
| tests/test_setup_wizard.py | Updates setup guidance messaging for unmanaged backup conflict to short form. |
| tests/test_drive_health.py | Formatting-only change in a test helper definition. |
| tests/test_drive_health_docs.py | New docs regression tests to ensure drive-health docs reference the new Samba share file paths. |
| planning-archive/nmbd-wsdd2/samba-discovery-config-redesign/PRD.md | Adds internal planning documentation for Samba redesign. |
| planning-archive/nmbd-wsdd2/samba-discovery-config-redesign/issues/README.md | Adds internal planning issue index. |
| planning-archive/nmbd-wsdd2/samba-discovery-config-redesign/issues/01-establish-sss-owned-samba-layout.md | Internal planning slice for layout helper. |
| planning-archive/nmbd-wsdd2/samba-discovery-config-redesign/issues/02-move-managed-shares-to-sss-shares-file.md | Internal planning slice for shares ownership. |
| planning-archive/nmbd-wsdd2/samba-discovery-config-redesign/issues/03-install-and-report-discovery-services.md | Internal planning slice for discovery service install/status. |
| planning-archive/nmbd-wsdd2/samba-discovery-config-redesign/issues/04-track-three-file-sharing-services-in-web-ui.md | Internal planning slice for UI/service tracking. |
| planning-archive/nmbd-wsdd2/samba-discovery-config-redesign/issues/05-clean-up-sss-owned-samba-config-on-uninstall.md | Internal planning slice for uninstall behavior. |
| planning-archive/nmbd-wsdd2/samba-discovery-config-redesign/issues/06-document-samba-ownership-and-discovery.md | Internal planning slice for docs updates. |
| planning-archive/nmbd-wsdd2/samba-effective-config-hardening/PRD.md | Internal planning documentation for effective-config hardening. |
| planning-archive/nmbd-wsdd2/samba-effective-config-hardening/issues/README.md | Internal planning issue index. |
| planning-archive/nmbd-wsdd2/samba-effective-config-hardening/issues/01-detect-unmanaged-samba-shares-from-effective-config.md | Internal planning slice for unmanaged detection. |
| planning-archive/nmbd-wsdd2/samba-effective-config-hardening/issues/02-use-effective-unmanaged-share-detection-in-smb-writes.md | Internal planning slice for write safety. |
| planning-archive/nmbd-wsdd2/samba-effective-config-hardening/issues/03-surface-unmanaged-share-verification-state-in-ui.md | Internal planning slice for UI verification state. |
| planning-archive/nmbd-wsdd2/samba-effective-config-hardening/issues/04-complete-smb-share-publish-rollback.md | Internal planning slice for rollback completeness. |
| planning-archive/nmbd-wsdd2/samba-effective-config-hardening/issues/05-honor-required-smbd-failure-in-installer.md | Internal planning slice for installer gating. |
| planning-archive/nmbd-wsdd2/samba-effective-config-hardening/issues/06-clean-owned-samba-files-on-uninstall-independently.md | Internal planning slice for uninstall cleanup. |
| planning-archive/nmbd-wsdd2/samba-effective-config-hardening/issues/07-repair-manual-install-documentation.md | Internal planning slice for manual install docs. |
| planning-archive/nmbd-wsdd2/samba-pre-submit-cleanup/PRD.md | Internal planning documentation for cleanup work. |
| planning-archive/nmbd-wsdd2/samba-pre-submit-cleanup/issues/01-managed-share-user-helpers-owned-file-first.md | Internal planning slice for owned-file-first behavior. |
| planning-archive/nmbd-wsdd2/samba-pre-submit-cleanup/issues/02-remove-dead-inline-managed-share-machinery.md | Internal planning slice for dead-code removal. |
| planning-archive/nmbd-wsdd2/samba-pre-submit-cleanup/issues/03-discovery-service-badges-three-tier.md | Internal planning slice for badge tiers. |
| planning-archive/nmbd-wsdd2/samba-pre-submit-cleanup/issues/04-drive-health-docs-sss-shares-file.md | Internal planning slice for docs correction. |
| planning-archive/nmbd-wsdd2/samba-post-review-fixes/PRD.md | Internal planning documentation for post-review fixes. |
| planning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/01-remove-dead-samba-backup-dir-and-copy-config.md | Internal planning slice for removing dead backup infra. |
| planning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/02-uninstall-restarts-smbd-after-cleanup.md | Internal planning slice for uninstall restart. |
| planning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/03-uninstall-warns-broken-includes-on-malformed-markers.md | Internal planning slice for malformed-marker warnings. |
| planning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/04-uninstall-cleans-up-legacy-empty-backup-directory.md | Internal planning slice for legacy backup dir cleanup. |
| planning-archive/nmbd-wsdd2/samba-review-round-2-fixes/PRD.md | Internal planning documentation for round-2 fixes. |
| planning-archive/nmbd-wsdd2/samba-review-round-2-fixes/issues/01-anchor-rsync-excludes-in-install-sh.md | Internal planning slice for rsync anchoring. |
| planning-archive/nmbd-wsdd2/samba-review-round-2-fixes/issues/02-treat-unavailable-as-non-degrading-in-overall-status.md | Internal planning slice for unavailable semantics. |
| planning-archive/nmbd-wsdd2/samba-review-round-2-fixes/issues/03-change-test-patches-to-assertion-error.md | Internal planning slice for test intent guards. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+51
to
+64
| status = result.stdout.strip() | ||
| if status == "active": | ||
| return "active" | ||
| # 'inactive' from is-active covers both stopped units and missing units. | ||
| # Check whether the unit file actually exists on disk. | ||
| cat_result = self._command_runner.run( | ||
| self._command("systemctl", "cat", unit_name), | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=SMB_COMMAND_TIMEOUT_SECONDS, | ||
| ) | ||
| if cat_result.returncode != 0: | ||
| return "unavailable" | ||
| return status |
Comment on lines
686
to
688
| def list_unmanaged_shares(self): | ||
| _, shares = self._load_shares() | ||
| return [share.as_dict() for share in shares if not share.managed] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_smb_command_adapter.py (1)
18-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix Ruff formatting in this file before merge.
CI is currently failing on
ruff format --checkfor this file, so this is merge-blocking.🤖 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 `@tests/test_smb_command_adapter.py` around lines 18 - 83, The file fails ruff formatting: run the code formatter (ruff format) on the test file and fix formatting inconsistencies (whitespace, line breaks, imports) so it passes `ruff format --check`; specifically ensure classes ConfigurableRunner and UnitStatusTests and SmbCommandAdapterTests keep consistent indentation and blank lines, that method signatures like ConfigurableRunner.__init__, run and test methods have proper spacing, and that the call assertions (the list of expected runner.calls in test_validate_config_passes_optional_working_directory) match ruff's preferred wrapping/spacing conventions; after formatting, run the test linter/CI check to confirm the file now passes.tests/test_uninstall.py (1)
317-772:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the cleanup tests fully sandboxed.
These snippets invoke
cleanup_managed_smb_shares()without overriding every destructive path and, in several cases, without stubbingsystemctl. That lets the suite hit real/etc/samba/simple_safer_server_*.conf,/etc/samba/backups, orsystemctl restart smbdon the host running the tests. ScopeSSS_SAMBA_GLOBALS_FILE,SSS_SAMBA_SHARES_FILE,SSS_SAMBA_BACKUP_DIR, andsystemctlto the tempdir in every case.🤖 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 `@tests/test_uninstall.py` around lines 317 - 772, Many tests call cleanup_managed_smb_shares without fully sandboxing destructive targets or systemctl; update each test that invokes cleanup_managed_smb_shares (e.g., tests in this diff) to set SSS_SAMBA_GLOBALS_FILE, SSS_SAMBA_SHARES_FILE, and SSS_SAMBA_BACKUP_DIR to files/dirs inside the test temporary directory and to stub systemctl by placing a fake executable in a temporary bin and exporting PATH (so restarts are captured rather than hitting the host); ensure these environment variables and PATH are exported in the run_bash / run_bash_raw commands that source the UNINSTALL_SCRIPT so cleanup_managed_smb_shares operates only on test-scoped files and uses the stubbed systemctl.
🧹 Nitpick comments (8)
planning-archive/nmbd-wsdd2/samba-effective-config-hardening/PRD.md (1)
5-18: ⚡ Quick winRemove revision-history framing and describe only current behavior.
Line 5 through Line 18 and Line 167 through Line 169 are written as a “past vs now” narrative. Rephrase these sections to describe the current required behavior and constraints directly, without referencing prior implementation state or review history.
As per coding guidelines, "
**/*.{js,ts,jsx,tsx,py,go,java,rb,php,md}: ... Avoid documenting changes like 'previously did x, now does y' - instead just explain current behavior."Also applies to: 167-169
🤖 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 `@planning-archive/nmbd-wsdd2/samba-effective-config-hardening/PRD.md` around lines 5 - 18, Remove the "past vs now" revision-history framing in the opening paragraph that begins with "The Samba discovery/config redesign moved SimpleSaferServer-managed shares..." and the later paragraph around the section that currently reads like prior-vs-current behavior (the block mentioned in the review), and rewrite both as concise requirements describing only the current expected behavior and constraints: state how SimpleSaferServer must detect unmanaged Samba shares (including those from included files), fail smbd startup when core file serving cannot run, handle publish rollback by restarting smbd, and remove owned files on uninstall even if main config is missing; keep tone declarative, drop any mention of prior implementations or changes, and ensure the reworded text uses present-tense requirement language consistent with the rest of the PRD.planning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/01-remove-dead-samba-backup-dir-and-copy-config.md (1)
9-10: ⚡ Quick winUse target-state wording instead of change-history wording.
The “left over... replaced by...” phrasing reads as migration history. Rephrase this section to describe the intended current behavior directly.
As per coding guidelines, "**/*.{js,ts,jsx,tsx,py,go,java,rb,php,md}: When writing comments and documentation, write them assuming the reader has not seen previous versions. Avoid documenting changes like 'previously did x, now does y' - instead just explain current behavior".
🤖 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 `@planning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/01-remove-dead-samba-backup-dir-and-copy-config.md` around lines 9 - 10, Rewrite the sentence that currently reads "Remove the unused backup infrastructure left over from the old backup-before-write pattern that was replaced by the file-path ownership model" into target-state wording that states the intended current behavior; for example, describe that the backup infrastructure is unused and should be removed under the current file-path ownership model instead of mentioning it was “left over” or “replaced.” Update the heading/first paragraph in 01-remove-dead-samba-backup-dir-and-copy-config.md to assert the current action and rationale (e.g., “Remove unused backup infrastructure; current file-path ownership model manages backups and makes this code unnecessary”) without referring to past states or migration history.planning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/04-uninstall-cleans-up-legacy-empty-backup-directory.md (1)
9-12: ⚡ Quick winRewrite this section in current-behavior form.
This paragraph is framed as old-vs-new behavior. Please describe the desired current behavior directly without change-history framing.
As per coding guidelines, "**/*.{js,ts,jsx,tsx,py,go,java,rb,php,md}: When writing comments and documentation, write them assuming the reader has not seen previous versions. Avoid documenting changes like 'previously did x, now does y' - instead just explain current behavior".
🤖 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 `@planning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/04-uninstall-cleans-up-legacy-empty-backup-directory.md` around lines 9 - 12, Rewrite the paragraph to state the current desired behavior directly: the uninstall script should attempt to remove the legacy /etc/samba/backups directory only if it is empty; if the directory contains files (old backups) the script must leave it intact and not report an error, operating silently in that case. Mention the exact path (/etc/samba/backups) and that the removal is conditional on emptiness, avoiding any historical "previously/now" wording.planning-archive/nmbd-wsdd2/samba-pre-submit-cleanup/PRD.md (1)
13-15: ⚡ Quick winMake this doc sentence target-state only.
Line 13-15 is written as transition history. Please rewrite it to state the current intended operator guidance directly.
As per coding guidelines, "**/*.{js,ts,jsx,tsx,py,go,java,rb,php,md}: When writing comments and documentation, write them assuming the reader has not seen previous versions. Avoid documenting changes like 'previously did x, now does y' - instead just explain current behavior".
🤖 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 `@planning-archive/nmbd-wsdd2/samba-pre-submit-cleanup/PRD.md` around lines 13 - 15, Replace the transitional-history sentence in the drive health documentation with a target-state instruction that tells operators to check the current share config location /etc/samba/simple_safer_server_shares.conf (not /etc/samba/smb.conf); remove any phrasing that references previous versions or "previously/now" and instead state directly that the canonical share configuration is in /etc/samba/simple_safer_server_shares.conf and how to inspect it.planning-archive/nmbd-wsdd2/samba-review-round-2-fixes/issues/01-anchor-rsync-excludes-in-install-sh.md (1)
20-21: ⚡ Quick winMake acceptance criteria assert final state directly.
Lines 20–21 can be clearer if written as final conditions (e.g., “main rsync command uses
--exclude='/templates'/--exclude='/static'”) rather than “changed to …”.As per coding guidelines, “When writing comments and documentation, write them assuming the reader has not seen previous versions. Avoid documenting changes like 'previously did x, now does y' - instead just explain current behavior”.
🤖 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 `@planning-archive/nmbd-wsdd2/samba-review-round-2-fixes/issues/01-anchor-rsync-excludes-in-install-sh.md` around lines 20 - 21, Rewrite the acceptance criteria to assert the final state instead of describing a change: replace the "changed to" phrasing on the main rsync command lines with direct statements such as "main rsync command uses --exclude='/templates'" and "main rsync command uses --exclude='/static'"; update the two checklist lines that currently read `--exclude='templates' changed to --exclude='/templates'` and `--exclude='static' changed to --exclude='/static'` to the final-condition wording so the criteria describe the current behavior unambiguously.planning-archive/nmbd-wsdd2/samba-pre-submit-cleanup/issues/02-remove-dead-inline-managed-share-machinery.md (1)
5-7: ⚡ Quick winPhrase the goal in current-state terms, not transition language.
Line 5–7 explains this as “old model replaced by new model.” Reword as the intended architecture/state directly (what SMBManager owns and why), without contrasting with prior behavior.
As per coding guidelines, “When writing comments and documentation, write them assuming the reader has not seen previous versions. Avoid documenting changes like 'previously did x, now does y' - instead just explain current behavior”.
🤖 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 `@planning-archive/nmbd-wsdd2/samba-pre-submit-cleanup/issues/02-remove-dead-inline-managed-share-machinery.md` around lines 5 - 7, Rewrite lines 5–7 to describe the intended/current architecture in present-tense without referencing prior models; state what SMBManager owns (e.g., "SMBManager manages share ownership using a file-path ownership model: it records and enforces owner metadata per path to determine access and lifecycle") and why (e.g., "this ensures a single source of truth for share ownership and simplifies access control and cleanup"). Remove phrases like "old", "replaced", or "previously" and any mention of "marker-based" transition language; focus the sentence on SMBManager's current responsibility and rationale.tests/test_drive_health_docs.py (1)
13-18: ⚡ Quick winMake missing-section failures explicit in test output.
If the heading changes, this helper throws
ValueErrorinstead of failing with a clear assertion message. Prefer an explicit check so test failures are easier to diagnose.Suggested tweak
def _manual_recovery_section(): """Return the Manual Recovery section text from drive_health.md.""" content = DRIVE_HEALTH_MD.read_text() marker = "## Manual Recovery" - start = content.index(marker) + start = content.find(marker) + assert start != -1, "Missing '## Manual Recovery' section in docs/drive_health.md" # Find the next H2 or end of file next_h2 = content.find("\n## ", start + len(marker))🤖 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 `@tests/test_drive_health_docs.py` around lines 13 - 18, The helper that locates a section using content.index(marker) should fail with an explicit assertion instead of letting content.index raise ValueError; change the code around start = content.index(marker) to first check that marker in content (e.g., assert marker in content, f"Missing section heading: {marker}") and then compute start = content.find(marker) (or keep index after the assertion), and keep the existing logic for locating next_h2 so missing headings produce a clear assertion message rather than a ValueError.tests/test_smb_command_adapter.py (1)
25-31: ⚡ Quick winAvoid default-success for unknown commands in the test runner.
Returning success for unmatched commands can hide incorrect command construction and let tests pass unexpectedly. Prefer failing fast on unknown command prefixes.
Suggested tweak
def run(self, command, **kwargs): self.calls.append((command, kwargs)) # Match on the first two args (e.g. ["systemctl", "is-active"]) key = tuple(command[:2]) if len(command) >= 2 else tuple(command) if key in self._results: return self._results[key] - return SimpleNamespace(returncode=0, stdout="", stderr="") + raise AssertionError(f"Unexpected command prefix: {key}; full command: {command}")🤖 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 `@tests/test_smb_command_adapter.py` around lines 25 - 31, The test runner's run method currently returns a default successful SimpleNamespace for unknown command prefixes which can mask bad command construction; update run (the method that computes key from command[:2] and looks up self._results) to fail fast instead of returning success—either raise an AssertionError (including the full command and kwargs in the message) or return a non-zero returncode SimpleNamespace so tests fail when an unexpected command key is encountered; ensure the change affects the branch where "if key in self._results" is false.
🤖 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 `@docs/manual_install.md`:
- Around line 112-114: The docs currently run `systemctl enable nmbd wsdd2` and
`systemctl start nmbd wsdd2` unconditionally; update the instructions to make
`wsdd2` optional by splitting the commands and adding an existence check for
`wsdd2` (e.g., test for the unit or the binary) before running enable/start for
`wsdd2`, while always enabling/starting `nmbd`; reference the `nmbd` and `wsdd2`
service names and show the alternate flow (enable/start only `nmbd` when `wsdd2`
is not present) so manual installs won’t fail on systems without `wsdd2`.
In `@docs/network_file_sharing.md`:
- Around line 29-30: Update the "Restart Services" line to clarify that wsdd2 is
optional and that restarting discovery services should be best-effort or
conditional: explicitly state that users can run "systemctl restart smbd nmbd"
and then optionally restart wsdd2 only if present, or run a conditional restart
check; mention the three unit names `smbd`, `nmbd`, and `wsdd2` and show the
alternative phrasing (separate commands or a note to verify presence) so that
invoking `systemctl restart smbd nmbd wsdd2` on systems without wsdd2 is not
presented as always valid.
In `@install.sh`:
- Around line 416-427: The install script currently uses "systemctl start smbd"
which doesn't reload config if smbd is already active; update the logic around
the smbd startup (the block that sets smbd_enable_failed and smbd_start_failed
and calls systemctl start smbd) to ensure new Samba config is applied after
rewrites by invoking either "smbcontrol smbd reload-config" (preferred
lightweight reload) or "systemctl restart smbd" (full restart) when smbd is
active, and only set smbd_start_failed/smbd_enable_failed if the chosen
reload/restart fails; keep the existing active check ("systemctl is-active
--quiet smbd") and error messages but ensure the reload/restart is attempted
before that check so config changes take effect.
In
`@planning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/01-remove-dead-samba-backup-dir-and-copy-config.md`:
- Line 5: Replace the plain text occurrence
"Planning/samba-post-review-fixes/PRD.md" in the issue doc with a valid relative
Markdown link to the parent PRD so the link is clickable (use
[text](relative/path/to/parent-PRD.md) style); update the text in the file
referenced by this diff so navigation from the issue document works correctly
and verify the link resolves within the repo.
In
`@planning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/02-uninstall-restarts-smbd-after-cleanup.md`:
- Line 5: Update the incorrect parent document reference on line 5 that
currently reads "Planning/samba-post-review-fixes/PRD.md" so it points to the
actual repo path and is formatted as a proper Markdown link (e.g.,
[PRD](actual/repo/path/PRD.md)); locate the reference in the file and replace
the plain text path with the markdown-style link to ensure clickable navigation
and correct repository location.
In
`@planning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/03-uninstall-warns-broken-includes-on-malformed-markers.md`:
- Line 5: Update the Parent reference on line 5 which currently reads
"Planning/samba-post-review-fixes/PRD.md" to point to the correct
planning-archive path and make it a markdown link; replace that plain path with
a Markdown link such as "[PRD](planning-archive/…/PRD.md)" (use the actual
correct subpath in planning-archive) so the Parent is a valid clickable link.
In
`@planning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/04-uninstall-cleans-up-legacy-empty-backup-directory.md`:
- Line 5: Update the "Parent" reference in the issue doc
04-uninstall-cleans-up-legacy-empty-backup-directory.md so it uses a
repo-relative markdown link to the PRD file (PRD.md) instead of the incorrect
path; replace the current plain/incorrect text on the Parent line with a proper
markdown link like [PRD.md](<correct-repo-relative-path-to-PRD.md>) ensuring the
relative path resolves from the issue document's location.
In `@simple_safer_server/adapters/smb_commands.py`:
- Around line 39-64: The method currently returns the raw `status` from
`systemctl is-active` (stored in `status`) for existing units, which can be
values like `failed` or `activating`; change the logic so that after calling
`systemctl cat` (the `cat_result` check) you return "inactive" for any
non-"active" `status` instead of returning the raw value—i.e., if `status ==
"active"` return "active", else if `cat_result.returncode == 0` return
"inactive", and only return "unavailable" when `cat_result.returncode != 0`;
update any docstring/comment accordingly and keep references to
`self._command_runner.run`, `unit_name`, `result`, `status`, and `cat_result` to
locate the code.
In `@simple_safer_server/services/smb_manager.py`:
- Around line 867-873: The current except block for statuses["wsdd2"] blindly
treats any exception from command_adapter.unit_status("wsdd2") as the unit being
missing and sets "unavailable"; instead, detect if the exception indicates a
missing unit (same way smbd/nmbd handling does) and only then set
statuses["wsdd2"] = "unavailable", otherwise set statuses["wsdd2"] = "error" and
log the real exception; update the try/except around
command_adapter.unit_status("wsdd2") to distinguish missing-unit errors from
runtime failures, use logger.info for missing-unit messages and logger.error (or
logger.exception) when recording other failures, and mirror the error-handling
pattern used for smbd/nmbd status checks.
In `@tests/test_install_preflight.py`:
- Around line 371-397: Replace the shared temp-file usage for CALLS_PATH in
test_optional_wsdd2_install_warns_and_continues by creating a per-test temporary
path (e.g., via tempfile.TemporaryDirectory() or tempfile.NamedTemporaryFile())
and exporting that path into the snippet instead of the fixed
Path(tempfile.gettempdir()) / "sss-wsdd2-install-calls"; update the snippet
construction so the bash snippet uses the per-test CALLS_PATH and cleans it up,
and apply the same change to the other affected tests that use CALLS_PATH (the
other tests listed in the review) to avoid cross-test collisions when running
tests in parallel.
---
Outside diff comments:
In `@tests/test_smb_command_adapter.py`:
- Around line 18-83: The file fails ruff formatting: run the code formatter
(ruff format) on the test file and fix formatting inconsistencies (whitespace,
line breaks, imports) so it passes `ruff format --check`; specifically ensure
classes ConfigurableRunner and UnitStatusTests and SmbCommandAdapterTests keep
consistent indentation and blank lines, that method signatures like
ConfigurableRunner.__init__, run and test methods have proper spacing, and that
the call assertions (the list of expected runner.calls in
test_validate_config_passes_optional_working_directory) match ruff's preferred
wrapping/spacing conventions; after formatting, run the test linter/CI check to
confirm the file now passes.
In `@tests/test_uninstall.py`:
- Around line 317-772: Many tests call cleanup_managed_smb_shares without fully
sandboxing destructive targets or systemctl; update each test that invokes
cleanup_managed_smb_shares (e.g., tests in this diff) to set
SSS_SAMBA_GLOBALS_FILE, SSS_SAMBA_SHARES_FILE, and SSS_SAMBA_BACKUP_DIR to
files/dirs inside the test temporary directory and to stub systemctl by placing
a fake executable in a temporary bin and exporting PATH (so restarts are
captured rather than hitting the host); ensure these environment variables and
PATH are exported in the run_bash / run_bash_raw commands that source the
UNINSTALL_SCRIPT so cleanup_managed_smb_shares operates only on test-scoped
files and uses the stubbed systemctl.
---
Nitpick comments:
In `@planning-archive/nmbd-wsdd2/samba-effective-config-hardening/PRD.md`:
- Around line 5-18: Remove the "past vs now" revision-history framing in the
opening paragraph that begins with "The Samba discovery/config redesign moved
SimpleSaferServer-managed shares..." and the later paragraph around the section
that currently reads like prior-vs-current behavior (the block mentioned in the
review), and rewrite both as concise requirements describing only the current
expected behavior and constraints: state how SimpleSaferServer must detect
unmanaged Samba shares (including those from included files), fail smbd startup
when core file serving cannot run, handle publish rollback by restarting smbd,
and remove owned files on uninstall even if main config is missing; keep tone
declarative, drop any mention of prior implementations or changes, and ensure
the reworded text uses present-tense requirement language consistent with the
rest of the PRD.
In
`@planning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/01-remove-dead-samba-backup-dir-and-copy-config.md`:
- Around line 9-10: Rewrite the sentence that currently reads "Remove the unused
backup infrastructure left over from the old backup-before-write pattern that
was replaced by the file-path ownership model" into target-state wording that
states the intended current behavior; for example, describe that the backup
infrastructure is unused and should be removed under the current file-path
ownership model instead of mentioning it was “left over” or “replaced.” Update
the heading/first paragraph in
01-remove-dead-samba-backup-dir-and-copy-config.md to assert the current action
and rationale (e.g., “Remove unused backup infrastructure; current file-path
ownership model manages backups and makes this code unnecessary”) without
referring to past states or migration history.
In
`@planning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/04-uninstall-cleans-up-legacy-empty-backup-directory.md`:
- Around line 9-12: Rewrite the paragraph to state the current desired behavior
directly: the uninstall script should attempt to remove the legacy
/etc/samba/backups directory only if it is empty; if the directory contains
files (old backups) the script must leave it intact and not report an error,
operating silently in that case. Mention the exact path (/etc/samba/backups) and
that the removal is conditional on emptiness, avoiding any historical
"previously/now" wording.
In
`@planning-archive/nmbd-wsdd2/samba-pre-submit-cleanup/issues/02-remove-dead-inline-managed-share-machinery.md`:
- Around line 5-7: Rewrite lines 5–7 to describe the intended/current
architecture in present-tense without referencing prior models; state what
SMBManager owns (e.g., "SMBManager manages share ownership using a file-path
ownership model: it records and enforces owner metadata per path to determine
access and lifecycle") and why (e.g., "this ensures a single source of truth for
share ownership and simplifies access control and cleanup"). Remove phrases like
"old", "replaced", or "previously" and any mention of "marker-based" transition
language; focus the sentence on SMBManager's current responsibility and
rationale.
In `@planning-archive/nmbd-wsdd2/samba-pre-submit-cleanup/PRD.md`:
- Around line 13-15: Replace the transitional-history sentence in the drive
health documentation with a target-state instruction that tells operators to
check the current share config location
/etc/samba/simple_safer_server_shares.conf (not /etc/samba/smb.conf); remove any
phrasing that references previous versions or "previously/now" and instead state
directly that the canonical share configuration is in
/etc/samba/simple_safer_server_shares.conf and how to inspect it.
In
`@planning-archive/nmbd-wsdd2/samba-review-round-2-fixes/issues/01-anchor-rsync-excludes-in-install-sh.md`:
- Around line 20-21: Rewrite the acceptance criteria to assert the final state
instead of describing a change: replace the "changed to" phrasing on the main
rsync command lines with direct statements such as "main rsync command uses
--exclude='/templates'" and "main rsync command uses --exclude='/static'";
update the two checklist lines that currently read `--exclude='templates'
changed to --exclude='/templates'` and `--exclude='static' changed to
--exclude='/static'` to the final-condition wording so the criteria describe the
current behavior unambiguously.
In `@tests/test_drive_health_docs.py`:
- Around line 13-18: The helper that locates a section using
content.index(marker) should fail with an explicit assertion instead of letting
content.index raise ValueError; change the code around start =
content.index(marker) to first check that marker in content (e.g., assert marker
in content, f"Missing section heading: {marker}") and then compute start =
content.find(marker) (or keep index after the assertion), and keep the existing
logic for locating next_h2 so missing headings produce a clear assertion message
rather than a ValueError.
In `@tests/test_smb_command_adapter.py`:
- Around line 25-31: The test runner's run method currently returns a default
successful SimpleNamespace for unknown command prefixes which can mask bad
command construction; update run (the method that computes key from command[:2]
and looks up self._results) to fail fast instead of returning success—either
raise an AssertionError (including the full command and kwargs in the message)
or return a non-zero returncode SimpleNamespace so tests fail when an unexpected
command key is encountered; ensure the change affects the branch where "if key
in self._results" is false.
🪄 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
Run ID: a0b58e0d-17b8-4659-9e25-d639e0b22460
📒 Files selected for processing (58)
docs/dashboard.mddocs/drive_health.mddocs/install.mddocs/manual_install.mddocs/network_file_sharing.mddocs/uninstall.mdindex.htmlinstall.shplanning-archive/nmbd-wsdd2/samba-discovery-config-redesign/PRD.mdplanning-archive/nmbd-wsdd2/samba-discovery-config-redesign/issues/01-establish-sss-owned-samba-layout.mdplanning-archive/nmbd-wsdd2/samba-discovery-config-redesign/issues/02-move-managed-shares-to-sss-shares-file.mdplanning-archive/nmbd-wsdd2/samba-discovery-config-redesign/issues/03-install-and-report-discovery-services.mdplanning-archive/nmbd-wsdd2/samba-discovery-config-redesign/issues/04-track-three-file-sharing-services-in-web-ui.mdplanning-archive/nmbd-wsdd2/samba-discovery-config-redesign/issues/05-clean-up-sss-owned-samba-config-on-uninstall.mdplanning-archive/nmbd-wsdd2/samba-discovery-config-redesign/issues/06-document-samba-ownership-and-discovery.mdplanning-archive/nmbd-wsdd2/samba-discovery-config-redesign/issues/README.mdplanning-archive/nmbd-wsdd2/samba-effective-config-hardening/PRD.mdplanning-archive/nmbd-wsdd2/samba-effective-config-hardening/issues/01-detect-unmanaged-samba-shares-from-effective-config.mdplanning-archive/nmbd-wsdd2/samba-effective-config-hardening/issues/02-use-effective-unmanaged-share-detection-in-smb-writes.mdplanning-archive/nmbd-wsdd2/samba-effective-config-hardening/issues/03-surface-unmanaged-share-verification-state-in-ui.mdplanning-archive/nmbd-wsdd2/samba-effective-config-hardening/issues/04-complete-smb-share-publish-rollback.mdplanning-archive/nmbd-wsdd2/samba-effective-config-hardening/issues/05-honor-required-smbd-failure-in-installer.mdplanning-archive/nmbd-wsdd2/samba-effective-config-hardening/issues/06-clean-owned-samba-files-on-uninstall-independently.mdplanning-archive/nmbd-wsdd2/samba-effective-config-hardening/issues/07-repair-manual-install-documentation.mdplanning-archive/nmbd-wsdd2/samba-effective-config-hardening/issues/README.mdplanning-archive/nmbd-wsdd2/samba-post-review-fixes/PRD.mdplanning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/01-remove-dead-samba-backup-dir-and-copy-config.mdplanning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/02-uninstall-restarts-smbd-after-cleanup.mdplanning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/03-uninstall-warns-broken-includes-on-malformed-markers.mdplanning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/04-uninstall-cleans-up-legacy-empty-backup-directory.mdplanning-archive/nmbd-wsdd2/samba-pre-submit-cleanup/PRD.mdplanning-archive/nmbd-wsdd2/samba-pre-submit-cleanup/issues/01-managed-share-user-helpers-owned-file-first.mdplanning-archive/nmbd-wsdd2/samba-pre-submit-cleanup/issues/02-remove-dead-inline-managed-share-machinery.mdplanning-archive/nmbd-wsdd2/samba-pre-submit-cleanup/issues/03-discovery-service-badges-three-tier.mdplanning-archive/nmbd-wsdd2/samba-pre-submit-cleanup/issues/04-drive-health-docs-sss-shares-file.mdplanning-archive/nmbd-wsdd2/samba-review-round-2-fixes/PRD.mdplanning-archive/nmbd-wsdd2/samba-review-round-2-fixes/issues/01-anchor-rsync-excludes-in-install-sh.mdplanning-archive/nmbd-wsdd2/samba-review-round-2-fixes/issues/02-treat-unavailable-as-non-degrading-in-overall-status.mdplanning-archive/nmbd-wsdd2/samba-review-round-2-fixes/issues/03-change-test-patches-to-assertion-error.mdsimple_safer_server/adapters/smb_commands.pysimple_safer_server/routes/smb.pysimple_safer_server/services/drive_health.pysimple_safer_server/services/runtime.pysimple_safer_server/services/samba_layout.pysimple_safer_server/services/smb_manager.pysimple_safer_server/services/templates/simple_safer_server_globals.conftemplates/dashboard.htmltemplates/network_file_sharing.htmltests/test_app_factory_routes.pytests/test_drive_health.pytests/test_drive_health_docs.pytests/test_install_preflight.pytests/test_samba_layout.pytests/test_setup_wizard.pytests/test_smb_command_adapter.pytests/test_smb_manager.pytests/test_uninstall.pyuninstall.sh
Comment on lines
+112
to
+114
| - Best-effort discovery services can be started with `sudo systemctl enable nmbd wsdd2` and | ||
| `sudo systemctl start nmbd wsdd2`. Warnings or inactive states for these services mean network | ||
| discovery may be degraded, but existing SMB clients can still connect directly when `smbd` is |
Contributor
There was a problem hiding this comment.
Make discovery-service startup commands resilient to missing wsdd2.
This step currently runs systemctl enable/start nmbd wsdd2 unconditionally, which can fail on hosts where wsdd2 is unavailable (even though the doc says it’s optional). Split the commands or add explicit “if installed” checks so manual installs don’t fail on a valid no-wsdd2 setup.
🤖 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 `@docs/manual_install.md` around lines 112 - 114, The docs currently run
`systemctl enable nmbd wsdd2` and `systemctl start nmbd wsdd2` unconditionally;
update the instructions to make `wsdd2` optional by splitting the commands and
adding an existence check for `wsdd2` (e.g., test for the unit or the binary)
before running enable/start for `wsdd2`, while always enabling/starting `nmbd`;
reference the `nmbd` and `wsdd2` service names and show the alternate flow
(enable/start only `nmbd` when `wsdd2` is not present) so manual installs won’t
fail on systems without `wsdd2`.
Comment on lines
+29
to
30
| - **Restart Services**: restarts `smbd`, `nmbd`, and `wsdd2` | ||
| Restarting Samba disconnects anyone who is currently connected to a share, so active file copies or other SMB activity will drop. |
Contributor
There was a problem hiding this comment.
Clarify restart behavior for optional discovery services.
These lines imply wsdd2 is always restartable. On systems without wsdd2, systemctl restart smbd nmbd wsdd2 fails. Please document discovery restarts as best-effort (or show separate commands with optional checks) to match the optional-service model.
Also applies to: 208-208
🤖 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 `@docs/network_file_sharing.md` around lines 29 - 30, Update the "Restart
Services" line to clarify that wsdd2 is optional and that restarting discovery
services should be best-effort or conditional: explicitly state that users can
run "systemctl restart smbd nmbd" and then optionally restart wsdd2 only if
present, or run a conditional restart check; mention the three unit names
`smbd`, `nmbd`, and `wsdd2` and show the alternative phrasing (separate commands
or a note to verify presence) so that invoking `systemctl restart smbd nmbd
wsdd2` on systems without wsdd2 is not presented as always valid.
Comment on lines
+416
to
+427
| if ! systemctl enable smbd; then | ||
| smbd_enable_failed=1 | ||
| fi | ||
| if ! systemctl start smbd; then | ||
| smbd_start_failed=1 | ||
| fi | ||
| if ! systemctl is-active --quiet smbd; then | ||
| echo -e "${RED}ERROR: smbd is not active after start.${NC}" | ||
| echo -e "${RED}Samba file serving is required, so installation cannot continue safely.${NC}" | ||
| echo -e "${RED}Run 'systemctl status smbd' and 'journalctl -u smbd --no-pager' to inspect the failure, then rerun the installer after smbd can start.${NC}" | ||
| return 1 | ||
| fi |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does systemctl start smbdapplysmb.confchanges for an already-active Samba service, or issystemctl reload smbd/systemctl restart smbd required after editing Samba include files?
💡 Result:
systemctl start smbd does not reliably “apply” changes to smb.conf/include files for an already-active Samba service. After editing Samba config (including included files), you should request a config reload (e.g., via smbcontrol) or restart, depending on what changed; Samba only reloads server configuration when asked, and active connections do not automatically change their configuration. In practice: - systemctl start smbd: if the service is already running, systemd will not restart it, so it will not pick up changed config unless something else triggers a reload/restart. - systemctl reload smbd: whether this is sufficient depends on how the distro’s systemd unit maps “reload” to an actual mechanism for smbd. Samba’s authoritative guidance is to use smbcontrol to refresh server daemons’ configuration. - systemctl restart smbd (or service smb restart in SysV setups): is the most reliable way to make configuration changes take effect after editing the config file; Samba explicitly notes that issuing a manual restart is effective. Samba’s man-page clarifies the key limitation: server daemons reload configuration when requested, but already active connections do not change their configuration; to request daemons to refresh configuration, use smbcontrol(1). So the recommended answer: use smbcontrol smbd reload-config (or smbcontrol all reload-config if you want to ensure related daemons are reloaded) to refresh the configuration; if your changes still don’t take effect for existing clients or depend on parameters that require new sessions, use systemctl restart smbd for guaranteed effect.
Citations:
- 1: https://www.samba.org/samba/docs/current/man-html/smb.conf.5.html
- 2: https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/5/html/deployment_guide/s1-samba-startstop
- 3: https://serverfault.com/questions/976264/how-can-make-changes-to-my-smb-conf-file-go-live-without-a-reboot
- 4: https://unix.stackexchange.com/questions/104598/reloading-samba4s-smb-conf-without-restarting-the-service
- 5: https://samba.samba.narkive.com/EHQegNPv/smbcontrol-smbd-reload-config-or-service-smbd-reload-doesn-t-reload-include-files
Use smbcontrol or systemctl restart to apply config changes after rewriting Samba configuration.
Line 419 uses systemctl start smbd, which does not apply configuration changes when the service is already active. Samba only reloads configuration when explicitly requested; existing daemons will continue serving the old config. On update/reinstall paths, this step will succeed while smbd keeps the prior configuration until a reload or restart occurs.
After rewriting smb.conf or include files, use:
smbcontrol smbd reload-config(lighter, requests configuration refresh)systemctl restart smbd(guaranteed to apply all changes)
🤖 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 `@install.sh` around lines 416 - 427, The install script currently uses
"systemctl start smbd" which doesn't reload config if smbd is already active;
update the logic around the smbd startup (the block that sets smbd_enable_failed
and smbd_start_failed and calls systemctl start smbd) to ensure new Samba config
is applied after rewrites by invoking either "smbcontrol smbd reload-config"
(preferred lightweight reload) or "systemctl restart smbd" (full restart) when
smbd is active, and only set smbd_start_failed/smbd_enable_failed if the chosen
reload/restart fails; keep the existing active check ("systemctl is-active
--quiet smbd") and error messages but ensure the reload/restart is attempted
before that check so config changes take effect.
|
|
||
| ## Parent | ||
|
|
||
| Planning/samba-post-review-fixes/PRD.md |
Contributor
There was a problem hiding this comment.
Fix the parent reference path/link.
Line 5 uses Planning/samba-post-review-fixes/PRD.md, which does not match this repo’s planning path and is not clickable markdown. Please switch to a valid relative markdown link so navigation works from the issue doc.
🤖 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
`@planning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/01-remove-dead-samba-backup-dir-and-copy-config.md`
at line 5, Replace the plain text occurrence
"Planning/samba-post-review-fixes/PRD.md" in the issue doc with a valid relative
Markdown link to the parent PRD so the link is clickable (use
[text](relative/path/to/parent-PRD.md) style); update the text in the file
referenced by this diff so navigation from the issue document works correctly
and verify the link resolves within the repo.
|
|
||
| ## Parent | ||
|
|
||
| Planning/samba-post-review-fixes/PRD.md |
Contributor
There was a problem hiding this comment.
Correct the Parent doc reference.
Line 5 points to Planning/samba-post-review-fixes/PRD.md; this should be updated to the actual repo path and formatted as a markdown link for reliable navigation.
🤖 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
`@planning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/02-uninstall-restarts-smbd-after-cleanup.md`
at line 5, Update the incorrect parent document reference on line 5 that
currently reads "Planning/samba-post-review-fixes/PRD.md" so it points to the
actual repo path and is formatted as a proper Markdown link (e.g.,
[PRD](actual/repo/path/PRD.md)); locate the reference in the file and replace
the plain text path with the markdown-style link to ensure clickable navigation
and correct repository location.
|
|
||
| ## Parent | ||
|
|
||
| Planning/samba-post-review-fixes/PRD.md |
Contributor
There was a problem hiding this comment.
Update the Parent reference to a valid path/link.
Line 5 currently uses Planning/samba-post-review-fixes/PRD.md; please point this to the correct planning-archive path and make it a markdown link.
🤖 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
`@planning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/03-uninstall-warns-broken-includes-on-malformed-markers.md`
at line 5, Update the Parent reference on line 5 which currently reads
"Planning/samba-post-review-fixes/PRD.md" to point to the correct
planning-archive path and make it a markdown link; replace that plain path with
a Markdown link such as "[PRD](planning-archive/…/PRD.md)" (use the actual
correct subpath in planning-archive) so the Parent is a valid clickable link.
|
|
||
| ## Parent | ||
|
|
||
| Planning/samba-post-review-fixes/PRD.md |
Contributor
There was a problem hiding this comment.
Fix Parent reference path/link for this issue doc.
Line 5 should use the correct repo-relative PRD path and markdown link format so this issue can be navigated directly.
🤖 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
`@planning-archive/nmbd-wsdd2/samba-post-review-fixes/issues/04-uninstall-cleans-up-legacy-empty-backup-directory.md`
at line 5, Update the "Parent" reference in the issue doc
04-uninstall-cleans-up-legacy-empty-backup-directory.md so it uses a
repo-relative markdown link to the PRD file (PRD.md) instead of the incorrect
path; replace the current plain/incorrect text on the Parent line with a proper
markdown link like [PRD.md](<correct-repo-relative-path-to-PRD.md>) ensuring the
relative path resolves from the issue document's location.
Comment on lines
+39
to
+64
| """Return 'active', 'inactive', or 'unavailable' for a systemd unit. | ||
|
|
||
| 'unavailable' means the unit file does not exist on this system (e.g. | ||
| wsdd2 on distros that don't package it). We distinguish this from | ||
| 'inactive' (unit exists but is stopped) by probing with systemctl cat. | ||
| """ | ||
| result = self._command_runner.run( | ||
| self._command("systemctl", "is-active", unit_name), | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=SMB_COMMAND_TIMEOUT_SECONDS, | ||
| ) | ||
| return result.stdout.strip() | ||
| status = result.stdout.strip() | ||
| if status == "active": | ||
| return "active" | ||
| # 'inactive' from is-active covers both stopped units and missing units. | ||
| # Check whether the unit file actually exists on disk. | ||
| cat_result = self._command_runner.run( | ||
| self._command("systemctl", "cat", unit_name), | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=SMB_COMMAND_TIMEOUT_SECONDS, | ||
| ) | ||
| if cat_result.returncode != 0: | ||
| return "unavailable" | ||
| return status |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify call sites and comparisons that may assume normalized states.
rg -n --type=py -C2 '\bunit_status\s*\(|["'\'']inactive["'\'']|["'\'']unavailable["'\'']'Repository: chrismin13/SimpleSaferServer
Length of output: 9134
Normalize non-active existing units to inactive.
At line 64, returning raw is-active output for existing units allows values outside the documented contract of 'active', 'inactive', or 'unavailable' to escape. Since systemctl is-active can return states like failed, reloading, or activating, the method should return normalized "inactive" for any non-active existing unit to maintain consistency with documented behavior and caller expectations.
Suggested diff
if cat_result.returncode != 0:
return "unavailable"
- return status
+ return "inactive"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| """Return 'active', 'inactive', or 'unavailable' for a systemd unit. | |
| 'unavailable' means the unit file does not exist on this system (e.g. | |
| wsdd2 on distros that don't package it). We distinguish this from | |
| 'inactive' (unit exists but is stopped) by probing with systemctl cat. | |
| """ | |
| result = self._command_runner.run( | |
| self._command("systemctl", "is-active", unit_name), | |
| capture_output=True, | |
| text=True, | |
| timeout=SMB_COMMAND_TIMEOUT_SECONDS, | |
| ) | |
| return result.stdout.strip() | |
| status = result.stdout.strip() | |
| if status == "active": | |
| return "active" | |
| # 'inactive' from is-active covers both stopped units and missing units. | |
| # Check whether the unit file actually exists on disk. | |
| cat_result = self._command_runner.run( | |
| self._command("systemctl", "cat", unit_name), | |
| capture_output=True, | |
| text=True, | |
| timeout=SMB_COMMAND_TIMEOUT_SECONDS, | |
| ) | |
| if cat_result.returncode != 0: | |
| return "unavailable" | |
| return status | |
| """Return 'active', 'inactive', or 'unavailable' for a systemd unit. | |
| 'unavailable' means the unit file does not exist on this system (e.g. | |
| wsdd2 on distros that don't package it). We distinguish this from | |
| 'inactive' (unit exists but is stopped) by probing with systemctl cat. | |
| """ | |
| result = self._command_runner.run( | |
| self._command("systemctl", "is-active", unit_name), | |
| capture_output=True, | |
| text=True, | |
| timeout=SMB_COMMAND_TIMEOUT_SECONDS, | |
| ) | |
| status = result.stdout.strip() | |
| if status == "active": | |
| return "active" | |
| # 'inactive' from is-active covers both stopped units and missing units. | |
| # Check whether the unit file actually exists on disk. | |
| cat_result = self._command_runner.run( | |
| self._command("systemctl", "cat", unit_name), | |
| capture_output=True, | |
| text=True, | |
| timeout=SMB_COMMAND_TIMEOUT_SECONDS, | |
| ) | |
| if cat_result.returncode != 0: | |
| return "unavailable" | |
| return "inactive" |
🤖 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 `@simple_safer_server/adapters/smb_commands.py` around lines 39 - 64, The
method currently returns the raw `status` from `systemctl is-active` (stored in
`status`) for existing units, which can be values like `failed` or `activating`;
change the logic so that after calling `systemctl cat` (the `cat_result` check)
you return "inactive" for any non-"active" `status` instead of returning the raw
value—i.e., if `status == "active"` return "active", else if
`cat_result.returncode == 0` return "inactive", and only return "unavailable"
when `cat_result.returncode != 0`; update any docstring/comment accordingly and
keep references to `self._command_runner.run`, `unit_name`, `result`, `status`,
and `cat_result` to locate the code.
Comment on lines
867
to
+873
| try: | ||
| return { | ||
| "smbd": self.command_adapter.unit_status("smbd"), | ||
| "nmbd": self.command_adapter.unit_status("nmbd"), | ||
| } | ||
| statuses["wsdd2"] = self.command_adapter.unit_status("wsdd2") | ||
| except Exception as exc: | ||
| logger.error("Error getting service status: %s", exc) | ||
| return {"smbd": "error", "nmbd": "error"} | ||
| # wsdd2 is optional on older distributions, so a missing unit is | ||
| # surfaced as unavailable instead of collapsing the whole response. | ||
| logger.info("wsdd2 status unavailable: %s", exc) | ||
| statuses["wsdd2"] = "unavailable" |
Contributor
There was a problem hiding this comment.
Differentiate missing wsdd2 unit from real status-check failures.
Line 867 currently maps every wsdd2 status exception to "unavailable", which can hide real runtime failures as if the unit is merely absent. Keep "unavailable" only for missing-unit cases, and report other failures as "error" (consistent with smbd/nmbd handling).
Suggested patch
try:
statuses["wsdd2"] = self.command_adapter.unit_status("wsdd2")
except Exception as exc:
- # wsdd2 is optional on older distributions, so a missing unit is
- # surfaced as unavailable instead of collapsing the whole response.
- logger.info("wsdd2 status unavailable: %s", exc)
- statuses["wsdd2"] = "unavailable"
+ message = str(exc).lower()
+ if "not found" in message or "could not be found" in message:
+ # wsdd2 is optional on older distributions.
+ logger.info("wsdd2 status unavailable: %s", exc)
+ statuses["wsdd2"] = "unavailable"
+ else:
+ logger.error("Error getting wsdd2 status: %s", exc)
+ statuses["wsdd2"] = "error"🤖 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 `@simple_safer_server/services/smb_manager.py` around lines 867 - 873, The
current except block for statuses["wsdd2"] blindly treats any exception from
command_adapter.unit_status("wsdd2") as the unit being missing and sets
"unavailable"; instead, detect if the exception indicates a missing unit (same
way smbd/nmbd handling does) and only then set statuses["wsdd2"] =
"unavailable", otherwise set statuses["wsdd2"] = "error" and log the real
exception; update the try/except around command_adapter.unit_status("wsdd2") to
distinguish missing-unit errors from runtime failures, use logger.info for
missing-unit messages and logger.error (or logger.exception) when recording
other failures, and mirror the error-handling pattern used for smbd/nmbd status
checks.
Comment on lines
+371
to
+397
| def test_optional_wsdd2_install_warns_and_continues(self): | ||
| snippet = textwrap.dedent( | ||
| f"""\ | ||
| set -e | ||
| {self.installer_function("install_optional_wsdd2")} | ||
| apt-get() {{ | ||
| printf '%s\\n' "$*" >> "$CALLS_PATH" | ||
| return 42 | ||
| }} | ||
| export CALLS_PATH="{Path(tempfile.gettempdir()) / "sss-wsdd2-install-calls"}" | ||
| rm -f "$CALLS_PATH" | ||
| install_optional_wsdd2 | ||
| cat "$CALLS_PATH" | ||
| """ | ||
| ) | ||
|
|
||
| result = subprocess.run( | ||
| ["bash", "-lc", snippet], | ||
| text=True, | ||
| capture_output=True, | ||
| check=False, | ||
| ) | ||
|
|
||
| self.assertEqual(result.returncode, 0, result.stderr + result.stdout) | ||
| self.assertIn("install -y wsdd2", result.stdout) | ||
| self.assertIn("Continuing without modern Windows discovery", result.stdout) | ||
|
|
Contributor
There was a problem hiding this comment.
Use a per-test CALLS_PATH.
These tests write to fixed files under the shared system temp directory. Parallel runs can clobber each other’s call logs and make the stdout assertions flaky. Allocate CALLS_PATH from a per-test TemporaryDirectory() or NamedTemporaryFile instead.
Also applies to: 405-439, 452-488, 489-525, 526-570, 571-609
🤖 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 `@tests/test_install_preflight.py` around lines 371 - 397, Replace the shared
temp-file usage for CALLS_PATH in
test_optional_wsdd2_install_warns_and_continues by creating a per-test temporary
path (e.g., via tempfile.TemporaryDirectory() or tempfile.NamedTemporaryFile())
and exporting that path into the snippet instead of the fixed
Path(tempfile.gettempdir()) / "sss-wsdd2-install-calls"; update the snippet
construction so the bash snippet uses the per-test CALLS_PATH and cleans it up,
and apply the same change to the other affected tests that use CALLS_PATH (the
other tests listed in the review) to avoid cross-test collisions when running
tests in parallel.
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
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.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes / UI