feat: Enhance door system with DOS emulation, single-instance locking, and access control#67
Open
robbiew wants to merge 7 commits intostlalpha:mainfrom
Open
feat: Enhance door system with DOS emulation, single-instance locking, and access control#67robbiew wants to merge 7 commits intostlalpha:mainfrom
robbiew wants to merge 7 commits intostlalpha:mainfrom
Conversation
…e handling - Added `acquireDoorLock` and `releaseDoorLock` functions to manage door access and prevent concurrent usage. - Introduced `dosDropfileName` function to determine the appropriate dropfile name based on configuration. - Enhanced `executeDOSDoor` and `executeNativeDoor` functions to support dropfile generation and placeholder substitution. - Updated door configuration to include `min_access_level` and `single_instance` options. - Implemented cleanup commands for doors post-execution. - Improved error handling and logging for door access and execution. - Added support for socket I/O mode in native doors. - Updated Windows door execution to support STDIO redirection and environment variable management. - Refactored door information display to include new configuration options. - Added documentation for keeping binaries updated and managing symlinks for development.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds per-door DOS support, per-node dropfile and dosemurc generation, single-instance locking and access-level gating, Windows native door execution and cleanup hooks, moves SSH host-key check into SSH startup, and includes Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Menu as Menu Handler
participant Config as Config
participant FS as File System
participant Door as Door Executor
participant PTY as PTY/Socket
participant Cleanup as Cleanup
User->>Menu: Request to open door (door, node)
Menu->>Config: Load DoorConfig
Menu->>Door: Start execution (user, node)
Door->>Config: Verify MinAccessLevel
alt access denied
Door-->>User: DoorAccessDenied message
else access allowed
Door->>Door: acquireDoorLock
alt lock held
Door-->>User: DoorBusyFormat message
else lock acquired
Door->>FS: Create per-node dirs & write dropfile/dosemurc
alt DOS door
Door->>PTY: Launch dosemu2 (PTY or SOCKET bridge)
PTY-->>Door: I/O and boot gating
else Native door
Door->>PTY: Launch native process (optional shell)
PTY-->>Door: I/O and exit
end
Door->>Cleanup: Run CleanupCommand (optional)
Door->>Door: releaseDoorLock
Door-->>User: Return exit status
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
Pull request overview
Enhances ViSiON/3’s door subsystem with expanded DOS door support (dosemu2), new door configuration options (access control, locking, cleanup, shell execution, socket I/O), Windows native door execution improvements, and corresponding updates to docs/templates and the TUI config editor.
Changes:
- Adds DOS door execution improvements (per-node dosemurc generation, FOSSIL support, dropfile handling, PTY boot-text gating, relative
drive_c_path, globaldosemuPath). - Introduces new door features:
single_instancelocking,min_access_level,cleanup_command/cleanup_args,use_shell, dropfile location, and socket I/O mode. - Updates config editor + documentation + menus/templates to expose and describe the new configuration options.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/configs/strings.json | Adds user-facing door access-denied and door-busy strings |
| templates/configs/doors.json | Updates example door config with new fields (DOS + access/locking/cleanup) |
| menus/v3/cfg/DOORSM.CFG | Updates doors menu configuration to a LORD entry |
| menus/v3/ansi/DOORSM.ANS | Adds ANSI screen for doors menu |
| internal/menu/executor.go | Enforces per-door minimum access level for DOOR: runnable |
| internal/menu/door_lock.go | Adds single-instance lock helpers (in-memory map + mutex) |
| internal/menu/door_handler_windows.go | Adds Windows native door execution via stdio, plus access filtering, locking, cleanup |
| internal/menu/door_handler_dropfiles.go | Adds placeholders for startup dir and dropfile paths |
| internal/menu/door_handler.go | Major door execution enhancements (dosemu2 integration, dropfile location, socket I/O, locking, cleanup, use_shell) |
| internal/configeditor/update_syscfg.go | Expands key range for sysconfig menu navigation |
| internal/configeditor/model.go | Adds “DOS Emulation” system config screen |
| internal/configeditor/fields_system.go | Adds system-wide dosemuPath field |
| internal/configeditor/fields_doors.go | Adds new door fields (dropfile location, min access, locking, cleanup, DOS fields, use_shell) |
| internal/config/config.go | Adds new config fields for doors + server-wide dosemuPath |
| docs/sysop/reference/api-reference.md | Updates DoorConfig reference with new fields |
| docs/sysop/how-to-guides/keeping-binaries-updated.md | Adds new sysop guide for updating binaries via symlinks |
| docs/sysop/how-to-guides/README.md | Links to the new guide |
| docs/sysop/doors/doors.md | Major door documentation updates (DOS emulation, placeholders, locking, access control, etc.) |
| docs/sysop/configuration/configuration.md | Updates configuration docs for new door fields and global dosemuPath |
| docs/sysop/_sidebar.md | Adds link to new how-to guide |
| cmd/vision3/main.go | Moves SSH host key checks under SSHEnabled gate |
| Dockerfile | Copies third_party/ before go mod download to satisfy local replaces |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+402
to
+410
| // Prepare command — optionally wrap in shell | ||
| var cmd *exec.Cmd | ||
| if doorConfig.UseShell { | ||
| shellCmd := doorConfig.Command + " " + strings.Join(substitutedArgs, " ") | ||
| cmd = exec.Command("/bin/sh", "-c", shellCmd) | ||
| log.Printf("DEBUG: Node %d: Using shell execution: /bin/sh -c %q", ctx.NodeNumber, shellCmd) | ||
| } else { | ||
| cmd = exec.Command(doorConfig.Command, substitutedArgs...) | ||
| } |
There was a problem hiding this comment.
use_shell builds a single shell command string by concatenating doorConfig.Command and substitutedArgs, then executes it via /bin/sh -c. Because substitutedArgs/environment placeholders can include user-controlled values (e.g., handle/real name), this enables command injection when a sysop enables use_shell and uses placeholders. Prefer exec.Command with direct argv; if shell wrapping is required, shell-escape/quote each argument (platform-appropriate) and document the risk prominently.
Comment on lines
+282
to
+291
| // Display each door (skip doors the user lacks access to) | ||
| midTemplate := string(ansi.ReplacePipeCodes(midBytes)) | ||
| for i, name := range doorNames { | ||
| displayIdx := 0 | ||
| for _, name := range doorNames { | ||
| doorCfg := doorRegistryCopy[name] | ||
|
|
||
| // Filter out doors the user doesn't have access to | ||
| if doorCfg.MinAccessLevel > 0 && currentUser.AccessLevel < doorCfg.MinAccessLevel { | ||
| continue | ||
| } |
There was a problem hiding this comment.
runListDoors filters out doors below MinAccessLevel, but the “no doors configured” message is still based on the total registry size, not the number actually displayed. This can leave users with a blank list if all doors are restricted. Track displayed count (displayIdx) and use that to decide whether to show DoorNoneConfigured (or a dedicated “no accessible doors” message).
menus/v3/cfg/DOORSM.CFG
Outdated
| "CMD": "DOOR:LORD", | ||
| "ACS": "*", | ||
| "HIDDEN": false, | ||
| "NODE_ACTIVITY": "In MRC Chat" |
There was a problem hiding this comment.
DOORSM.CFG launches LORD but NODE_ACTIVITY is still set to "In MRC Chat" (left over from the removed MRC entry). Update NODE_ACTIVITY to reflect door gameplay (e.g., "Playing Door Game" / "Playing LORD") so node status is accurate.
Suggested change
| "NODE_ACTIVITY": "In MRC Chat" | |
| "NODE_ACTIVITY": "Playing LORD" |
internal/menu/door_handler.go
Outdated
Comment on lines
44
to
46
| // writeBatchFile generates EXTERNAL.BAT for dosemu2 execution. | ||
| // driveCNodeDir is the Linux path to the per-node temp directory inside drive_c. | ||
| func writeBatchFile(ctx *DoorCtx, batchPath, driveCNodeDir string) error { |
There was a problem hiding this comment.
writeBatchFile still takes a driveCNodeDir parameter and its comment says it maps a D: drive, but the implementation no longer uses this argument (the lredir line was removed). Consider removing the unused parameter and updating the comment to match the current batch contents to avoid confusion.
internal/menu/door_handler.go
Outdated
| } | ||
| return executeNativeDoor(ctx) | ||
|
|
||
| // Run cleanup command after door exits (before dropfile cleanup) |
There was a problem hiding this comment.
The cleanup command is executed after executeNativeDoor/executeDOSDoor returns, but those functions clean up dropfiles via deferred removals before returning. That means cleanup_command cannot reliably access {DROPFILE} / {NODEDIR} artifacts, and the comment here (“before dropfile cleanup”) is inaccurate. If cleanup needs the dropfile/node directory, run cleanup before those deferred removals (e.g., move cleanup execution into the door executor, or delay dropfile cleanup until after executeCleanup).
Suggested change
| // Run cleanup command after door exits (before dropfile cleanup) | |
| // Run cleanup command after the door executor returns. | |
| // Note: executeDOSDoor/executeNativeDoor may already have removed any | |
| // dropfiles/node directories via deferred cleanup before this point, so | |
| // cleanup_command must not rely on {DROPFILE} / {NODEDIR} being present. |
Comment on lines
+114
to
+122
| // Prepare command — optionally wrap in cmd.exe | ||
| var cmd *exec.Cmd | ||
| if doorConfig.UseShell { | ||
| shellCmd := doorConfig.Command + " " + strings.Join(substitutedArgs, " ") | ||
| cmd = exec.Command("cmd", "/c", shellCmd) | ||
| log.Printf("DEBUG: Node %d: Using shell execution: cmd /c %q", ctx.NodeNumber, shellCmd) | ||
| } else { | ||
| cmd = exec.Command(doorConfig.Command, substitutedArgs...) | ||
| } |
There was a problem hiding this comment.
use_shell builds a single command line string and runs it via cmd /c. Since placeholders can include user-controlled values, this enables command injection if a sysop enables use_shell and uses placeholders in args/env. Prefer direct exec.Command(argv) where possible; if cmd.exe wrapping is necessary, ensure each argument is quoted/escaped for cmd.exe and document the security implications.
docs/sysop/doors/doors.md
Outdated
| ### How It Works | ||
|
|
||
| 1. ViSiON/3 generates a per-node `dosemurc` config that maps `drive_c_path` as the DOS C: drive | ||
| 2. An `EXTERNAL.BAT` batch file is generated containing: FOSSIL driver loading (if configured), `cls`, dropfile copy (if `dropfile_dest` is set), and the user's `dos_commands` |
There was a problem hiding this comment.
The “How It Works” section says EXTERNAL.BAT is generated with cls and then dropfile copy, but the implementation writes the COPY (with >NUL) before cls. Either update the documentation to match the actual order, or reorder the batch generation to match the documented sequence (especially since cls is also used as the output gate trigger).
Suggested change
| 2. An `EXTERNAL.BAT` batch file is generated containing: FOSSIL driver loading (if configured), `cls`, dropfile copy (if `dropfile_dest` is set), and the user's `dos_commands` | |
| 2. An `EXTERNAL.BAT` batch file is generated containing: FOSSIL driver loading (if configured), dropfile copy (if `dropfile_dest` is set), `cls`, and the user's `dos_commands` |
internal/menu/door_handler.go
Outdated
Comment on lines
+183
to
+187
| // Use custom config if specified, otherwise the generated per-node config | ||
| rcPath := nodeRcPath | ||
| if ctx.Config.DosemuConfig != "" { | ||
| rcPath = ctx.Config.DosemuConfig | ||
| } |
There was a problem hiding this comment.
In executeDOSDoor, a per-node dosemurc is generated (nodeRcPath) that appends required overrides, but when DoorConfig.DosemuConfig is set you then pass the base config path to dosemu (-f), ignoring the generated file. This drops the per-node overrides (drive_c mapping, lredir paths, terminal translator settings) and likely breaks door execution. Use the generated nodeRcPath for -f, and treat DoorConfig.DosemuConfig only as an optional base template that writeDosemurc reads/merges.
internal/menu/door_lock.go
Outdated
Comment on lines
+8
to
+22
| doorLocksMu sync.Mutex | ||
| doorLocks = make(map[string]int) // door name (uppercase) -> node number | ||
| ) | ||
|
|
||
| // acquireDoorLock attempts to acquire a lock for the given door. | ||
| // Returns true if the lock was acquired, false if another node holds it. | ||
| func acquireDoorLock(doorName string, nodeNumber int) bool { | ||
| doorLocksMu.Lock() | ||
| defer doorLocksMu.Unlock() | ||
|
|
||
| if holder, exists := doorLocks[doorName]; exists && holder != nodeNumber { | ||
| return false | ||
| } | ||
| doorLocks[doorName] = nodeNumber | ||
| return true |
There was a problem hiding this comment.
Single-instance locking keys the map by the doorName string as provided, but the comment indicates uppercase while acquireDoorLock/releaseDoorLock do not normalize. Because some call sites pass un-normalized names (e.g., DOOR: runnable passes the raw menu token), different casings can bypass the lock. Normalize (e.g., strings.ToUpper) inside the lock functions (or ensure all callers pass a canonical value) so SingleInstance enforcement is consistent.
Comment on lines
+703
to
+707
| if ctx.Config.SingleInstance { | ||
| if !acquireDoorLock(ctx.DoorName, ctx.NodeNumber) { | ||
| log.Printf("WARN: Node %d: Door '%s' is already in use by another node", ctx.NodeNumber, ctx.DoorName) | ||
| return fmt.Errorf("door '%s' is currently in use by another node", ctx.DoorName) | ||
| } |
There was a problem hiding this comment.
When SingleInstance lock acquisition fails, this returns a raw fmt.Errorf message. The UI layer then wraps it in DoorErrorFormat, but the repo now has a dedicated DoorBusyFormat string (with ANSI pipe codes) that is never used. Consider returning a recognizable error (or handling the busy case here) so the caller can display DoorBusyFormat to the user instead of a generic error wrapper.
![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: 9
🧹 Nitpick comments (1)
internal/configeditor/fields_doors.go (1)
251-256: Consider validating I/O Mode values.The I/O Mode field accepts any string, but only "STDIO" and "SOCKET" are valid. Consider adding validation similar to Dropfile Location.
♻️ Optional: Add I/O Mode validation
row++ fields = append(fields, fieldDef{ Label: "I/O Mode", Help: "I/O handling: STDIO or SOCKET (pass FD to door)", Type: ftString, Col: 3, Row: row, Width: 15, Get: func() string { return dPtr.IOMode }, - Set: func(val string) error { dPtr.IOMode = val; save(); return nil }, + Set: func(val string) error { + val = strings.ToUpper(strings.TrimSpace(val)) + switch val { + case "", "STDIO", "SOCKET": + dPtr.IOMode = val + save() + return nil + default: + return fmt.Errorf("invalid I/O mode %q: must be blank, STDIO, or SOCKET", val) + } + }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/configeditor/fields_doors.go` around lines 251 - 256, The I/O Mode field currently accepts any string; update the Set handler for the fieldDef with Label "I/O Mode" to validate that dPtr.IOMode is only set to "STDIO" or "SOCKET" (case-sensitive or normalized per existing conventions), returning an error if the value is invalid instead of assigning it; on valid input assign to dPtr.IOMode, call save(), and return nil—use the same validation pattern used by the Dropfile Location setter to guide error messaging and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/config.go`:
- Around line 426-427: The new string keys DoorAccessDenied and DoorBusyFormat
are not being backfilled so older strings.json yields empty values; update the
applyStringDefaults function to check for empty or missing entries and assign
sensible default values for DoorAccessDenied and DoorBusyFormat (the same
pattern used for other keys in applyStringDefaults). Locate the struct fields
DoorAccessDenied and DoorBusyFormat in the config strings struct and add the
same conditional default-assignment logic used for other keys so upgrades
populate these two strings when deserializing older configs.
In `@internal/menu/door_handler_windows.go`:
- Around line 408-502: runDoorInfo currently displays door configuration without
verifying the user's access level; after retrieving doorConfig via
e.GetDoorConfig in runDoorInfo, add the same MinAccessLevel check used by
runOpenDoor: if doorConfig.MinAccessLevel > currentUser.AccessLevel then write a
short "access denied" message to the terminal (use an existing localized string
if available, otherwise reuse the DoorNotFoundFormat with inputClean), clear the
input line like other error paths, and continue/return without showing the door
details so users cannot view configs above their access level.
In `@internal/menu/door_handler.go`:
- Around line 662-694: The cleanup command currently runs after executors have
already removed temp files and uses exec.Command + CombinedOutput (which can
hang), so change the flow: stop deferring temp-file removal in executeDOSDoor
and executeNativeDoor and move temp-file cleanup into executeDoor so phases are
ordered as: run door, run cleanup, remove temp artifacts, release lock; modify
executeCleanup to accept a context and use exec.CommandContext with a
configurable timeout (instead of exec.Command/CombinedOutput) and ensure it
substitutes ctx.Subs the same way, logs errors but does not return them, and
that executeDoor calls executeCleanup with a timeout-bound context before it
performs any dropfile/{NODEDIR} removals and lock release.
- Around line 58-64: The emitted COPY command uses raw tokens and will break on
paths with spaces; update the generation in the block that builds the COPY line
(where dosDropfileName, ctx.Subs["{DOSNODEDIR}"], ctx.Config.DropfileDest and
b.WriteString are used) to wrap both src and dest in double quotes (and escape
any interior quotes if necessary) before formatting the string, e.g. produce
COPY "src" "dest" >NUL so paths with spaces like C:\DOORS\Trade Wars are
preserved.
- Around line 183-187: The current code overwrites rcPath with
ctx.Config.DosemuConfig, which discards the per-node overrides that
writeDosemurc already merged into nodeRcPath; instead, remove the conditional
that sets rcPath = ctx.Config.DosemuConfig and always use nodeRcPath (the output
of writeDosemurc) so the generated $_hdimage and $_lredir_paths remain applied;
references: writeDosemurc, nodeRcPath, rcPath, ctx.Config.DosemuConfig.
- Around line 989-1012: The DOORINFO printing block leaks restricted door
metadata; before assembling info from doorConfig (fields like
DropfileLocation/DropfileType, IOMode, DOSCommands, MinAccessLevel,
SingleInstance, UseShell, CleanupCommand) perform the same access check used by
LISTDOORS/OPENDOOR (call the existing door access check function or verify
caller permissions for the requested door) and return/deny if the caller lacks
access; in short, locate the DOORINFO handler that reads doorConfig and gate the
entire info formatting block behind the same access-check routine used by
LISTDOORS/OPENDOOR.
- Around line 402-410: The current shell path builds shellCmd with strings.Join
which loses quoting; instead, when doorConfig.UseShell is true, invoke the shell
with a short script that execs "$@" and pass doorConfig.Command as the script
name/first arg and the substitutedArgs as the remaining argv so argument
boundaries are preserved (i.e. call exec.Command with "/bin/sh", "-c", "exec
\"$@\"", doorConfig.Command, substitutedArgs...); alternatively, if you prefer
not to support exec-through-shell, reject/return an error when
doorConfig.UseShell is true and Args/substitutedArgs are present. Update the
branch that sets cmd (the block referencing doorConfig.UseShell,
doorConfig.Command, substitutedArgs, and cmd) accordingly.
In `@internal/menu/executor.go`:
- Around line 489-499: The access-denied branch in the check (inside the
function handling door access) returns immediately after writing the message,
which lets the UI redraw erase it; update the block around
terminalio.WriteProcessedBytes(s.Stderr(),
ansi.ReplacePipeCodes([]byte(errMsg)), outputMode) to (1) use a safe fallback
string when e.LoadedStrings.DoorAccessDenied is empty, (2) ensure the processed
bytes are flushed and then sleep briefly (e.g., time.Sleep for a short duration)
so the message stays visible, and (3) keep the existing logging and return
behavior; locate this logic by the symbols doorConfig.MinAccessLevel,
currentUser.AccessLevel, errMsg, terminalio.WriteProcessedBytes,
ansi.ReplacePipeCodes, and s.Stderr() and apply the changes there.
In `@menus/v3/cfg/DOORSM.CFG`:
- Around line 3-7: Update the stale NODE_ACTIVITY value for the menu entry that
launches CMD "DOOR:LORD": replace the current "In MRC Chat" string with a status
that reflects the door (for example "Playing LORD") by editing the NODE_ACTIVITY
field for that entry so the activity shown matches the DOOR:LORD action.
---
Nitpick comments:
In `@internal/configeditor/fields_doors.go`:
- Around line 251-256: The I/O Mode field currently accepts any string; update
the Set handler for the fieldDef with Label "I/O Mode" to validate that
dPtr.IOMode is only set to "STDIO" or "SOCKET" (case-sensitive or normalized per
existing conventions), returning an error if the value is invalid instead of
assigning it; on valid input assign to dPtr.IOMode, call save(), and return
nil—use the same validation pattern used by the Dropfile Location setter to
guide error messaging and behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72f0a5b8-3a16-4e76-a75e-b706524974d6
📒 Files selected for processing (22)
Dockerfilecmd/vision3/main.godocs/sysop/_sidebar.mddocs/sysop/configuration/configuration.mddocs/sysop/doors/doors.mddocs/sysop/how-to-guides/README.mddocs/sysop/how-to-guides/keeping-binaries-updated.mddocs/sysop/reference/api-reference.mdinternal/config/config.gointernal/configeditor/fields_doors.gointernal/configeditor/fields_system.gointernal/configeditor/model.gointernal/configeditor/update_syscfg.gointernal/menu/door_handler.gointernal/menu/door_handler_dropfiles.gointernal/menu/door_handler_windows.gointernal/menu/door_lock.gointernal/menu/executor.gomenus/v3/ansi/DOORSM.ANSmenus/v3/cfg/DOORSM.CFGtemplates/configs/doors.jsontemplates/configs/strings.json
internal/menu/door_handler.go
Outdated
Comment on lines
+662
to
+694
| // executeCleanup runs the optional post-door cleanup command. | ||
| // Errors are logged but not returned — cleanup failure should not mask door results. | ||
| func executeCleanup(ctx *DoorCtx) { | ||
| if ctx.Config.CleanupCommand == "" { | ||
| return | ||
| } | ||
|
|
||
| // Substitute placeholders in cleanup args | ||
| substitutedArgs := make([]string, len(ctx.Config.CleanupArgs)) | ||
| for i, arg := range ctx.Config.CleanupArgs { | ||
| newArg := arg | ||
| for key, val := range ctx.Subs { | ||
| newArg = strings.ReplaceAll(newArg, key, val) | ||
| } | ||
| substitutedArgs[i] = newArg | ||
| } | ||
|
|
||
| log.Printf("INFO: Node %d: Running cleanup command for door '%s': %s %v", | ||
| ctx.NodeNumber, ctx.DoorName, ctx.Config.CleanupCommand, substitutedArgs) | ||
|
|
||
| cmd := exec.Command(ctx.Config.CleanupCommand, substitutedArgs...) | ||
| if ctx.Config.WorkingDirectory != "" { | ||
| cmd.Dir = ctx.Config.WorkingDirectory | ||
| } | ||
| cmd.Env = os.Environ() | ||
|
|
||
| if output, err := cmd.CombinedOutput(); err != nil { | ||
| log.Printf("WARN: Node %d: Cleanup command for door '%s' failed: %v (output: %s)", | ||
| ctx.NodeNumber, ctx.DoorName, err, string(output)) | ||
| } else { | ||
| log.Printf("DEBUG: Node %d: Cleanup command for door '%s' completed successfully", ctx.NodeNumber, ctx.DoorName) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Run cleanup_command before tearing down temp files, and bound it with a timeout.
executeDOSDoor and executeNativeDoor both defer their dropfile cleanup internally, so {DROPFILE} and {NODEDIR} may already be gone by the time executeCleanup runs here. CombinedOutput() also waits indefinitely, which can hang the user's return to the BBS and keep a single_instance lock held.
Move the temp-file cleanup out of the executors so executeDoor can order the phases as: run door → run cleanup via exec.CommandContext with a timeout → remove temp artifacts → release the lock.
Also applies to: 701-719
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/menu/door_handler.go` around lines 662 - 694, The cleanup command
currently runs after executors have already removed temp files and uses
exec.Command + CombinedOutput (which can hang), so change the flow: stop
deferring temp-file removal in executeDOSDoor and executeNativeDoor and move
temp-file cleanup into executeDoor so phases are ordered as: run door, run
cleanup, remove temp artifacts, release lock; modify executeCleanup to accept a
context and use exec.CommandContext with a configurable timeout (instead of
exec.Command/CombinedOutput) and ensure it substitutes ctx.Subs the same way,
logs errors but does not return them, and that executeDoor calls executeCleanup
with a timeout-bound context before it performs any dropfile/{NODEDIR} removals
and lock release.
- Fix command injection in use_shell by preserving argument boundaries - Always use generated per-node dosemurc instead of discarding overrides - Quote COPY paths in DOS batch files to handle spaces - Add MinAccessLevel check to runDoorInfo on both Linux and Windows - Fix LISTDOORS "none configured" using total count instead of displayed - Backfill DoorAccessDenied and DoorBusyFormat string defaults - Add fallback string and sleep for access-denied visibility in executor - Use ErrDoorBusy sentinel error with DoorBusyFormat for lock failures - Normalize door lock keys with strings.ToUpper for consistent locking - Remove unused driveCNodeDir parameter from writeBatchFile - Fix stale NODE_ACTIVITY "In MRC Chat" to "Playing LORD" in DOORSM.CFG - Fix docs EXTERNAL.BAT sequence to match actual code order - Add 30s timeout to cleanup commands via context.WithTimeout - Validate I/O Mode (STDIO/SOCKET) in config editor Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/menu/door_lock.go`:
- Around line 14-17: The current process-local mutex and map (doorLocksMu and
doorLocks) only serialize within a single process; replace them with a
system-wide/shared lock primitive (e.g., an OS file lock or a distributed lock
backed by the cluster DB/consensus) so door exclusivity works across processes.
Change any code that reads/writes doorLocks/doorLocksMu (the door lock
acquisition/release logic) to use the chosen shared lock API: create/open a
per-door lock resource (file path or distributed key), acquire a
blocking/advisory lock when entering a door, record node identity in the shared
store if needed, and release/close the shared lock on exit; remove reliance on
the in-memory map and mutex (doorLocks and doorLocksMu) and update callers to
handle lock acquisition failures/timeouts accordingly.
In `@internal/menu/executor.go`:
- Around line 522-534: The code currently logs cmdErr as an ERROR before
checking for ErrDoorBusy; change the order so you test errors.Is(cmdErr,
ErrDoorBusy) first and handle the busy case (use e.LoadedStrings.DoorBusyFormat,
fmt.Sprintf to build busyMsg, terminalio.WriteProcessedBytes, time.Sleep(1s))
without emitting the generic error log, and only call log.Printf for other
errors and then doorErrorMessage for non-busy failures; locate symbols cmdErr,
ErrDoorBusy, e.LoadedStrings.DoorBusyFormat, terminalio.WriteProcessedBytes,
doorErrorMessage to apply the fix.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 985667fc-aff0-4659-9d3c-330d16528380
📒 Files selected for processing (8)
docs/sysop/doors/doors.mdinternal/config/config.gointernal/configeditor/fields_doors.gointernal/menu/door_handler.gointernal/menu/door_handler_windows.gointernal/menu/door_lock.gointernal/menu/executor.gomenus/v3/cfg/DOORSM.CFG
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/configeditor/fields_doors.go
Comment on lines
+14
to
+17
| var ( | ||
| doorLocksMu sync.Mutex | ||
| doorLocks = make(map[string]int) // door name (uppercase) -> node number | ||
| ) |
Contributor
There was a problem hiding this comment.
Use a shared lock primitive here, not a process-local map.
Line 16 makes single_instance effective only inside one server process. A second process can still enter the same door concurrently, which falls short of the file-based/system-wide locking described for this feature and still leaves shared door state exposed to races.
Also applies to: 22-31, 36-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/menu/door_lock.go` around lines 14 - 17, The current process-local
mutex and map (doorLocksMu and doorLocks) only serialize within a single
process; replace them with a system-wide/shared lock primitive (e.g., an OS file
lock or a distributed lock backed by the cluster DB/consensus) so door
exclusivity works across processes. Change any code that reads/writes
doorLocks/doorLocksMu (the door lock acquisition/release logic) to use the
chosen shared lock API: create/open a per-door lock resource (file path or
distributed key), acquire a blocking/advisory lock when entering a door, record
node identity in the shared store if needed, and release/close the shared lock
on exit; remove reliance on the in-memory map and mutex (doorLocks and
doorLocksMu) and update callers to handle lock acquisition failures/timeouts
accordingly.
Door-busy contention is an expected outcome for single-instance doors, not an error. Check for ErrDoorBusy before logging to avoid inflating error noise and triggering false alerts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/menu/door_handler.go (1)
728-731:⚠️ Potential issue | 🟠 MajorThe cleanup hook still runs after the temp artifacts are gone.
executeCleanupis invoked only after the executor returns, but both door executors still tear down their node/dropfile paths before that. Cleanup args like{DROPFILE},{NODEDIR},{DOSDROPFILE}, and{DOSNODEDIR}can therefore resolve to deleted paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/menu/door_handler.go` around lines 728 - 731, The cleanup hook is invoked via executeCleanup only after the door executors (executeDOSDoor/executeNativeDoor) have already removed dropfile/node paths, so cleanup args like {DROPFILE}/{NODEDIR}/{DOSDROPFILE}/{DOSNODEDIR} may point to deleted files; modify the flow so executeCleanup runs before the executors perform their teardown (or preserve and pass resolved temp paths into executeCleanup), i.e., move or call executeCleanup prior to the teardown code in executeDOSDoor/executeNativeDoor (or change those functions to return the preserved path strings and invoke executeCleanup with them) so cleanup substitutions resolve to existing paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/menu/door_handler_windows.go`:
- Around line 76-86: The cleanup hook (executeCleanupWindows) is run after
executeNativeDoorWindows removes dropfilePath/nodeDropfileDir, so any cleanup
args referencing {DROPFILE}/{NODEDIR} can point at deleted paths; modify
executeDoor/executeNativeDoorWindows so executeCleanupWindows is invoked before
deleting dropfilePath and nodeDropfileDir (or move the defer that deletes
dropfilePath/nodeDropfileDir to after executeCleanupWindows), ensuring
executeCleanupWindows runs while dropfilePath and nodeDropfileDir still exist.
- Around line 160-167: The Windows door start path always binds child stdio
(cmd.Stdout/Stderr/Stdin = ctx.Session) but never handles IOMode == "SOCKET",
causing silent misbehavior; add an explicit guard in the function containing
this code (the block using ctx.NodeNumber, ctx.DoorName, cmd, ctx.Session) that
checks ctx.IOMode (or equivalent field) for the value "SOCKET" and returns a
clear error (and/or processLogger/log.Printf) stating SOCKET I/O is unsupported
on Windows and that DOOR_SOCKET_FD is not provided, so the caller fails fast
instead of launching with wrong stdio; ensure the guard runs before setting
cmd.Stdout/Stderr/Stdin or calling cmd.Run().
---
Duplicate comments:
In `@internal/menu/door_handler.go`:
- Around line 728-731: The cleanup hook is invoked via executeCleanup only after
the door executors (executeDOSDoor/executeNativeDoor) have already removed
dropfile/node paths, so cleanup args like
{DROPFILE}/{NODEDIR}/{DOSDROPFILE}/{DOSNODEDIR} may point to deleted files;
modify the flow so executeCleanup runs before the executors perform their
teardown (or preserve and pass resolved temp paths into executeCleanup), i.e.,
move or call executeCleanup prior to the teardown code in
executeDOSDoor/executeNativeDoor (or change those functions to return the
preserved path strings and invoke executeCleanup with them) so cleanup
substitutions resolve to existing paths.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e61e0f2-cb89-47cc-8203-59cd722fb450
📒 Files selected for processing (3)
internal/menu/door_handler.gointernal/menu/door_handler_windows.gointernal/menu/executor.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/menu/executor.go
Move executeCleanup calls from executeDoor into the individual door
executors so cleanup commands run while dropfiles and node directories
still exist. Previously, deferred dropfile cleanup would fire before
the cleanup hook, causing {DROPFILE}/{NODEDIR} placeholders to resolve
to deleted paths.
Also add an explicit error for SOCKET I/O mode on Windows, which is
not yet implemented and would silently fall back to STDIO.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/menu/door_handler_windows.go`:
- Around line 41-47: Create the node temp directory unconditionally when
dropfileLoc == "node" and ensure its cleanup is always scheduled and recursive:
when you call filepath.Join to set nodeDropfileDir and use os.MkdirAll to create
it, immediately defer removal with os.RemoveAll(nodeDropfileDir) (or otherwise
ensure RemoveAll is invoked regardless of which DropfileType/branch is later
taken) so the directory is removed even if it contains files; update both the
earlier block (where nodeDropfileDir is created) and the later analogous block
(lines ~76-85) to use os.RemoveAll and a guaranteed defer so temp dirs are not
leaked.
In `@internal/menu/door_handler.go`:
- Around line 327-333: The code creates nodeDropfileDir (using os.MkdirAll with
filepath.Join(os.TempDir(), fmt.Sprintf("vision3_node%d", ctx.NodeNumber)))
eagerly when dropfileLoc == "node", which can leak stale files because cleanup
is only deferred inside the recognized-dropfile block and os.Remove can't remove
non-empty dirs; change to create a truly ephemeral unique temp directory only
when you know you will write a supported dropfile (use ioutil.TempDir or
os.MkdirTemp) or, if you must reuse a location, ensure you always schedule
os.RemoveAll(nodeDropfileDir) (not os.Remove) in the same scope as creation so
cleanup runs regardless of later control flow; apply the same fix to the
analogous creation/cleanup logic in the second block referenced (lines ~363-374)
so directories are removed recursively and not left with stale contents.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f0f48a3-a354-469b-85df-d1d57d3d1877
📒 Files selected for processing (2)
internal/menu/door_handler.gointernal/menu/door_handler_windows.go
- Replace fixed-path node temp dirs with os.MkdirTemp for unique names - Use unconditional defer os.RemoveAll so dirs are cleaned up even when no recognized dropfile type is configured or doors write extra files - Replace process-local in-memory door lock map with file-based locking (flock on Unix, LockFileEx on Windows) for cross-process safety Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/menu/door_handler_windows.go`:
- Around line 118-125: When doorConfig.UseShell is true (and also when
CleanupCommand points to a .bat/.cmd), do not rely on passing args as a slice to
exec.Command because cmd.exe/batch parsing bypasses Go's CommandLineToArgvW
rules; instead construct a single, properly escaped command line by applying
Windows escaping (use syscall.EscapeArg for each substitutedArg and the
doorConfig.Command), join them with spaces, create the exec.Cmd for the shell
invocation and assign that single string to cmd.SysProcAttr.CmdLine (and
clear/avoid using Args) so the exact escaped command line is sent to cmd.exe;
update the branch that creates cmd when doorConfig.UseShell is true (and the
CleanupCommand handling) to use this escaped CmdLine approach rather than
passing individual slice items to exec.Command.
In `@internal/menu/door_lock.go`:
- Around line 45-59: acquireDoorLock currently collapses all failures into a
false (treated as ErrDoorBusy); change its signature to return error, return
ErrDoorBusy only when tryFileLock indicates the lock is already held, and
propagate real errors from os.MkdirAll, os.OpenFile and any unexpected lock-call
errors (e.g., wrap/return the underlying err). In practice, update
acquireDoorLock to return the error from os.MkdirAll(lockDir, 0700) and from
os.OpenFile(lockPath, ...), ensure f is closed on all error paths, and turn the
tryFileLock result into either nil (success) or ErrDoorBusy (contention) while
returning other errors directly so callers can distinguish filesystem/permission
failures from a busy lock.
- Around line 81-86: The current release logic in the block that checks
doorLockNodes and doorLockFiles calls os.Remove(f.Name()) after
releaseFileLock(f) and f.Close(), which can unlink a newly-created lockfile from
another process and break the single-instance guard; remove the
os.Remove(f.Name()) call so the file path remains stable (keep
releaseFileLock(f), f.Close(), and delete(doorLockFiles, key) as-is) and ensure
no other cleanup deletes the lockfile on a normal release in functions/variables
doorLockNodes, doorLockFiles, releaseFileLock, key, and nodeNumber.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71c8fcb3-3f6e-4f80-af70-c8a8c5d93828
📒 Files selected for processing (5)
internal/menu/door_handler.gointernal/menu/door_handler_windows.gointernal/menu/door_lock.gointernal/menu/door_lock_unix.gointernal/menu/door_lock_windows.go
- Fix lock file race: don't delete lock file on release to prevent TOCTOU race where another process locks then loses its file - Return typed errors from acquireDoorLock: ErrDoorBusy for contention, real errors for filesystem/permission failures - Fix Windows cmd.exe argument quoting: use SysProcAttr.CmdLine with syscall.EscapeArg for shell execution and cleanup commands to prevent special characters (&|^()%!) from being reinterpreted by cmd.exe Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
single_instance: true, with proper cleanup on exitmin_access_levelfor doors,cleanup_command/cleanup_argsfor post-exit housekeeping,use_shelloption, socket I/O mode for native doors, and system-widedosemuPathsettingTest plan
min_access_levelrestricts door access for lower-level users🤖 Generated with Claude Code
Summary by CodeRabbit