Skip to content

fix(config): warn on a relative PAD_DATA_DIR / PAD_DB_PATH; anchor install-refresh's marker and config reads (BUG-4066 phase 1) - #2308

Merged
xarmian merged 7 commits into
mainfrom
rook/bug-4066-relative-data-dir
Oct 25, 2026
Merged

xarmian merged 7 commits into
mainfrom
rook/bug-4066-relative-data-dir

Conversation

@xarmian

@xarmian xarmian commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes BUG-4066 (phase 1; phase 2 is TASK-4072).

config.Load takes PAD_DATA_DIR verbatim, or PAD_DB_PATH's dirname, with no filepath.Abs. So every process resolves a relative value against its own cwd, and a CLI and a server started in different directories silently use different data: the database, the attachments, the key.

Phase 1 (lead ruling): resolution unchanged, plus a loud warning.

  • One helper, envDataDir(), is the only reader of both variables; Load and userConfigPath use it. Every input resolves exactly as on main.
  • A relative value warns once per process. The warning names the absolute data dir this process resolved (for PAD_DB_PATH, the directory of the file, not the file), says a CLI and a server in different directories will use different data, and says a future release refuses it. A literal ~ gets a hint.
  • Where it goes:
    • a CLI prints it to stderr;
    • pad server start logs it through slog;
    • pad mcp serve prints it once on its own stderr. Its pad children (tool calls, the resource and bootstrap fetchers) run with an internal PAD_INTERNAL_NO_DATA_DIR_WARNING=1, so the warning never lands in an agent-facing error;
    • a server auto-started by the CLI does not inherit that variable, so it still logs the warning even when an MCP child started it;
    • silent for pad watch --stream --for-session (the plugin monitor) and for shell completion (__complete / __completeNoDesc).
  • install-refresh.sh anchors the refresh marker and config_value the way section 2a anchors the log (${SERVER_CWD:-$PWD}). The marker is now written and cleaned up in one place; before, it leaked under the caller's cwd. The script warns once the server's cwd is captured, before the marker is written or anything is stopped, naming the server's path, with . and .. collapsed. It is one line, like the Go warning: when the caller's directory differs, that reading rides inside it.
  • docs/deployment.md: both variables must be absolute; relative is deprecated and warns.

Tests

  • The warning rule, including PAD_DB_PATH naming the data dir; Windows drive-relative and rooted paths are GOOS-guarded.
  • The marker is present where the server looks while the restarted stub starts, and gone from both roots afterwards; the warning comes before the stop; both end to end.
  • config_value at function level.
  • Child-process tests: the monitor and completion stay silent (the completion cases run the real main()), the server logs through slog, and mcp serve warns once.
  • Source scans: every pad exec in internal/mcp sets the child env, and internal/cli has exactly one server start spawn, which drops only the internal variable.

Harness guard. Every install-refresh exec in internal/buildtools passes requireSafeScriptEnv first. It refuses an environment where:

  • the resolved port is missing (the script would fall back to 7777) or is numerically 7777 (07777 too). That check runs before any dial, so the guard never dials 7777;
  • the config file's port is not a whole number (read the way the script reads it);
  • PAD_HOST is set to anything but loopback;
  • HOME or the data dir is the real user's, or is outside this test's temp dir;
  • the port answers on a LAN address without this test's random stub token.

A source scan checks that each exec is guarded in its own function before it runs. The scan has its own self-test.

The guard exists because this unit's first draft of a test stopped the box's shared :7777 server twice. That draft never reached a commit.

Correction to the first commit's message: it says the script "warns before anything is read or stopped". Since r1 it warns once the server's cwd is captured, which comes before the marker and the stop. The text above is the accurate one.

https://claude.ai/code/session_01Hk4vxsaKDm3E1LvsoHtHeV

…B_PATH; install-refresh anchors its marker and config reads (BUG-4066 phase 1)

config.Load takes PAD_DATA_DIR verbatim, or PAD_DB_PATH's dirname, with no
filepath.Abs, so every process resolves a relative value against its own cwd:
a CLI and a server started in different directories silently use different
data (database, attachments, key). Phase 1 (lead ruling 4b4cb4ff) leaves the
resolution unchanged and makes it loud:

- One helper, envDataDir(), is the only reader of the two variables (Load and
  userConfigPath both use it). A relative value warns ONCE per process,
  naming the absolute path this process resolved, that a CLI and a server in
  different directories will use different data, and that a future release
  refuses it (TASK-4072). A literal '~' gets a hint. A CLI prints it to
  stderr; `pad server start` logs it through slog; `pad watch --stream
  --for-session` (the plugin monitor) suppresses it.
- install-refresh anchors the refresh marker and config_value the way 2a
  anchors the log (${SERVER_CWD:-$PWD}), so the marker is written and
  cleaned up in one place (it used to leak under the caller's cwd), and warns
  before anything is read or stopped.
- docs/deployment.md: both variables must be absolute; relative is
  deprecated and warns.

Tests: the warning rule (and Windows, GOOS-guarded), the marker and the
warning-before-stop end to end, config_value at function level, and
child-process tests for the monitor's silence and the server's slog line.

Harness guard (after this unit's draft test stopped the box's shared :7777
server twice): every install-refresh exec in internal/buildtools now passes
requireSafeScriptEnv first, which refuses an environment whose resolved port
is missing (the script would fall back to 7777) or is 7777, whose HOME or
data dir is the real user's or outside a test temp dir, or whose port answers
on a LAN address without this test's random stub token.

Claude-Session: https://claude.ai/code/session_01Hk4vxsaKDm3E1LvsoHtHeV
…er's path and stays out of agent-facing errors; harden the test guard (BUG-4066 r1)

r1 rulings (c0ef54e8):
- install-refresh's warning now runs once SERVER_CWD is captured (still
  before the marker and the stop) and names the anchored path
  (${SERVER_CWD:-$PWD}/<dir>, a leading ./ stripped); when the caller's cwd
  differs it also names the caller's reading. It used to name $PWD/<dir>,
  which the server does not use.
- `pad mcp serve` checks the data dir once on its own stderr; the stdio
  dispatcher's `pad` children get PAD_INTERNAL_NO_DATA_DIR_WARNING=1, so the
  warning is not folded into a failed tool call's agent-facing error. The
  variable is internal (code-documented only), so users cannot silence the
  deprecation. Shell completion no longer prints the warning mid-Tab.
- Test guard: refuses a relative data dir with a config-sourced port (the
  script reads that port from the server's cwd, which the guard cannot see);
  compares symlink-resolved temp paths (macOS); asks for the stub token on
  the LAN address that answered; compares 7777 numerically; and a source-scan
  test fails if any install-refresh exec in the package is unguarded.

Claude-Session: https://claude.ai/code/session_01Hk4vxsaKDm3E1LvsoHtHeV
… completion and MCP fetchers stay quiet; harden the test guard (BUG-4066 r2)

- PAD_DB_PATH warning names the resolved data dir (Dir of the value), in Go and in install-refresh.sh; the script normalises . and .. in pure shell.
- Shell completion (__complete, __completeNoDesc) sets the silent sink in main.
- MCP resource and bootstrap fetchers exec with dispatchChildEnv; a population scan pins every pad exec in internal/mcp.
- Harness guard: numeric 7777 refusal before any dial (asserted by message), PAD_HOST must be empty or loopback, temp-dir check scoped to this test, hardened and self-tested source scan, marker-presence probe in the restarted stub, child timeout, sink reset.

Claude-Session: https://claude.ai/code/session_01Hk4vxsaKDm3E1LvsoHtHeV
…ta dir; completion and MCP fetchers stay quiet; harden the test guard (BUG-4066 r2)

test(buildtools): the config-file 7777 row asserts the numeric refusal by message too (BUG-4066 r2 item 12, same class)
m24: a scanner self-test case whose variable never runs in its function,
with a guard in a later function, so the function boundary is what flags it.
m28: insideTestTempDir refuses another test's directory, and admits a
subtest's, which t.TempDir names <Parent><sub...> with no separator.
m15: the --workspace completion func no longer sets its own sink. main()
silences every completion request, which made that line dead code.

Claude-Session: https://claude.ai/code/session_01Hk4vxsaKDm3E1LvsoHtHeV
…ing silencer; the guard reads the config port like the script (BUG-4066 r3)

Item 2: an MCP tool-call child carries PAD_INTERNAL_NO_DATA_DIR_WARNING=1
and is often the process that auto-starts the server, so that long-lived
server never logged the relative data-dir warning. serverStartCommand drops
that one variable from the spawn env, and keeps every other one. It is the
only `server start` spawn in internal/cli; a source test pins both facts.

Item 7: the harness guard took the leading digits of config.toml's port, so
"8080abc" read as 8080 where install-refresh rejects it and falls back to
7777. It now reads the first port line the way config_value does, and a
non-numeric value is refused as unreadable.

Claude-Session: https://claude.ai/code/session_01Hk4vxsaKDm3E1LvsoHtHeV
… completion tests run main() (BUG-4066 r4)

Item 1: when the server's cwd differs from the caller's, the warning printed
a second "Warning:" line naming the shell's-directory reading, and a third
for a leading ~. Phase 1's contract is one warning per process, as
config.Load prints it, so both now ride in the one line. The test asserts
both paths sit in that line and that it prints once.

Item 6: the completion child cases called silenceWarningsForCompletion
themselves, so deleting main()'s call passed every test. They now set
os.Args and run main().

Claude-Session: https://claude.ai/code/session_01Hk4vxsaKDm3E1LvsoHtHeV
@xarmian
xarmian force-pushed the rook/bug-4066-relative-data-dir branch from 1abfe6e to f4d4b7c Compare October 25, 2026 03:59
@xarmian
xarmian marked this pull request as ready for review October 25, 2026 03:59
@xarmian
xarmian merged commit cba880c into main Oct 25, 2026
12 checks passed
@xarmian
xarmian deleted the rook/bug-4066-relative-data-dir branch October 25, 2026 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant