Skip to content

🤖 feat: back up Mux settings to a git repository - #3767

Open
ibetitsmike wants to merge 162 commits into
mainfrom
mike/backup-sync-dbgh
Open

🤖 feat: back up Mux settings to a git repository#3767
ibetitsmike wants to merge 162 commits into
mainfrom
mike/backup-sync-dbgh

Conversation

@ibetitsmike

@ibetitsmike ibetitsmike commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds phase 1 of settings backup: a Backup settings tab that pushes a portable subset of ~/.mux to a git repository the user specifies, and restores it on another deployment. Backup and restore are manual, the repository is addressed by URL rather than a new sign-in flow, and MCP commands and URLs remain usable on a fresh device while literal header values stay redacted.

Background

Mux user state lives entirely under MUX_ROOT on one host, so a second deployment or a fresh install starts from scratch. The obvious idea, "sign in with GitHub and use the existing login", does not work today:

Surface Scope What it retains
Server login device flow (serverAuthService.ts:12-18) read:user Discards the GitHub token; keeps only a hashed Mux session token
Copilot device flow (copilotOauthService.ts:11-19) read:user Stores the token as github-copilot.apiKey
GH_TOKEN forwarding (credentialForwarding.ts:28-30) n/a Forwarded into Docker/Devcontainer only

Neither token can clone or push, and there is no GitHub App or repo scope anywhere in the repo. Storing an OAuth token or PAT for backups, even a read-only one, is out of the question, so this PR adds no token input, no token storage, and no token pathway of any kind. Mux never holds token material itself: authentication is delegated entirely to credentials that already live on the backend host.

Implementation

Credential ladder (credentials.ts). One controlled attempt, then an ambient retry. The controlled attempt clears inherited helpers (-c credential.helper=) so the mechanism that authenticated is known and reportable in the UI:

  1. SSH URL: ambient agent/keys, with -o BatchMode=yes appended to any ambient GIT_SSH_COMMAND so a passphrase prompt fails instead of hanging, without discarding a host's ssh wrapper.
  2. HTTPS where gh auth status --hostname <host> succeeds: -c credential.helper='!gh auth git-credential'. This is the honest way to reuse an existing gh login, and the token never passes through Mux. GH_TOKEN, GITHUB_TOKEN, and their enterprise variants are stripped from both the probe and the helper invocation, since gh would otherwise consume them ahead of its stored login and quietly turn this rung back into a token pathway.
  3. Auth-class failure falls back to whatever ambient helper exists, still non-interactive.

Every rung sets GIT_TERMINAL_PROMPT=0, GH_PROMPT_DISABLED=1, GCM_INTERACTIVE=never. Repository settings reject credentials in URL userinfo and high-confidence query or fragment parameters before the URL can reach config.json or cache git metadata.

Dedicated secret stores have no export path. This is enforced structurally, not by filtering: payload.ts walks an explicit path allowlist, so providers.jsonc, secrets.json, mcp-oauth.json, server.lock, and serverAuthSessions.json have no code path into the payload. A closed-world test seeds all of them into a fake MUX_ROOT and asserts none appear in the pushed tree. Free-form documentation still passes through the high-confidence scanner. Every exported MCP command requires exact-payload approval, and MCP URLs receive structural checks for raw, malformed special-scheme, and WHATWG-normalized authority userinfo plus credential-like query or fragment parameters, including low-entropy values. Any override is bound to the exact payload bytes the user reviewed.

What syncs: AGENTS.md, agents/*.md, skills/** (execute bits preserved, so skill scripts stay runnable), memory/global/**, mcp.jsonc (commands and URLs verbatim, portable runtime fields, header names, and secret references; literal header values redacted), and preferences.json. Preferences use an explicit versioned allowlist because AppConfigOnDiskSchema is .passthrough(), so a denylist would leak every future key; the same reasoning excludes the record-typed ai.providerOptions providers, whose schema is z.record(z.unknown()). Project-path-keyed and workspace-keyed entries are excluded as machine-local, and AGENTS.local.md on the strength of its own .local marker. appearance.editorConfig is excluded too: its customCommand reaches spawn(..., { shell: true }), so a restore from a repository someone else can write to would be a code-execution vector. Symlinks are never collected, and neither is any hidden file: one rule applied to every path segment keeps .git object stores, .env and its variants, and .netrc out of the recursive collections, where the secret scanner would not recognise a low-entropy value like PASSWORD=hunter2 anyway.

MCP commands and URLs are portable data. Bare stdio command strings and object command and url values are stored verbatim so a fresh device can reconstruct its servers. Every exported command, recognizable secret, and structurally credential-bearing URL component blocks push until the user approves the exact payload digest. Restore separately derives every runnable command it would introduce, change, or enable from the exact bytes it would write and refuses with COMMAND_APPROVAL_REQUIRED until the user approves that text. A command shadowed by a non-empty URL needs no restore approval because McpConfigService.normalizeEntry() ignores it.

Redaction metadata disambiguates literal marker values. New manifests persist exact JSON paths for values the exporter redacted, including an explicit empty list when mcp.jsonc has no redactions. Restore treats only those paths as placeholders, so an approved command whose literal text is __MUX_BACKUP_REDACTED__ remains executable data and still goes through command approval. Metadata is validated against the final serialized MCP document, duplicate manifest and JSONC keys are rejected, and projection-dropped keys cannot leave stale paths behind.

Unrecognized MCP fields remain local. McpConfigService.normalizeEntry() reads transport, disabled, toolAllowlist, url, command, and headers, and ignores the rest, so a field like env: { "API_KEY": "hunter2" } is a credential nobody would notice leaving. Export projects exactly the fields Mux reads, at the types it reads them as; every other value becomes a marker and restore uses only the local value at that exact path. Portable-field lookups require an own property, so document-controlled names such as toString and constructor cannot resolve inherited validators.

Header credentials never come from the repository. Export replaces literal header values with a redaction marker. A portable { secret: NAME } reference may remain in the repository projection, but restore keeps a header only when the local file already has a value at that exact path and the restored entry still points at the same endpoint. A changed endpoint or a fresh device with no local header value gets no credential, so a backup cannot redirect a locally resolved secret.

Repository exports strip MCP comments, while restore preserves local-only definitions. A comment is prose the projection cannot inspect, so export reserializes the projected values instead of publishing the user's text. Restore applies edits in place, keeps destination-only server definitions, and carries their raw JSONC comments and trivia with them. Same-name definitions from the backup remain authoritative.

Older marker-based backups remain safe. A manifest without redaction-path metadata is treated as a legacy backup, so marker-bearing values still rehydrate from local state when available. If no usable local value exists, restore removes the executable marker or unusable server instead of passing marker text to the runtime. Existing repositories remain readable without a schema bump, while their next write adds exact metadata and prevents marker ambiguity.

Only documentation publishes from a recursive collection without review. skills/** and memory/global/** hold whatever the user put there, and no content scanner can decide whether an arbitrary file is a credential, since {"password":"hunter2"} has no distinguishing shape. So the gate is structural: anything that is not .md, .mdx, .markdown, or .txt, plus any file whose path promises credentials or has the exact conventional stem auth or passwd, is held back until the user approves that exact payload. The approval is a hash of every flagged path and its contents, so approving one payload cannot publish a different one another window wrote meanwhile.

Everything lands in one managed subdirectory (default mux/) so an existing dotfiles repo is a safe target, and a persistent cache clone under <MUX_ROOT>/backup-cache/ makes the two-direction preview cheap. Both sides check sizes before reading, since a repository is written by anyone who can push to it and a payload is buffered whole. The clone is sparse, single-branch, and blob-filtered, so a dotfiles repo with substantial history elsewhere is neither materialized nor transferred, and a path this platform cannot create never blocks a backup. Local paths and file: URLs use an upload-pack transport with filtering enabled for both clone and later fetches, preventing Git's local optimization from copying the full object database. Cache creation preserves the remote's SHA-1 or SHA-256 object format when the backup branch is missing, including zero-ref remotes, and SHA-256 clones retain their required extension during config rebuilding. Cache corruption cannot be enumerated, since an empty HEAD, a truncated index, and a ref naming nothing all keep the shape a structural check looks for and only fail a later Git command. So any unrecognized failure discards the disposable cache and retries once rather than leaving Preview, Push, and Restore blocked. Refusals to touch content the cache cannot prove it owns are excluded from that rebuild, as are outages and origin mismatches, so neither a foreign path nor a healthy cache is deleted; the discard itself rechecks ownership at the point of deletion. Relative local repository paths are anchored to the stable parent of <MUX_ROOT>/backup-cache (MUX_ROOT itself), so terminal and desktop launches use the same repository and git -C cannot reinterpret the path from the cache. Before Git runs, Mux rejects symlinks and special files under .git and replaces multiply-linked metadata with cache-owned inodes, so Git cannot rewrite an outside hard-link alias; new local clones also disable Git's hard-link optimization. The cache config is rebuilt from validated platform flags and known forced values on every use, so malformed retained settings cannot brick later retries. The payload is treated as bytes: end-of-line conversion is pinned off in the cache, because the manifest checksums what was written and a restore writes what it reads.

Local payload work is serialized across repositories. The repository lock is keyed per repository and branch, but every payload adapter reads and writes the one Mux root, so a second lock on a single key wraps the local payload work and is always taken inside the repository lock. Without it a push could export a half-restored root and publish it as a complete backup. Git and network work stays outside it, so different repositories still run in parallel. Serialization is process-local; cross-process locking is out of scope.

Validation

Beyond unit tests, a service-level integration test drives a real bare repository end to end and asserts the security invariant on bytes that actually reached a remote. Dogfooded all 12 planned scenarios against a live dev server and a real bare repo, which found 8 defects that are fixed here:

Defects found and fixed during validation
  1. BackupService declared its two dependency interfaces but nothing implemented them, so the service container did not compile.
  2. Every export rewrote manifest.exportedAt, so an unchanged backup produced a commit that only churned metadata.
  3. Restore preview always reported preferences.json as added, because preferences live in config and have no local file to compare against.
  4. createBackupPayload threw on detected secrets before the service could apply the user's override, making the override unreachable.
  5. isPortableReference treated $VAR / ${VAR} / env:VAR header strings as portable and exported them verbatim, but MCPHeaderValue is string | { secret }, so Mux never interpolates those and they are literal credentials.
  6. Restore reserialized mcp.jsonc through JSON.stringify, discarding the document the user had just previewed. Restore now applies jsonc edits in place. (The export deliberately does reserialize, so that no comment ships.)
  7. Preview against a repository with no backup yet leaked a raw ENOENT containing the internal cache path.
  8. The secret-scan override checkbox only rendered inside the preview panel, so a blocked push had no visible way to proceed.

Each fix has regression coverage. The current MCP portability work covers verbatim commands and URLs, literal-header redaction, exact-payload publish approval for every command, exact runnable-command restore approval, literal redaction-marker commands, strict persisted metadata validation, metadata-less legacy backups, prototype-colliding portable-field names, projection-dropped __proto__ keys, disabled URL-to-stdio transitions, local-only server preservation, malformed map rejection, JSONC comment retention, low-entropy and malformed special-scheme URL credential gating, conventional credential filename stems, special-file-safe local reads, and a real bare-repository push. Repository URL tests cover credential-bearing userinfo, query, and fragment forms while retaining benign parameters. make static-check passes, and the configured Storybook story was verified at 1200 px and 390 px with no horizontal overflow.

Risks

Restore mutates user config, but it is deliberately not a mirror: it overwrites paths present in the backup, preserves local-only files and MCP servers, and writes a safety snapshot to <MUX_ROOT>/backup-cache/restore-<id>/ first. Restore planning rejects existing directories and special files before any destination is opened for writing. Optional local MCP reads preflight special files and use a nonblocking checked open so a path replacement race fails instead of hanging restore. Commands and URLs are repository-controlled input, so every exported MCP command and credential-bearing URL component requires exact-payload approval before publication, and changed runnable commands require separate exact-text approval before restore. Literal header values are never exported, and a local header is restored only to its original endpoint. Pushes use --force-with-lease against the commit the preview read, so a remote that moved is reported as drift rather than overwritten, and a server-side denial such as a protected branch is reported as itself.

Blast radius outside the feature is small: the shared-file edits are the settings tab registration, an added settingsBackup config key, and the service container wiring.

Out of scope

Continuous/automatic sync, drift indicators, three-way merge, plans and project-scoped memory, and mirror-mode restore. A "sign in with GitHub" flow is rejected, not deferred: it would require Mux to hold an OAuth token, and storing one, even read-only, is out of the question.


Mux opened this PR on Mike's behalf.


Generated with mux • Model: openai:gpt-5.6-sol • Thinking: xhigh • Cost: $1249.89

@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4b7a54c1d0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/backup/payload.ts Outdated
Comment thread src/node/services/backup/gitRepo.ts Outdated
Comment thread src/node/services/backup/payload.ts Outdated
Comment thread src/browser/features/Settings/Sections/BackupSection.tsx Outdated
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

Thanks, three of the four were real. Fixed with red-green verified regression tests (removed each guard, confirmed the intended test fails, restored).

1. Windows separators in manifest paths (payload.ts:101) — fixed. Correct: skills/..\..\escaped.md passed the allowlist because isAllowedPayloadPath's /^skills\/.+/ matches it and the .. check only split on /. Harmless on Linux, where a backslash is an ordinary filename character, but path.join treats it as a separator on Windows and escapes the destination. assertAllowedPayloadPath now rejects any payload path containing a backslash, since payload paths are always posix. Test: "rejects payload paths that escape the destination on Windows".

2. Managed paths deleting the repository cache (gitRepo.ts:50) — fixed. Correct, and the reachable trigger is worse than traversal: the managed path is the user-facing "Subdirectory" field, so . was accepted and would widen git clean -fd -- . and git commit -- . to the entire cache clone. assertSafeRelativePath now rejects . and empty segments alongside .., and I added the same rule at the schema boundary (isValidBackupPath) so the UI rejects it before any git command runs. Test: "refuses managed paths that are not a real subdirectory", covering ., ./, .., mux/../.., /mux, and mux\..\...

3. Bare key query parameters (payload.ts:263) — fixed. Correct on both halves: api[_-]?key required the api prefix so a Google-style ?key=AIza... was not redacted, and the fallback scanner had no AIza pattern, so it would have been pushed without even triggering the override. Added isSensitiveParamName (matches a bare key as a whole component) and an AIza pattern to SECRET_PATTERNS. Two tests: URL redaction, and free-form file detection.

4. Native titles (BackupSection.tsx:436) — respectfully disagree, no change. These are not native DOM tooltips. Every title in this file is on the shared Button, which deliberately intercepts the prop: Button.tsx:37 does Omit<React.ButtonHTMLAttributes<HTMLButtonElement>, "title">, :40 redeclares title?: React.ReactNode, :64 sets resolvedTooltip = tooltip ?? title, and :95 renders <TooltipIfPresent tooltip={resolvedTooltip}>. No native title attribute reaches the DOM, so there is no OS tooltip to duplicate or z-index. Button.tsx:61-63 documents this as intentional so callers can keep using the prop they reach for. The remaining title= occurrences in the file are props on ChangeList (a section heading) and ConfirmationModal, neither of which is a DOM attribute either. Happy to switch to an explicit tooltip prop if you'd prefer that for clarity, but the AGENTS.md concern about duplicate OS tooltips does not apply here.

Mux made these changes on Mike's behalf.

@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Please take another look.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e9a25a8260

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/common/orpc/schemas/backup.ts Outdated
Comment thread src/node/services/backup/adapters.ts Outdated
Comment thread src/node/services/backup/payload.ts Outdated
Comment thread src/node/services/backup/adapters.ts Outdated
Comment thread src/node/services/backup/gitRepo.ts
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

All five addressed, each with a red-green verified regression test (removed the guard, confirmed the intended test fails, restored).

1. .git components in backup paths — fixed. Confirmed reachable: the managed path is the user-facing "Subdirectory" field, so .git or .git/hooks would have written into the cache clone's own git directory. Rejected in three places now: isValidBackupPath (schema refine), assertSafeRelativePath (the git guard), and BackupService.saveSettings, which owns the invariant rather than trusting the transport. Test: "rejects a managed path that targets the git directory".

2 & 3. Symlink traversal on read and restore — fixed. I confirmed the premise before fixing: git stores symlinks as mode 120000, so a backup repository can carry one, and fs.readFile follows it.

$ git ls-tree HEAD mux/AGENTS.md
120000 blob 48980ad58db1b502c17dd015c92dd262ee8092af   mux/AGENTS.md

Added one shared helper, resolveContainedPath(root, relativePath), which walks the path segment by segment and rejects any component that lstat reports as a symlink. Applied at all three sites rather than only the two reported: the payload write, the payload read, and the restore write. Two tests: reading an entry that was replaced with a symlink to a file outside the payload, and restoring into a MUX_ROOT whose skills directory is a symlink (the test also asserts the outside directory stays empty).

4. Validate payload contents before writing — fixed. readBackupPayload now parses preferences.json and mcp.jsonc after verifying hashes, so a corrupt payload is rejected there. Since restore calls validateRestore before taking the snapshot and before any write, a malformed backup can no longer leave a partially restored install. That also removes the case you noted where the error omitted the snapshot path: no snapshot exists yet when validation fails. Test asserts the target directory is still empty after rejection.

5. Non-fast-forward mapped to drift — fixed. Correct, and the cause was exactly as described: BackupNonFastForwardError had no code, so toOperationError fell through to IO_ERROR. Both error classes now carry one (REPOSITORY_CHANGED and GIT_ERROR for BackupOriginMismatchError, which had the same defect).

One note on how I tested it, since it changes the claim: I first tried to force the drift through the real adapter by moving the remote between pushes, and the push succeeded. That is not the bug you found, it is by design: prepare() fetches and resets before each operation, so a remote that moved is simply the new base and the push fast-forwards from it. The git-level non-fast-forward is therefore a narrow race between prepare and push, which I could not force deterministically. I deleted that test rather than keep a misleading one, and instead assert the mapping you actually flagged by rejecting the real BackupNonFastForwardError from commitAndPush and checking the resulting code. Warning the user when the remote moved since they last read it is a separate feature, tracked for a later phase.

Mux made these changes on Mike's behalf.

@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Please take another look.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e9a25a8260

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/serviceContainer.ts Outdated
Comment thread src/browser/features/Settings/Sections/BackupSection.tsx
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

CI's Test / Unit failure was a real defect in this PR, not flake, and it is fixed.

Root cause. src/browser/hooks/useAnalytics.test.tsx:135 mocks @/version as a bare string:

void mock.module("@/version", () => ({ VERSION: "test-version" }));

CI runs all unit files in one Bun process, so that module mock reaches this feature. VERSION.git_describe was then undefined, JSON.stringify dropped muxVersion from the written manifest, and parseManifest rejected our own output on the next read. With no readable previous manifest there was nothing to reuse, so every export wrote a fresh exportedAt and each backup looked changed. That is why exactly the six no-op and round-trip tests failed while the rest passed.

This mattered beyond the test: any build whose version metadata is unavailable would have written a manifest that this code cannot read, i.e. an unrestorable backup.

Fix. muxVersion is normalized when the manifest is built, so the field is always present, and it is resolved defensively at the boundary in adapters.ts. Manifest validation stays strict on read. Two tests pass muxVersion: undefined directly rather than depending on mock ordering, so they reproduce the condition deterministically.

One correction to my own work. My first attempt also relaxed parseManifest to tolerate a missing muxVersion. Toggling showed normalization alone is sufficient, so the relaxed reader was weakening validation for no benefit and I reverted it. With the strict check restored, removing the normalization fails both new tests, which is the attribution I wanted before claiming the fix.

Local validation: 48 backup tests, 5 UI tests, make static-check exit 0.

Mux made these changes on Mike's behalf.

@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Please take another look.

@ibetitsmike

Copy link
Copy Markdown
Contributor Author

Both correct, and both were consequences of my own earlier fixes. Fixed.

1. Configured token never reached git (serviceContainer.ts:161). This was the more serious of the two: createBackupGitRepo accepted only { cacheRoot } and never set token, so rung 3 of the credential ladder was unreachable in production even though the tab tells the user Mux will try "configured tokens". credentials.ts handled the token correctly; nothing ever supplied one. The container now passes getToken: () => resolveGhToken(), reusing the existing helper at credentialForwarding.ts:28-30 so this matches how the rest of Mux resolves GH_TOKEN. It is resolved per call rather than captured at construction, so a token added after startup works without a restart. Test asserts the getter is consulted on a real operation.

2. Override shortcut still gated on preview (BackupSection.tsx:358). Correct, and it was a gap I introduced: when I made the checkbox appear on a blocked push, I updated the render condition but not the shortcut handler, so the advertised keybind was inert in exactly the situation the checkbox exists for. The handler now mirrors the render condition (preview || secretScanBlocked). Extended the existing "exposes the override after a secret-scan block" test to drive the keybind and assert the checkbox actually toggles, rather than only asserting it renders.

Local validation: 49 backup tests, 5 UI tests, make static-check exit 0.

Note on the Codex Comments job failure on the previous head: that job starts seconds after a push and fails if any thread is unresolved at that moment, so it raced the new review rather than reporting a code problem. Threads for this round are resolved before this push.

Mux made these changes on Mike's behalf.

@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Please take another look.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5221090a2e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/backup/payload.ts
Comment thread src/node/services/backup/payload.ts Outdated
Comment thread src/node/services/backup/payload.ts Outdated
Comment thread src/node/services/backup/adapters.ts Outdated
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Addressed all four findings from the previous round in 8b6b900 and 9700219, each with a red-green verified regression test:

  • Managed-root ancestors are resolved through resolveContainedPath() (per-segment lstat) before any read or recursive removal.
  • Stdio MCP commands are redacted in both supported shapes (object command and the shorthand string entry), covering --flag value, NAME=value, and Bearer <token>, while $VAR is preserved because the command runs through bash -c.
  • The execute bit now travels in the manifest and is reapplied on export and restore, the way chmod +x does it.
  • Restore preview compares against the rehydrated content restore would write, so a redacted value with a surviving local value no longer reports as modified.

Replies are on each thread and the threads are resolved.

Mux is working on this PR on Mike's behalf.

@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

One more preview correction pushed in fb41dd9: restore merges preferences into the local config rather than replacing a file, so preview now compares the merge result. A backup whose preference values all match locally no longer reports preferences.json as modified. Red-green verified against the previous byte comparison.

Mux is working on this PR on Mike's behalf.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fb41dd907c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/backup/adapters.ts Outdated
Comment thread src/node/services/backup/gitRepo.ts Outdated
Comment thread src/node/services/backup/payload.ts Outdated
Comment thread src/node/services/backup/payload.ts Outdated
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

Two more fixes from a self-review pass, pushed in 8babbc6 and 0050d1b. Both are red-green verified.

Restore was dropping machine-local preferences (data loss). restore() handed restoreBackupPayload the exportable projection as the local side, and the merge result then replaced the whole stored preference object. Keys the backup deliberately excludes (navigation.projectOrder, review.defaultBaseByProject) were deleted on restore. Reproduced with a probe, fixed by passing the full local preferences and keeping the projection for export and diffing only.

Redaction and mode gaps. Bare PASSWORD=value assignments and quoted values (--api-key "two word secret") now redact as one token; '$VAR' is no longer treated as a shell reference, because single quotes suppress expansion. Top-level files go through the same collection helper as directories so every entry carries its mode, restore clears the execute bit when the backup says non-executable, and preview and changedFiles compare the bit.

One deliberate non-change, so it is on the record: a redaction marker makes the whole JSONC scalar locally owned, so a non-secret edit the backup made inside that same string is not restored. This is intentional. Splicing a local credential into backup-controlled text would let a tampered backup move that credential to a different host or binary. The policy is stated at the restore rule and pinned by a test.

@codex review

Mux is working on this PR on Mike's behalf.

@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Round addressed in c00444e (plus 8babbc6 and 0050d1b, pushed just before this review landed):

  • Merge into unprojected local preferences (P1): confirmed and fixed. I hit the same defect in a self-review and reproduced it with a probe. Restore now merges into the full local preferences; the projection is only for export and diffing.
  • Force-stage payload files (P1): confirmed and fixed. Staging is now git add -A -f. A seeded .gitignore test asserts mux/preferences.json reaches the pushed tree.
  • Clear executable bits (P2): confirmed and fixed. Restore applies the bit in both directions, and preview plus changedFiles compare it.
  • Rehydrate only the redacted portion (P2): deliberately not changed, argued on the thread. A marker makes the whole scalar locally owned because partial substitution would let backup-controlled text choose where the local credential is sent (vendor.example to attacker.example). Preview and restore share one helper so they agree, the affected fields are surfaced in redactions, and the policy is pinned by a test.

Every fix is red-green verified. Backup suite is 57 tests, make static-check is green.

Mux is working on this PR on Mike's behalf.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c00444e125

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/backup/gitRepo.ts Outdated
Comment thread src/node/services/backup/payload.ts Outdated
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Both findings confirmed and fixed in 6e60656, each red-green verified:

  • Remove ignored payloads when refreshing the cache (P1): cleanManagedPath now uses git clean -fdx. A payload an earlier preview or blocked push left behind as an ignored file can no longer survive the reset and be read back as remote state. Test seeds .gitignore with mux/, exports without pushing, re-prepares, and asserts preview sees no remote backup.
  • Preflight every restore destination (P2): restoreBackupPayload resolves all destinations and restored content before the first write. The symlink test now seeds a local AGENTS.md (sorts before skills/) and asserts it is untouched after the symlinked destination is rejected.

Backup suite is 58 tests, make static-check green.

Mux is working on this PR on Mike's behalf.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6e60656f6b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/serviceContainer.ts Outdated
Comment thread src/node/services/backup/payload.ts Outdated
Comment thread src/node/services/backup/adapters.ts
Comment thread src/node/services/backup/payload.ts Outdated
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

All four findings confirmed and fixed in e667e5c, each red-green verified:

  • Limit GH_TOKEN to GitHub hosts (P1): the factory now passes the token only when the backup URL is a GitHub host (github.com, a *.github.com subdomain, or GH_HOST), rejecting lookalikes like github.com.evil.example. Separately, the token helper is now scoped to the repository's own origin instead of the global credential.helper, so a redirect or submodule on another host cannot be offered the token either.
  • Redact credentials inside stdio command URLs (P1): command redaction now runs the existing URL rules over any http(s):// argument first, so npx mcp-remote https://host/mcp?api_key=... is covered.
  • Preserve MCP secrets in local safety snapshots (P1): the snapshot is written with keepLocalSecrets: true. It never leaves the machine, so keeping mcp.jsonc verbatim is what makes it able to recover a server the restore removed outright.
  • Reject incompatible restore targets (P2): resolveContainedPath now rejects a non-directory intermediate component, and the restore preflight rejects a destination that is an existing directory. Both happen before the first write.

Backup suite is 60 tests, make static-check green.

Separately, the earlier Test / Integration failure was tests/ui/review/focus.test.ts ("Review panel not focused"), which is unrelated to this branch: my only keybinds are ctrl+alt+<letter>, none collide with Cmd/Ctrl+2, and that test passes locally. I reran the job.

Mux is working on this PR on Mike's behalf.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e667e5c173

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/backup/payload.ts Outdated
Comment thread src/node/services/backup/payload.ts Outdated
Comment thread src/node/services/backup/payload.ts Outdated
Comment thread src/node/services/backup/payload.ts Outdated
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

All four confirmed and fixed in 8382ddb, each red-green verified:

  • Reject symlinked top-level backup files (P1): collection is now uniformly no-symlink. collectDirectory already skipped symlinked entries, but top-level files used stat and a symlinked collection root was followed by readdir. Both fixed, which matches the restore side that already refuses to write through symlinks.
  • Exclude nested Git metadata (P1): .git entries are skipped during collection and .git components are rejected in assertAllowedPayloadPath, so a manifest cannot smuggle one in either.
  • Project provider options through a closed allowlist (P1): this was my own rule broken in one spot. Only providers with a closed z.object schema (anthropic, openai, xai) are copied; the record-typed google/ollama/openrouter are excluded, and a provider added later is excluded by default. The guard iterates MuxProviderOptionsSchema.shape and injects apiKey under every provider, so adding a record-typed provider to the allowlist fails the test.
  • Redact non-Bearer credentials in stdio headers (P2 in effect, filed P1): replaced the Bearer-only rule with a header-argument rule keyed on the header name, covering Authorization: Basic ... and X-API-Key: ... while leaving Accept: application/json intact.

Backup suite is 62 tests, make static-check green. PR body updated to describe the provider-option, symlink, and .git exclusions.

Mux is working on this PR on Mike's behalf.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8382ddbc66

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/backup/payload.ts Outdated
Comment thread src/node/services/backup/payload.ts Outdated
Comment thread src/node/services/backup/payload.ts Outdated
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

All three confirmed and fixed in b74a6e3, each red-green verified:

  • Merge provider options without replacing excluded providers (P1): mergeAiPreferences merges providerOptions provider by provider. The other ai fields keep replace semantics on purpose, because they are fully backed up; only data the backup could not carry is preserved.
  • Reject .git case-insensitively (P1): done, and I extended the same fix to FORBIDDEN_BASENAMES since it is the same defect class (skills/Providers.JSONC would have passed).
  • Stop header redaction consuming later arguments (P2): a regression I introduced last round. Header matching is now quote-aware, with the unquoted value stopping at whitespace, and an empty value left alone. The test asserts -H X-API-Key:hunter2 --transport stdio keeps its trailing flags.

Backup suite is 64 tests, make static-check green.

Mux is working on this PR on Mike's behalf.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b74a6e3888

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/backup/payload.ts Outdated
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Fixed in 2b94f45. This was a straight violation of the self-healing rule you cited: rehydration parsed the local mcp.jsonc unconditionally, so the file most in need of repair blocked the repair. restoreMcpFile now returns early when the backup has no redaction marker, and a local parse failure is treated as no recoverable values. Preview shares the helper, so it agrees.

The guarantee is pinned by "restores over a malformed local MCP config", red-green verified by removing the try/catch. I also wrote a test for the early return and then deleted it: with the try/catch in place that branch is behaviorally unobservable, so the test passed with the code removed. I would rather have no test than one that cannot fail.

Backup suite is 65 tests, make static-check green.

Mux is working on this PR on Mike's behalf.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2b94f4549e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/backup/payload.ts Outdated
Comment thread src/node/services/backup/payload.ts Outdated
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

Both confirmed and fixed in 64baf00, each red-green verified:

  • Reject duplicate MCP keys before redaction (P1): I took the reject option rather than tree-based redaction, since a duplicate key is already ambiguous config and refusing to export beats promising a redaction I cannot prove complete. assertNoDuplicateKeys walks the parseTree and is wired into parseJsoncObject, so export, read-back, and restore validation share it. The rehydration read of the local file keeps its try/catch, so a local duplicate still cannot block the restore that replaces it.
  • Reject case-colliding manifest paths (P2): case-folded in both readBackupPayload's duplicate check and the restore preflight. The test asserts the restore root is still empty, so nothing is written before the rejection.

Backup suite is 67 tests, make static-check green.

Mux is working on this PR on Mike's behalf.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4db1e5423e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/backup/credentials.ts
The scp-like predicate matched the drive-letter colon in a remote such as
C:\backups\mux.git, so the ladder reported the ssh credential and the
Backup UI claimed an SSH key or agent was used for what git treats as a
local path.

Excluded on Windows only, mirroring git's has_dos_drive_prefix. Verified
that git elsewhere really does read C:/repo as scp-like and dial host `C`
(GIT_SSH_COMMAND runs), so an unconditional exclusion would drop the ssh
rung from a remote that needs it.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f2c3d22b5c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/backup/backupService.ts Outdated
Every restore left its safety snapshot behind forever; the only cleanup
path handled a snapshot whose write failed. Each one is an unredacted copy
of the whole local payload, so repeated restores grew the cache without
limit.

The three newest are kept, so the recovery point a restore depends on and
its predecessors survive. Snapshot directories now carry a timestamp in
their name because mtime cannot order restores that land in the same
millisecond, and only snapshots older than the one just written are
candidates. The reap runs outside the write's own failure handling, so
reclaiming disk cannot delete the recovery point.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7ce70fb7d8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/backup/backupService.ts Outdated
The retention pass reaped by age, but withRepoLock is per repository, so
restores of other repositories run concurrently. Once enough newer
snapshots existed, an in-progress restore's snapshot could be deleted and
that restore would then hand back a snapshotPath that no longer existed.

A snapshot is now reapable only once its own restore has returned, marked
by a sibling `.released` file written in a finally block. An unmarked
snapshot is either in progress or was killed partway; neither is
distinguishable from outside, so both are left alone. Leaking a snapshot
the user can delete is recoverable, deleting a live restore's only
recovery point is not.

Ordering no longer decides safety, only which released recovery points to
keep, so same-millisecond stamps can no longer change what is retained.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5b4bb89f1d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/backup/backupService.ts Outdated
Comment thread src/node/services/backup/adapters.ts
writeSafetySnapshot writes with portable: false so a snapshot can hold
names only this filesystem must accept, such as skills/a:b.txt or both
Foo.md and foo.md. readBackupPayload applied the cross-platform rules
unconditionally, so it rejected the snapshot as INVALID_BACKUP: the copy
kept for recovery could be written but never read.

The reader takes the same portable option the writer does. A repository
payload still gets the portable rules, because another platform has to
write it out. Also corrected the comment that claimed readBackupPayload
could already read a snapshot.

Retention keeps a returned snapshotPath readable for RETAINED_SNAPSHOTS-1
later restores, now covered by a test.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 50242cfa47

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/backup/credentials.ts Outdated
A GIT_SSH_COMMAND or core.sshCommand starting with a double-quoted
executable, which is how Windows spells a path with spaces, was tokenized
at the first space. Two consequences: the variant went unrecognized so no
BatchMode was added and prompts stayed live, and when GIT_SSH_VARIANT=ssh
forced the flag on, the option was inserted inside the quoted path and git
failed outright. The corruption was a regression from moving the flag
ahead of the configured options.

Both the variant check and the insertion now read the same program token,
which understands single quotes, double quotes, and a bare word.
The released marker is written in a finally block, which runs before the
restore's promise settles, so a concurrent restore of another repository
could still count and delete a snapshot whose caller had not received the
path yet. No marker placement fixes that, because no point inside the
service is after the caller resumes.

The service knows what no marker can: which snapshots belong to restores
that have not returned. Reaping now happens only when none are in flight,
which closes the window for this process. The marker still covers another
process, whose in-flight restores this set cannot see.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2129c0463f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/backup/credentials.ts Outdated
The double-quote decoder from the previous commit treated every backslash
as an escape, so "C:\Program Files\OpenSSH\ssh.exe" decoded to
C:Program FilesOpenSSHssh.exe. Variant detection then missed OpenSSH and
added no BatchMode, leaving prompts live until the backup timeout: the same
symptom the quoting fix was meant to remove.

Only the two characters double-quoted syntax escapes are decoded. Variant
detection also splits on both separators instead of path.basename, because
a core.sshCommand written on Windows is read verbatim wherever the config
is used and basename only knows the host's separator.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ee11aa8ac7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/backup/backupService.ts
Comment thread src/common/config/schemas/settingsBackup.ts Outdated
Git decodes userinfo before invoking ssh, so `ssh://user%3Apw@host` and
`ssh://user:pw@host` reach ssh as identical bytes (verified against git
2.54 with a fake ssh that prints its argv). The raw-text check only looked
for a literal colon and `new URL` leaves `%3A` in `username` with an empty
`password`, so the encoded spelling of a rejected URL was persisted.
The repository lock is keyed per repository and branch, so two operations
on different tuples both reached the one Mux root every payload adapter
reads and writes: a push could export a half-restored root and publish it
as a complete backup, and two restores could interleave their write loops.

A second lock on a single key now wraps only the local payload work, taken
inside the repository lock everywhere so the order is fixed. Git and
network work stays parallel across repositories. This also subsumes the
narrower unreturnedSnapshots guard, since no other in-process restore can
be inside the critical section when a snapshot is reaped.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: 8cef579681

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

One malformed escape made decodeURIComponent throw for the whole string,
so the previous check judged the raw text and accepted a credential git
still decodes: `ssh://user%zz%3Apw@host` reaches ssh as `user%zz:pw@host`
(verified against git 2.54 with a fake ssh that prints its argv).

Scanning for %3A is both correct and simpler. Git decodes in one pass, so
`user%253Apw` stays a username and remains accepted.
The name stamp has millisecond resolution, so restores landing in one
millisecond tied and ordering fell back to mkdtemp's random suffix. Reaping
could then delete a newer recovery point and keep an older one, and the
retention test failed 8 times in 40 runs. A monotonic sequence in the name
breaks the tie: 0 failures in 40 runs after this change.

Retention safety still comes from the .released marker, not from ordering;
the order only chooses which returned snapshots to keep.
Double quotes also escape `$`, a backtick, and a newline, so the parser
comment claimed more than the code does; it decodes quotes and backslashes
only. The discard guard proves the shape of what sits at the cache path,
not that it is the same clone ensureCache validated.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

Heads up: this head moved after your approval of 8cef579681, and one of the three changes fixes a real hole in the code you just approved. Flagging it explicitly rather than letting it slide through on a stale OK.

1. The encoded-credential fix I shipped last round was incomplete (3c43acbaa)

My fix decoded the userinfo and looked for a colon. But decodeURIComponent throws on the whole string if any escape is malformed, and my catch then fell back to judging the raw text. Git does not work that way: it decodes each valid triplet independently. Verified against git 2.54 with a fake GIT_SSH_COMMAND that prints its argv:

ssh://user%zz%3Apw@example.com/repo.git  ->  user%zz:pw@example.com git-upload-pack

So the credential still reached ssh. Scanning for %3A is both simpler and correct, and it drops the decode plus catch entirely:

function userInfoHasPassword(userInfo: string): boolean {
  return userInfo.includes(":") || ENCODED_USERINFO_DELIMITER.test(userInfo);
}

Git decodes in one pass, so user%253Apw stays the username user%3Apw and is still accepted. Both spellings are now pinned in tests, and reverting to the decode version fails them.

2. A 20 percent CI flake in snapshot retention (675ac5a8b)

The name stamp is millisecond resolution, so restores landing in the same millisecond tied and ordering fell back to mkdtemp's random suffix. Reaping could delete a newer recovery point and keep an older one. Measured on the retention test: 8 failures in 40 runs before, 0 in 40 after adding a monotonic sequence to the name. Retention safety still comes from the .released marker; ordering only chooses which returned snapshots to keep.

3. Two comments corrected (34e52f2ed)

The ssh parser comment claimed double quotes escape only quote and backslash. They also escape $, a backtick, and a newline, so the comment claimed more than the code does. And the cache discard guard comment claimed the same ownership ensureCache demands; it actually proves the shape of what is at the path, not that it is the same clone.

One thing I deliberately did not change. assertDiscardableCache() does not prove identity, so a cache directory swapped mid-materialization for a structurally valid git repo would be discarded. I left it: reaching that state needs write access to a 0700 directory owned by the user, so the actor is already the user, and the data destroyed is what they placed there. Capturing dev/ino and adding a quarantine rename is a redesign of the delete path with a worse risk-to-reward ratio than the bug. I corrected the comment so it no longer overclaims instead.

Posted by Mux on Mike's behalf.

@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 34e52f2ede

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

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