mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-10 05:01:28 -10:00
fix/opencode-session-header
6
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
ff62f73b7f |
fix(secrets,worktrees): fix secret redaction leakage and prune stale worktrees (#855)
* fix(secrets,worktrees): fix secret redaction leakage and prune stale worktrees 1. Prevent trailing redaction leaks in github_token, aws_access_key_id, and google_api_key by adding trailing word boundaries and allowing variable lengths. Refine the openai_key pattern to cleanly distinguish legacy keys and modern prefixed keys (sk-proj-, sk-svcacct-) from ordinary kebab-case phrases. 2. Implement auto-pruning of zero-owned git worktrees older than 24 hours at the start of worktrees.Prepare to prevent indefinite disk space leaks. * fix(secrets,worktrees): close tail-leak edge case and worktree data-loss risk Drop the trailing \b anchor on the four secret patterns whose body class allows "-" (slack_token, google_api_key, the modern openai_key branch, jwt). \b requires a word/non-word transition, so a secret ending in "-" right before a delimiter has none, and the engine backtracked the greedy quantifier to drop that last character instead of failing the match, leaking it. The body character class already provides the real stopping boundary, so the anchor was unnecessary. Fix two issues in worktree Clean flagged in review: - Staleness was decided by the worktree directory's own mtime, which only changes when an entry is added/removed/renamed directly inside it, not when a long-running task edits existing files deeper in the tree. Clean now walks the tree and treats any recently modified entry as live, and also skips any worktree a caller has explicitly locked via git worktree lock. - baseDir ownership used a raw strings.HasPrefix, so a sibling like "<baseDir>-other" would false-match. Replaced with a filepath.Rel path-boundary check. * fix(worktrees): check exit codes on removal, fail closed on inspection errors defaultRunGit deliberately returns a nil error alongside a nonzero CommandResult.ExitCode for a failed git invocation, so the worktree remove call must check ExitCode itself instead of trusting a nil error to mean success. Route it through gitOutput, which already does that. worktreeIsStale treated an inspection failure (an unreadable file, a WalkDir error) the same as "keep walking," which can let an incompletely-inspected worktree be judged stale. Any inspection error now makes it ineligible for removal instead. * fix(secrets,worktrees): catch appended-suffix keys and scope pruning to owned worktrees The scanner's trailing \b anchors made a credential vanish entirely when followed by a word character outside its body class (an appended suffix like AKIA...EXTRA, or ghp_..._suffix): the fixed or unbounded-greedy quantifier had no valid word boundary to land on and the whole match failed, so the real secret reached the redaction output unredacted. Dropping the trailing anchors lets the body class itself stop the match, so the credential prefix still gets redacted even when noise follows it. Also recognize sk-admin- alongside sk-proj-/sk-svcacct- so OpenAI admin keys aren't skipped by the narrowed modern-key branch. Clean pruned any worktree under the caller-supplied BaseDir, but Prepare only ever creates worktrees under a per-repository zero-worktree-<repoKey> subtree of it. Scope pruning to that subtree so a worktree a user manages by hand elsewhere under a shared BaseDir is never force-removed. Also refuse to force-remove a worktree whose mtime looks stale but that still has uncommitted or untracked changes: a task can hold live work while waiting on a model, network, or user for longer than the staleness window without writing to the tree again. * fix(worktrees): make nested-activity test exercise the deep walk it claims to activePath/internal was created by the same MkdirAll as the nested pkg dir but never backdated, so it kept a fresh mtime and worktreeIsStale's walk reported "not stale" as soon as it hit that directory, before ever reaching the freshly-written file two levels deeper. The test passed without actually exercising recursion past the first directory. * fix(worktrees): lock zero-created worktrees and treat ignored files as dirty Prepare never called git worktree lock, so the entry.locked skip in Clean only ever protected worktrees a human locked by hand, never zero's own; a worktree that finished committing and sat idle (e.g. waiting on a slow model or network retry) for more than 24h looked clean-and-stale and got force-removed by the mtime+dirty heuristic alone. Lock every worktree Prepare creates so it gets the same protection. worktreeIsDirty also used git status --porcelain with no --ignored, so a worktree holding only .gitignore-matched task data (credentials, generated drafts, artifacts) reported as clean and got force-removed with --force, silently discarding it. Add --ignored so those files count as dirty too. * fix(worktrees): release Zero's Prepare lock so Clean can reclaim finished worktrees Prepare locks every worktree it creates so Clean's mtime+dirty staleness heuristic never force-removes one Zero is still using, but nothing ever unlocked it, making the automatic disk-space cleanup permanently inert. Add Release (git worktree unlock) and wire it in two ways: zero exec --worktree defers a release once its own run finishes, since that flow's use of the worktree is bound to its own process. zero worktrees prepare hands the path to a longer-lived external caller with no defined end-of-life, so a new zero worktrees release <path> subcommand lets that caller release it explicitly when done. * fix(worktrees): normalize release path, aggregate Clean errors, unlock deleted worktrees Address CodeRabbit's review on the lock-release fix: - zero worktrees release now resolves its path argument to absolute before calling Release, since git worktree unlock matches against the path git recorded at creation, not whatever directory the caller happens to be running from. - Clean now aggregates removal failures with errors.Join instead of overwriting lastErr, so multiple stale worktrees failing removal in the same pass are all reported, not just the last one. - Release falls back to options.Cwd as the git working directory when the worktree path itself no longer exists (e.g. a caller deleted a locked worktree by hand instead of releasing it first), so the orphaned lock can still be cleared. Added regression coverage for all three. * style(worktrees): align aggregation test comments to gofmt output * fix(worktrees,cli): restore reuse lease and scope unlock to owned locks - Prepare re-locks a reused worktree so Clean's staleness heuristic cannot force-remove it while the new caller is still using it; a lock already held by a live external caller is kept in place and reported through the new Result.LockAcquired field. - exec --worktree only releases the lock its own Prepare call acquired and surfaces a failed release on stderr with the affected path instead of discarding the error. - worktrees release wires the resolved workspace root into Options.Cwd so the deleted-path recovery works outside the worktree directory. - The relative-path release test derives its expected value via filepath.Abs, matching the resolution the CLI uses, so macOS /var vs /private/var spellings no longer break it. * fix(worktrees,cli): reject in-use leases, validate before cleanup, add release -C - Prepare rejects a worktree whose lock another run still holds, on both the reuse and the create-race paths, instead of handing a second live caller an unprotected shared checkout whose sole Git lock the first caller's exit would release. - The automatic stale-worktree pruning runs only after the request itself validates, so a rejected command (an invalid --name) has no destructive cleanup side effect; covered end to end with real git in both directions. - worktrees release accepts -C/--cwd naming the source repository, which the deleted-path recovery needs when launched outside the repo (the deleted worktree path is a one-way hash with no way back to its source). * test(worktrees): canonicalize test roots to physical spelling git records worktree paths in physical form, so the CI runners' symlinked (/var -> /private/var) and 8.3-short (RUNNER~1) temp spellings made Clean's containment check skip the test's stale entry and the pruning assertion fail on macOS and Windows. * fix(worktrees): preserve orphaned commits, verify release ownership, canonicalize base dir Four fixes from the latest review round: - Clean now creates a durable ref (refs/zero/orphaned-worktree/<sha>) for a detached worktree's HEAD before force-removing it, when that commit isn't already reachable from any other ref. Prepare always creates worktrees with `worktree add --detach`, so a commit made there had no ref pointing at it once the worktree was deleted, making it immediately eligible for git gc despite never having been merged/pushed elsewhere. - Clean resolves its configured base directory through EvalSymlinks before comparing it against git's reported worktree paths: `git worktree list --porcelain` reports each worktree's PHYSICAL location (resolving symlink components), so a symlinked --worktree-dir made every worktree created under it permanently unprunable. - Release now verifies path has a zero-worktree-<repoKey> ancestor directory component before running `git worktree unlock`, so it can't be used to clear the lock on a worktree a user or another tool manages by hand. The check doesn't need to know which --dir a given Prepare call used (nothing records that against a specific worktree, and the CLI never threads BaseDir through to Release) — the repoKey component is Prepare's actual ownership signature regardless of which directory it was created under. - Route the release command's printed path and exec's release-failure diagnostic through the existing CLI redaction helper, and split the worktrees help text into prepare-specific and release-specific flag sections (release only ever supported -C/--cwd, not the --name/--dir/ --json the shared block advertised). All four have regression tests confirmed to fail without their fix (two using real git worktrees, not just fakeRunner sequences). Build, vet, and gofmt clean on linux/windows/darwin. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(worktrees): recoverable PID leases and reclaimable released worktrees Two review findings on the cleanup lifecycle: - A lock left by an abnormal exit (SIGKILL, crash, power loss) was skipped by Clean forever, recreating the permanent disk leak this PR set out to fix. exec --worktree now records its PID in the lease reason; Clean expires a lease whose recorded owner is provably dead, unlocking only after the staleness, dirty, and HEAD-preservation guards all pass. Human locks and PID-less leases (external `worktrees prepare` owners) remain permanent until explicit release, and any ambiguity in the liveness probe counts as alive. - An explicitly released worktree holding only gitignored residue (node_modules, build output) was skipped at every age. Release is the owner's completion signal, so unlocked entries now block removal only on tracked/untracked changes; expired crashed leases keep the conservative --ignored probe since they never signaled completion. Co-Authored-By: Claude Fable 5 <[email protected]> * fix(worktrees): address review feedback on lease detection and release safety Split dead-lease PID checking into posix/windows implementations so Windows can reliably tell a dead process from a live one. Fix a path canonicalization mismatch in two release tests. Derive release ownership from git worktree list instead of the git-dir parent, which was wrong for repos with a separate git-dir. Refuse to clear a lock that was not taken by Zero in the first place. * fix(worktrees): canonicalize paths for clean/release ownership checks Compare Clean containment and Release ownership against physical path spellings so macOS /var vs /private/var and symlink TMPDIR layouts match git worktree list. Require a registered porcelain entry and a Zero lease reason before unlock; treat an already-unlocked Zero worktree as a no-op. * fix(worktrees,secrets): address jatmn review findings on #632 - Prepare and Release now agree on repoKey regardless of which worktree (main or linked) Prepare runs from, by keying off git worktree list's first entry (always the main worktree) instead of --show-toplevel. A worktree prepared from a linked checkout previously failed its own ownership check on release and its lease could never be cleared. - osProcessAlive on Windows no longer treats every OpenProcess failure as "process is dead": only ERROR_ACCESS_DENIED (a live process this caller lacks rights to query) is now distinguished from a genuinely missing PID, so Clean can no longer force-remove an active worktree whose owning process it simply couldn't query. - The openai_key redaction pattern now recognizes sk-or-v1- (OpenRouter) alongside the existing sk-proj-/sk-svcacct-/sk-admin- prefixes, so hyphenated OpenAI-compatible provider keys are redacted again without reopening the sk-<kebab-case-phrase> false-positive this pattern was narrowed to avoid. - Release/exec --worktree error text is redacted before reaching stderr, matching the already-redacted success path; ownership errors interpolate the caller-supplied path, which could carry a key-shaped segment. - canonicalizePath resolves symlinks through the nearest existing ancestor when the target itself no longer exists, so the documented `release -C` recovery path works again for a worktree deleted by hand under a symlinked --worktree-dir. - Prepare rolls back the worktree `git worktree add` just created if the subsequent lock call fails for a reason other than a concurrent racer, instead of leaking an unleased checkout until Clean's 24h staleness window reclaims it. Not addressed here: the P2 finding that worktree ownership is provable only by a directory-name convention plus a lock-reason prefix, both of which a user can reproduce by hand. A durable per-worktree ownership marker would close that gap, but internal/worktrees has been on main since #70, so a marker requirement could reject worktrees an already-installed zero created before this change existed. Needs a decision on migration before implementing. * fix(worktrees): prove Prepare ownership with a git-admin marker Address review findings that path convention plus lease-reason prefix are forgeable by hand. Prepare now writes a zero-owner marker into the worktree admin dir; Release and Clean require it before force-touching a path. Clean also keys its owned subtree off the main worktree root so linked-checkout calls still prune the same bucket Prepare uses. Tests plant the marker and list the main worktree first so fixtures match production. * fix(cli): complete worktrees release in shell completions * test(worktrees,cli): add coverage for linked-worktree Clean, forged lease rejection, and completions - TestCleanFromLinkedWorktreePrunesStaleWorktree: pins Clean deriving its owned-subtree key from the main worktree root (not the invoking linked checkout's --show-toplevel) so Prepare/Clean run from a linked worktree actually reclaim the worktrees Prepare created there. - TestReleaseRejectsForgedZeroLeaseWithoutOwnershipMarker: pins the ownership-marker requirement against the exact forgery jatmn described - a worktree under the predictable zero-worktree-<repoKey> path, manually locked with a reason that merely starts with the zero lease prefix. - Fix TestPrepareCreatesDetachedGitWorktree: its fake git runner ran out of canned results at Prepare's post-lock ownership-marker write, so writeOwnershipMarker resolved gitDir to "" and os.WriteFile wrote "zero-owner" as a relative path into the test process's real working directory instead of failing loudly. Give it autoAbsoluteGitDir like the other Prepare-exercising tests use and assert on the marker-write call. - completions_test.go: assert `worktrees`/`worktree` completions include `release` alongside `prepare`. * fix(redaction,worktrees): sort extra secret values by length descending and canonicalize worktree paths * fix(secrets,worktrees): restore Anthropic key redaction and handle legacy worktree cleanup * fix(secrets): add bash output redaction regression test for Anthropic API keys * fix(worktrees): touch worktree mtime on reuse to prevent stale pruning When Prepare reuses an existing worktree, it now touches the directory to refresh its mtime. This prevents Clean from force-removing a long-running but idle worktree (e.g. waiting on a model) that has no recent file changes but is still actively in use. Addresses the data-loss risk flagged in the PR review where mtime-only staleness + --force removal could discard live worktrees. Co-authored-by: cairn-code Co-Authored-By: cairn-code <[email protected]> * fix(worktrees): properly handle os.Chtimes error on reused worktree path Co-Authored-By: cairn-code <[email protected]> * fix(worktrees,secrets): unlock using porcelain entry path, set RepoRoot to primaryRoot, and add anthropic_key pattern Unlock using matched porcelain entry path, set Result.RepoRoot to primaryRoot in Prepare, add dedicated anthropic_key secret pattern, and canonicalize fake-runner test paths. Refs #632 * fix(tools): correct redaction placeholder assertion in Anthropic key test TestFormatBashOutputRedactsAnthropicKey checked for the openai_key placeholder instead of anthropic_key, a copy-paste leftover. The redaction itself was already correct; only the assertion was wrong. * test(worktrees): physicalize test paths for macOS tempdir symlink resolution * fix(secrets,worktrees): address CodeRabbit review findings Align RedactString token boundaries with secrets.Scan, strengthen the Anthropic bash redaction assertion, fix Clean fixtures so they reach the guards under test, and fail closed on ambiguous processAlive probes. * fix(secrets,worktrees): restore broad key redaction and legacy Clean safety Address human review on #855: keep sk- bodies with a digit filter instead of enumerated vendor prefixes, add a looser JWT form, restore the sk-test fixture, probe legacy ownership before dirty, treat non-INVALID Windows OpenProcess errors as alive, redact Abs/cwd release errors, and reclaim dead-owner leases on Prepare reuse. * fix(secrets,redaction): always redact known OpenAI key prefixes Alphabet-only sk-proj-/sk-svcacct-/sk-admin- tokens are still credentials; keep the digit filter only for unknown sk- vendor forms so kebab phrases like sk-learn-… stay un-redacted. * Preserve digit-free legacy keys during redaction. Refs #855 * fix(secrets,worktrees): address CodeRabbit findings on PR #855 Redact full compact JWE tokens, pin git locale for lock parsing, write the ownership marker atomically, surface missing-dir unlock failures, and fail closed when HEAD probes are indeterminate. Align Clean fixtures and digit-free known-prefix redaction tests with the shipped behavior. Refs #855 --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: cairn-code <[email protected]> Co-authored-by: euxaristia <[email protected]> |
||
|
beardthelionandKevin Codex
|
a880ce80a6 |
feat: gate project-scoped hooks, plugins, and MCP servers behind workspace trust (#529)
* feat(workspacetrust): add exact-match trust store for project config gating * feat(hooks): add ExcludeProject option to LoadConfig * feat(plugins): add ExcludeProject option to Load * feat(cli): gate project hooks and plugins behind workspace trust Trust check lives inside newHookDispatcherWithExtra and activatePlugins (fail-closed by construction); trustRoot is the original launch dir so a --worktree run inherits the source repo's trust. Callers OR the skip reports and emit one combined stderr notice. * feat(cli): add zero trust command (trust/list/remove) * fix(review): key spec-draft trust on the original launch dir; cover trust CLI edge branches Spec-draft runs (zero exec --use-spec --worktree) passed the reassigned worktree path as trustRoot, dropping a trusted repo's project hooks (fail-closed, but breaks worktree-inheritance). Thread the captured pre-worktree trustRoot into execSpecDraftRun. Add tests for the trust 'remove' too-many-args and --help branches, and a resolveTrust comment noting the per-chokepoint check must not be hoisted to callers. * test(cli): end-to-end trust-gate coverage through agent.Run and exec --worktree Drives the real agent loop with a fake tool-calling provider so a project beforeTool hook actually fires (or is gated away) through production code: gate blocks a tool call via agent.Run, and --worktree / --use-spec --worktree key trust on the original launch dir. All three are load-bearing (RED when the gate or the trustRoot threading regresses). * style(cli): gofmt struct alignment in exec_spec.go * fix(workspacetrust): compare stored trust entries literally, no symlink re-resolution External adversarial review (codex) found a false positive: IsTrusted re-normalized stored entries on every check (Abs+EvalSymlinks), so a trusted path later replaced by a symlink to attacker content re-resolved to the new target and matched. Stored entries are already canonical (normalized once at Trust time); compare them as literals and only normalize the incoming query. Add a regression test for the symlink-retarget case. * feat(mcp): gate project-scoped MCP servers behind workspace trust Closes the P0 an external adversarial review (codex) found: project ./.zero/config.json stdio MCP servers spawned before the hook/plugin gate, an ungated arbitrary-execution path that could also self-trust the workspace. Add ExcludeProject to config.ResolveMCP (skips the project layer) and thread a fail-closed trust check into every MCP spawn site (exec, TUI setup+refresh, mcp tools list, mcp check); reporting sites stay ungated. Verified the sole server-command exec (mcp/client.go) is reachable only through the now-gated RegisterTools paths. * test(cli): cover default-mode beforeTool firing through the trust gate Assert a project beforeTool hook fires in the default auto and ask permission modes (not only under unsafe), and is blocked when the workspace is untrusted, so the gate is verified in the mode a normal run actually uses. * docs(cli): note MCP servers in zero trust help text and doc comment The gate covers project hooks, plugins, and MCP servers, but the help string and runTrust doc comment listed only hooks and plugins. * test(cli): make workspace-trust tests portable on macOS and Windows setTrustConfigRoot set HOME on darwin, but config.UserConfigDir is XDG-first there (=$HOME/.config), so tests wrote trust.json and user hooks.json where the code never read them; set XDG_CONFIG_HOME on every non-Windows platform instead. writeMarkerHook shells out to /bin/sh, absent on Windows, so its trusted-case marker never appeared; skip it there like writeMarkerHookScript already does. TestRunTrustAddThenList compared `trust list` output (canonicalized via EvalSymlinks) against the raw cwd, which diverges on the Windows runner's 8.3 short names (RUNNER~1 -> runneradmin); compare the resolved path, and add TestRunTrustListShowsCanonicalPath to exercise that via a symlink on any platform. * feat(cli): surface the project MCP skip in the workspace-trust notice Project MCP servers were dropped silently in an untrusted workspace: the trust notice covered hooks and plugins but not MCP, and app.go discarded the resolveTrust error. registerMCPToolsForWorkspace now returns a trustSkip, gated on a new projectMCPConfigExists check plus the previously-dropped store-read error, and emitTrustNotice is variadic so the exec, spec-draft, interactive, and `mcp tools list` paths all fold in the MCP skip. The skip drives the notice only; the gate stays on resolveTrust/excludeProject, so a trusted workspace is unaffected and an untrusted one still fails closed. * fix(cli): print zero trust --help to stdout with a success exit zero trust -h/--help/help routed usage to stderr with the usage exit code, unlike every other subcommand (mcp, sandbox, skills, cron, ...) which prints help to stdout and returns success. Align it; the unknown-subcommand path still writes to stderr with the usage exit code. * fix(cli): gate oauth MCP login and surface mcp-check trust notice Two project-scoped MCP activation sites were left outside the workspace trust gate. zero mcp oauth login loaded a project-defined OAuth server from an untrusted workspace and ran the full discovery/registration/token-exchange flow against its configured URL. Gate resolveOAuthServer with resolveTrust so an untrusted clone's ./.zero/config.json OAuth server is dropped before login, and emit the same one-line trust notice the other spawn sites use. zero mcp check already gated the project layer but discarded the trust-check result and never emitted the notice, so a gated project server read as a bare "not configured". Emit the notice on the not-found branch. The three remaining ungated resolveMCPConfig(cwd, false) sites (backends doctor/snapshot and the extensions listing) are enumeration-only and stay ungated. * test(cli): adapt MCP startup-skip tests to two-arg resolveMCPConfig --------- Co-authored-by: Kevin Codex <[email protected]> |
||
|
Vasanth T
|
43c256068a |
feat: make Go runtime the app path (#71)
* feat: make go runtime the app path * fix: harden update semver parsing |
||
|
gnanam1990
|
4e19bd992f | fix: address MCP review feedback | ||
|
gnanam1990
|
e8e77c8a8f | [codex] expose MCP tools in Go CLI | ||
|
KRATOSandKRATOS
|
f43db6f600 |
[codex] add Go extension backend commands (#61)
* feat(go): add plugin manifest backend * feat(go): add extension policy stores * feat(go): expose extension backend commands * fix(go): reject rooted plugin paths cross-platform * fix(go): address extension backend review * fix(go): harden extension backend persistence * fix(go): preserve extension usage and hook enabled semantics * fix(go): use process-released MCP permission locks * fix(plugins): close symlink missing-leaf escapes * fix(plugins): fail closed on manifest realpath errors * fix(tests): keep plugin realpath regression portable * fix: enable CodeRabbit verdict workflow --------- Co-authored-by: KRATOS <[email protected]> |