M3: drift / status / diff / reconcile - #8
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Files/keys hashed and stored to ~/.agentsync/.state/targets.json. Drift detection in Task 4+ reads from this. State save is atomic (iox.AtomicWrite via state.Save). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eys ops Plan() now accepts *state.Targets; for each merge-json-keys/merge-jsonc-keys op it populates FileOp.OwnedKeys from the matching state.Keys entries so the adapter apply layer knows which JSON-pointer paths agentsync owns. Callers updated (apply.go loads state before planning). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ed items Walks the render plan + state to classify every file-op and key-op via drift.Classify. Prints [agent] sections with one %-20s class + path per row. Uses render.CollectPointers (actual export name) not PublicCollectPointers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…destination Uses github.com/sergi/go-diff/diffmatchpatch for character-level diff output. Supports optional path filter; prints 'no diff' when source == destination for all tracked items. Key-level diff for merge-json-keys ops. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…auto-* flags Implements reconcile command with --auto-writeback, --auto-override, --auto-safe flags and an interactive read-char prompt loop ([w]rite-back, [o]verride, [s]kip, [i]gnore, [d]iff, [q]uit; bulk W/O/S). Uses cmd.InOrStdin() for testable stdin injection. --auto-override re-applies the full plan and updates state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…s; wire reconcile [w] action - internal/source/writer.go: WriteMCP, WritePlugin, WriteMarketplace — atomic TOML write-back into ~/.agentsync/ (comments not preserved, v1 trade-off). - internal/source/writer_test.go: round-trip and overwrite tests for all three. - internal/cli/reconcile.go: replace "not yet implemented" stub with writeBackItem() — key-level MCP items reconstruct source.MCPServer from dest JSON and call source.WriteMCP; file-level items copy dest back to SourceID path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- TestApplyThenReconcileAutoSafe: apply + reconcile --auto-safe on clean state exits cleanly with "nothing to reconcile". - TestDriftLoop_FullRoundTrip: full loop — apply, mutate dest, status reports drift, reconcile --auto-override restores source value to destination. - TestDriftLoop_WriteBack: reconcile --auto-writeback updates source MCP file from drifted destination value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
spxrogers
force-pushed
the
claude/agentsync-m3
branch
from
May 7, 2026 02:55
ea3b6fe to
a936a54
Compare
spxrogers
added a commit
that referenced
this pull request
Jul 20, 2026
…ble release & doc/contract fidelity (#195) * docs(changelog): fix version-compare links and add 0.7.1/0.7.2 headings - [Unreleased] now compares from the latest released tag (v0.10.1) instead of v0.1.0, so the compare range no longer re-includes released 0.7.x-0.10.x. - add the missing [0.7.3] link-reference target. - add [0.7.1] and [0.7.2] section headings (referenced in the 0.7.3 prose) as truthful pointers to the consolidated 0.7.3 entry, with matching targets. - every bracketed version heading now has a link-reference definition and vice versa; no dangling references remain. Closes #133 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AeRcYquBpDB97v9nW7v8yz * fix(core): panic on adapter Register collision; add allAgentNames subset guard registryFactory discarded every Register error with `_ =`, so a name collision (e.g. a new generic.Spec whose Name equals a deep adapter's, or a duplicate spec) was silently swallowed: the second registration became a no-op, Lookup resolved to the first adapter, and the colliding agent still passed validateAgent — surfacing only when apply/status/diff misbehaved for that agent. - registryFactory now routes every Register through a mustRegister helper that panics (wrapped as a registry wiring bug) on the collision error. Signature is unchanged (func() *adapter.Registry), so all call sites compile untouched. - new white-box tests assert allAgentNames() is a subset of the registered set, and that the production wiring builds without collision, has the expected adapter count, and no duplicates. No secret/capture/render code touched; adapter.Registry.Register is unchanged (it already returns the collision error — the bug was the caller discarding it). Closes #160 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AeRcYquBpDB97v9nW7v8yz * test(adapter): assert KeyMergeStrategy() matches every emitted key-merge op Each adapter names its single key-merge strategy in two hand-maintained places — the KeyMergeStrategy() accessor and the MergeStrategy string stamped on each key-merge FileOp in Render — with no central cross-check. orphanCleanupOps trusts the accessor alone to synthesize destructive cleanup writes, so a drift between the two could decode a JSONC/TOML file as strict JSON and clobber it. - new TestKeyMergeStrategy_MatchesEmittedOps (internal/cli) renders a real MCP+hook fixture through every registered adapter (both scopes, all 22 generic specs) and asserts: every emitted key-merge op carries the accessor's strategy; an empty accessor emits zero key-merge ops; at most one distinct strategy per adapter (single-strategy invariant). A vacuous-run guard fails if the fixture exercises no key-merge surface at all. - document the single-strategy-per-adapter constraint on the KeyMergeStrategy interface doc and in docs/architecture.md (multi-format co-ownership unsupported), naming the guard. - fix the stale architecture.md line that listed Gemini under merge-json-keys; it emits merge-jsonc-keys. Closes #157 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AeRcYquBpDB97v9nW7v8yz * chore(release): pin goreleaser, reproducible archives, sign checksums, guard nfpms URLs Four release-hygiene fixes bundled: - Pin goreleaser to 2.16.0 across ci.yml (both goreleaser-action steps), release.yml, and `just ci` (via `go run …@2.16.0`, self-enforcing like golangci-lint), plus a lint-job guard that fails if the three ever diverge — closing the C3 version-skew where CI could validate a config against a newer `latest` than the pinned publisher ships. - Restore archive mtime reproducibility: goreleaser 2.16.0 has no top-level archives.mtime, so pin the binary via builds_info.mtime and the bundled LICENSE/README/CHANGELOG via files[].info.mtime, all to {{ .CommitDate }}. A re-cut release now produces byte-identical archives + checksums.txt, making the release: "reproducible, identical" comment true again (restores PR #115's drop). - Sign checksums.txt with keyless cosign (signs: block); add id-token: write and a cosign-installer step to release.yml. No new signing secret (ambient OIDC). - Add a lint-job guard coupling the nfpms version-less file_name_template to the README releases/latest/download/ URLs, and repoint the .goreleaser.yaml VERIFY comment at it. Validated with `goreleaser check` (2.16.0): 1 configuration file validated, no deprecations. Chocolatey stays commented out (#188); not reintroduced. Closes #138 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AeRcYquBpDB97v9nW7v8yz * docs(matrix): Claude Hook -> \xE2\x97\x90 and correct Codex subagent name claim Two capability-matrix corrections landing against the merged Wave-2 behavior they document: - #147: downgrade the Claude Hook cell from \xE2\x9C\x93 to \xE2\x97\x90. Post-#124, agentsync models only command hooks (matcher + command), which round-trip losslessly; a non-command handler type or an unmodeled field (e.g. timeout) is reported (a render Skip / an ingest warning that leaves the native entry untouched), which is exactly what makes \xE2\x97\x90 ("projected — translated with documented, reported loss") honest. Adds a Claude Hook bullet to "What each \xE2\x97\x90 loses" citing the artifact-anchored TestIngest_HookArtifactRoundTrip, and flips the condensed user-guide Hook row. (Line-23 prose was already corrected by #73.) - #150: correct the Codex subagent name claim. Post-#144 the name round-trips bidirectionally (ingest re-populates the frontmatter name from the TOML name, so a diverging name survives) and colliding effective names are refused at render — the matrix now says so instead of the one-way "carries over". Docs-only; the website contract page regenerates from docs/capability-matrix.md. Closes #147 Closes #150 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AeRcYquBpDB97v9nW7v8yz * docs(release+git): macOS Gatekeeper guidance, arm64 URLs, qualified revert/chmod notes Pure-docs (no Go, goreleaser, or test changes): - README + website install.mdx: document that raw-archive macOS binaries are unsigned/un-notarized so Gatekeeper blocks first run, with the `xattr -dr com.apple.quarantine` workaround (Homebrew users unaffected); show the arm64 .deb/.rpm download URLs alongside amd64 (filenames match the nfpms file_name_template). - docs/concepts.md: note the local git-backup .git 0700 hardening is POSIX-only (a Windows no-op → filesystem ACLs are the boundary). - docs/user-guide.md: qualify revert's "nothing is lost" to tracked files only (untracked scratch files are outside the snapshot and left untouched). The README git-backup Known-limits bullet this issue also lists is owned by #141 (dedup) and lands in that commit. Closes #132 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AeRcYquBpDB97v9nW7v8yz * fix(core): wire Detect() into doctor; remove the dead Capabilities() bitmask The Adapter interface carried two methods with zero production consumers. Detect() -> wired in (Option A): doctor's adapter-detection section now constructs each registered adapter and calls Detect() (config-dir stat + PATH fallback, richer than the old PATH-only loop). Detection stays informational and never fails doctor. Adds TestDoctor_ReportsAdapterDetection (config-dir-driven detected/not-detected). Capabilities() -> removed (Option B): nothing in the pipeline consumed the per-agent Capability bitmask, so it could silently drift from real Render/Skip behavior — the classic dual-list smell. Deleted Capabilities() from the interface, all 11 implementations, and both interface test stubs, plus the Capability type and Cap* consts and the dead per-adapter Capabilities tests. Component support is (and was) expressed by Render returning []Skip. Docs: docs/architecture.md drops Capabilities() from the interface block, annotates Detect() (consumed by doctor), and corrects the false 'the pipeline reports those components as skipped [via the bitmask]' claim to state skips come from Render returning []Skip. docs/components.md drops the Capability bitmask references. Per-method verdict is explicitly allowed by the issue. Verified: go build/vet clean, adapter/cli/render tests green, zero residual adapter.Cap* references. Closes #177 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AeRcYquBpDB97v9nW7v8yz * fix(secrets): harden secret-invariant edges (backstop, EnvBackend, fences) Seven defense-in-depth edges around the secret-handling invariants (issue #163): - capture.Capture is now FAIL-CLOSED UNDER INDETERMINACY: when the backend can't resolve a ${secret:…} the source references (vault locked/unavailable), the backstop value prong is blind, so it refuses the write-back instead of degrading to a warning. (env: refs are unaffected — they don't feed the value prong.) - EnvBackend uses presence semantics (os.LookupEnv): a set-but-empty var resolves to "" like AgeBackend; only an unset var errors. - the DestWriter write-ban is now fenced for os.Remove/os.RemoveAll/os.WriteFile/ os.Create via forbidigo rules (text-scoped exclusions for the DestWriter itself + non-destination callers + tests) AND a source-scanning test — previously only iox.AtomicWrite was fenced. - drift classifier deleted-destination rows pinned (drift-dest-deleted -> Drift, conflict-dest-deleted -> Conflict). - CheckIdentityPermissions stat-error bypass documented as deliberate + directly tested (a present group/other-readable identity is still rejected). - the value-prong substring over-refusal bias is documented at the call site. - the capture residual warning is reworded to honestly describe what it checks. Docs: architecture.md (backstop fail-closed-under-indeterminacy + broadened os.* fence), SECURITY.md (capture refusal note). No secret-bearing field added; walkSecretFields untouched. just test + golangci-lint (0 issues) both green. Closes #163 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AeRcYquBpDB97v9nW7v8yz * test(codex): MCP/hooks coexistence + skill/body fidelity; ingest enabled - Wire Codex's per-server MCP `enabled` boolean into ingest: IngestMCPSpec reads it into MCPServerSpec.Enabled (present-only capture via asBoolPtr; absent stays nil=default-on) and excludes it from Extra. Verified against Codex upstream docs (config.toml [mcp_servers.<name>] enabled, default true). Enabled is not secret-bearing — walkSecretFields untouched. - Add artifact-anchored fidelity tests: MCP+hook coexistence in one config.toml (asserting both [mcp_servers. and [hooks. survive the merge on disk); a spec-complete on-disk skill dir round-trip asserting bundled files survive byte-for-byte and scripts keep 0o755; and a multi-paragraph developer_instructions body round-trip. All in internal/adapter/codex; codex package tests + vet + gofmt green. Closes #152 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AeRcYquBpDB97v9nW7v8yz * test(git): cover doctor per-version-root git-backup classification checkDestinationGitBackup's per-version-root loop (os.Stat -> agit.Detect -> print agentsync-versioned/foreign/untracked) had zero behavioral coverage — the only prior assertion checked the static mode header on a fresh init where no destination dir exists, so the loop body never ran. - add TestDoctor_ReportsPerVersionRootGitState: plants three real on-disk dirs at three distinct deep-agent version roots (claude = agentsync-owned via agit.Init, roo = foreign via go-git PlainInit, cline = untracked) and asserts doctor's output reports each state, driven end-to-end through runCLI. Preconditions pin the planted agit.Detect states so a miss is unambiguously a reporting bug. - mark the all-registered-agents probe (reg.Names(), not the enabled set) intentional in a comment, mirroring checkPlugins. No production behavior change. Closes #174 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AeRcYquBpDB97v9nW7v8yz * fix(cli): refuse an empty secrets set value; add --allow-empty secrets set accepted an empty value from every input mode (--stdin with empty/ whitespace-only input, the interactive prompt with a bare Enter, and the legacy key= form) and silently stored an empty-string secret, reporting success. An empty secret is almost never intentional, is invisible to the fail-closed cleartext backstop, and resolves to "" in native config at apply time. - secretsSet now refuses a whitespace-only/empty value by default with a value-free error (names only the key, never the attempted value), unless --allow-empty is passed. The guard sits before decrypt/encrypt so a refused set leaves the vault untouched. - tests: TestSecretsSet_RefusesEmptyValue (table over stdin-empty/newline/ whitespace + legacy-empty; asserts nothing stored, value not echoed) and TestSecretsSet_AllowEmptyStoresEmpty (escape hatch round-trips through encrypt/decrypt). - docs: user-guide + cli.mdx document --allow-empty; while there, corrected the 'secrets set|get|edit <key>' reference row so edit is not shown as taking a <key> (also satisfies #145 item 7 for that row). Closes #165 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AeRcYquBpDB97v9nW7v8yz * fix(cli): accept id@marketplace ref in plugin upgrade/enable/disable/remove install accepts an id@marketplace ref and splits it; the four sibling subcommands did not — they passed args[0] straight to the plugins/<id>.toml path, so copying the exact install ref (demo@test-mp) targeted plugins/demo@test-mp.toml, which never exists. upgrade/enable/disable surfaced a raw file-not-found; remove named the ref instead of the bare id. - all four now run splitPluginRef(args[0]) before validateCacheKey and operate on the bare id (upgrade keeps deriving the marketplace from the stored id, which stays authoritative). - upgrade/enable/disable now map a not-found readPluginTOML to the same friendly 'plugin "<id>" is not installed' wording remove already used. - the @-permitted validateCacheKey/validateMCPID nit is subsumed structurally: splitting before validating means the bare id never contains @ (validators unchanged; TestValidateCacheKey documents this). - tests: TestPlugin_SubcommandsAcceptMarketplaceRef (full-ref lifecycle) + TestPlugin_NotInstalledReportsFriendlyError (no raw file-not-found leaks). - docs: cli.mdx / plugins.mdx / user-guide reflect the id[@marketplace] shape. Closes #168 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AeRcYquBpDB97v9nW7v8yz * fix(adapters): tighten VersionedDirs guards (boundary, root-tightness, owners keying) Three correctness gaps in the git-backup VersionedDirs machinery: - TestVersionedDirsContract used strings.HasPrefix (a byte-prefix) instead of a path-boundary check, so a buggy adapter returning a sibling like <root>-evil/x would pass the guard whose whole job is to catch escaping roots. Switched to the boundary-correct isUnderDir and added TestVersionedDirsContract_RejectsSibling. - generic versionRootOf's multi-segment base allowlist ({.config,.aws,.agents,.pi, .gemini}) must stay in sync with specs.go but had no code note and no over-broad-root assertion. Added a sync-obligation comment and a root-tightness assertion to TestVersionRoots_AllSpecs (a target 3+ segments below its root signals a collapsed base; current specs max at 2) plus TestVersionRootOf_Tightness pinning the hazard. - versionRootOwners keyed the owner map by the globally de-nested root set while revertAgent looked up owners by each agent's OWN de-nested roots — so a parent-folded root (OpenCode's ~/.claude/skills under Claude's ~/.claude) recovered nil owners, dropping the shared-dir blast-radius warning. Added an ownersFor(owners, root) helper (exact key OR nearest ancestor via isUnderDir) and a TestOwnersFor_RecoversFoldedRoot regression. No behavior change to the 22 current specs' roots/warnings. Closes #154 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AeRcYquBpDB97v9nW7v8yz * fix(cli): guard CLI write paths — gitbackup re-parse, reconcile EOF, verify offline, marketplace/purge Five disjoint write/validate-path defects (issue #171): 1. setDestinationGitBackupMode now re-parses the spliced agentsync.toml and refuses the write (file left byte-for-byte untouched) if it no longer parses or would alter content outside its table — mirroring writeAgents' fail-closed backstop. The line-based splicer could otherwise corrupt an unusual-but-valid layout. 2. reconcile's two interactive EOF sites now "goto done" instead of "return nil", so a queued [o]verride and pruned/dirty state are flushed on EOF (adversarially verified: the test fails without the fix). 3. offline verify (AGENTSYNC_ALLOW_OFFLINE_VERIFY=1) now validates the reference SHAPE of every secret/env reference via secrets.MalformedSecretRefs — a malformed empty-key ref fails — instead of skipping all reference checks while claiming "all references resolve". The misleading comment + CI/env/cli docs are corrected to say offline checks shape, not resolvability. 4. the user-scope agent-disable --purge cross-scope blast radius is RATIFIED and documented (machine-wide cleanup; project-scope purge stays isolated per #187), pinned by a new test. 5. marketplace head_sha/name are documented (on both the CLI marketplaceTOMLSpec and canonical source.MarketplaceSpec) as fetch-cache metadata deliberately not modeled canonically — no silent drop; a fidelity test pins the documented round-trip. just test (in-container) + golangci-lint green. Closes #171 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AeRcYquBpDB97v9nW7v8yz * docs(arch): soften never-push claim to a convention-guard framing architecture.md step 9 claimed internal/git 'exposes no remote/push surface at all', implying a type-level impossibility. The guarantee is actually enforced by TestNoPushSurface, a source-scanning grep over the package's .go files for banned tokens — the go-git *Repository that Repo holds still has Push/CreateRemote, so a struct-shape or reflection change could defeat the scan. Reworded to attribute the guarantee to the convention guard, mirroring CLAUDE.md's precision for the secrets lint fence. No code change (TestNoPushSurface unchanged). Closes #161 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AeRcYquBpDB97v9nW7v8yz * docs(drift): clear the carried-over doc-vs-code drift cluster (C2) Correct a cluster of contract-surface docs (and Gemini source comments) that had drifted from the code (issue #145): - six "plugin import is Claude-only in v1" claims -> "Claude and Codex" (Codex implements adapter.PluginIngester): docs/user-guide.md, cli.mdx (x2), doctor.go, import.go (x2), existing-configs.mdx. - components.md Windsurf section: memory AND commands render at both scopes (only MCP is user-scope-only), and the adapter DOES implement WarnEmitter (Ingest warns on a workspace rule missing the trigger: always_on frontmatter). - Gemini merge-json-keys -> merge-jsonc-keys in source comments (gemini.go, mcp.go, paths.go, apply_test.go) + the gemini line in components.md, matching KeyMergeStrategy(); other adapters' correct merge-json-keys left intact. - concepts.md architecture anchor fixed to #8-secrets--how-the-leak-is-prevented. - copy nits: comparison.md agent count 30+ -> 31, "three axes" -> "four axes". - capability-matrix: acknowledge Windsurf's separate 12,000-char workspace-rule / workflow limit (verified against upstream docs.devin.ai), alongside the existing 6,000-char global-rules note. The OpenCode tools->permission matrix line was already corrected to drop+Skip in Wave 2; the components.md adapter tree is owned by #141 (dedup). No functional code changed. Closes #145 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AeRcYquBpDB97v9nW7v8yz * docs(git-backup): sync v1.0 destination-git-backup contract docs (#141) Bring the contract docs and repo memory up to date for the v1.0 destination-git-backup feature and the packages it added. - components.md: package map gains internal/git (leaf go-git wrapper) and internal/ui; the adapter sub-tree lists the full set (9 deep + generic + noop) and each deep adapter's homedir.go; internal/cli commands gain revert + version (and git/ui deps); new ### internal/git and ### internal/ui subsections; internal/project corrected to the .agentsync/ directory overlay; secrets leakscan.go/runtime.go and marketplace treehash.go added to Files. - architecture.md: §10 package-layering graph gains a git node (no push surface) with a CLI->GIT edge and ui in the INFRA leaf set; new ### VersionedDirs (optional) §3 subsection documenting the read-only interface and its four-rule contract. - cli.mdx + user-guide.md: doctor now documents its Destination-git-backup section (mode + per-dir repo status). - README.md: new "Known limits" bullet — the backup history is local-only and never pushed (cleartext), .git is 0700 on POSIX only, foreign-source-control dirs are left un-versioned, revert covers tracked files only. - website/README.md: comparison/index.md <- docs/comparison.md mirrored-pages row. - .agentsync/memory/AGENTS.md re-rendered (revert + version in the command-tree sentence); CLAUDE.md/AGENTS.md regenerated via apply --scope project (no drift). * fix(secrets): sanitize offline verify output; harden review-round-1 findings Round-1 review-loop findings on the Wave 3 PR (#195): - ISSUE (adversarial): offline `verify` printed config-derived malformed ${secret:…}/${env:…} candidates unsanitized, reopening the #93/PR100 terminal-escape-injection class — looseRefRe's [^}]* tail captures raw ESC bytes, so a shared config could inject escapes into the CI log. MalformedSecretRefs now returns []untrusted.Text and verify renders via untrusted.Join, so display is sanitized by construction. New break-verified test (offline-malformed-ref-is-sanitized). - NIT (test-rigor): broadened the direct-os destination-write guard to cover os.OpenFile (the O_TRUNC truncate-write), os.Rename, and os.Truncate in both writer_lint_test.go and .golangci.yml — closing the rename/truncate bypass of the DestWriter foreign-collision backup. Allowlisted the two marketplace cache writers (fetch_relative.go, fetch_npm.go). - NIT (api-design): reworded the guard's "kept in lockstep" comment to state the true relationship — the per-file test allowlist is a subset of (at least as tight as) the whole-dir .golangci.yml exclusions. - NIT (adversarial): tightened setDestinationGitBackupMode / spliceTOMLTable docstrings — the line-splice's comment fidelity is best-effort for lines adjacent to the table; the fail-closed re-parse backstop (semantic), not the splice, is what guarantees no data outside the table is lost. Correctness lens returned CLEAN. just test green; golangci-lint 0 issues. * fix(cli): close remaining escape-injection sites; anchor offline ref shape check Round-2 review-loop findings on the Wave 3 PR (#195): - ISSUE (adversarial): `verify`'s [agents.<name>] validation error wrapped the config-derived map key with %s. A TOML quoted key can carry raw ESC bytes, so a shared config could inject terminal escapes on plain `agentsync verify` (no offline flag) — the same #93/#171 class R1 hardened, at a sibling site. Switched to %q (matching agent.go/status.go/revert.go). New break-verified test. - NIT (correctness): the offline malformed-ref shape check used an UNANCHORED re.MatchString(cand), so a malformed outer ref embedding a well-formed nested ref ("${secret:${env:FOO}}") was silently accepted (the strict shape matched the inner "${env:FOO}" substring). Now matches the whole candidate via FindStringIndex span == [0,len). New break-verified test. - Defense-in-depth (from the adversarial cousin note): `doctor`'s [secrets] identity_file/age_file path lines printed the config-derived path — and the os.Stat error embedding it — raw via %s/%v, and the "not readable"/"not yet created" branches fire even for a non-existent path (reachable via shared config alone). Both the path (via untrusted.Text) and the stat-error rendering are now sanitized. New test (surfaced the error-message leak too). api-design and test-rigor lenses returned CLEAN. just test green; golangci-lint 0 issues. * fix(verify): sanitize verifySecrets path errors; scope the escape-injection note Round-3 review-loop finding on the Wave 3 PR (#195): - ISSUE (correctness + test-rigor, convergent): verify's verifySecrets printed the config-derived [secrets].identity_file / .file path — and the *PathError re-embedding it — raw via %s/%w on the DEFAULT `agentsync verify` path. It is the direct twin of doctor's checkSecrets, which R2 (3682673) hardened in the same file while missing this mirror function. Now sanitizes both the path and the error rendering via untrusted.Text (dropping %w), matching the doctor fix. New break-verified test (TestVerify_SanitizesHostileSecretPath). Also narrowed the CHANGELOG escape-injection note: the R1-R3 fixes hardened the verify/doctor config-path print sites, NOT the whole class. The review's systematic sweep confirmed the same #93/#171 pattern is PRE-EXISTING at other display sites (marketplace/agent/mcp list columns, the status/diff/apply/reconcile path dashboards, source.Load error rendering); those are out of scope for this 18-issue wave and are called out for a dedicated follow-up rather than claimed closed here. just test green; golangci-lint 0 issues. * fix(cli): sweep the whole terminal-escape-injection class across the CLI Per request, close the entire #93/#171 class the review surfaced, not just the verify/doctor sites hardened in earlier rounds. Every command that prints a config-/native-config-derived string now sanitizes it at the display boundary (ui.Sanitize / untrusted.Text), so a shareable dotfiles repo can't smuggle ESC/bidi bytes into the terminal or a CI log: - marketplace list: url + head_sha columns (name was already sanitized). - agent list: the [agents.<name>] key and the display-only scope value. - mcp list: the server-id (filename-stem) column. - status: renderStatusItem path#pointer and renderSkillGroup root; the orphan warning's second %s -> %q. - diff: the --- source / +++ dest hunk labels. - apply: the plan-preview op.Path (synced + write lines). - reconcile: itemLabel via a display-only itemLabelDisp (the raw itemLabel is still written to ignore.toml, which must stay exact), the orphan-prompt paths, and the mcp write-back serverID errors. - doctor: the schema-invalid line (go-toml's strict error echoes the raw config source line). - internal/secrets/age.go: the age-backend identity/age-file path errors (single-point fix covering verify online resolution + apply). New escape_sweep_test.go drives marketplace/agent/mcp list, doctor schema, and apply --dry-run with hostile ESC/U+202E fixtures and asserts no raw dangerous rune reaches stdout while the sanitized marker survives; representative sites break-verified (revert the sanitize -> test fails on the raw byte). CHANGELOG's scope note updated from "deferred" to "class closed". just test green; golangci-lint 0 issues. * fix(cli): close the indirect escape-injection leaks (errors, derived paths) A verification pass over the whole-class sweep found the same #93/#171 class still leaking where a config-derived path surfaces INDIRECTLY — through an error value or a derived path rather than a direct print. Closed all of them: - render.CollisionReport.String() sanitizes its Path/Pointer at the single point it is formatted, covering both call sites (reconcile override backup notice, update --apply backup notice). - reconcile: the orphan-block backup-failed / backup-path / remove-failed lines, the write-back error, and the filepath.Rel conflict path (the itemLabelDisp first arg was already sanitized; its sibling rel was not). - import: importIO.item() sanitizes its path for every caller — closing the dry-run marketplace preview, whose native marketplace id is a plain string not gated by ValidateComponentID — plus the undeclared-native-items warning (path#pointer harvested from the native config). - revert --dry-run's change-list paths. Tests: render/collision_report_test.go (whole-file + pointer variants) and a new TestEscapeSweep_ImportDryRun (native marketplace id with a raw ESC via json.Marshal) — both break-verified. Correctness of the earlier logic-preserving edits (itemLabel vs itemLabelDisp, serverID lookup vs display, age.go %w drop) independently re-confirmed: no regressions. CHANGELOG updated. just test green; golangci-lint 0 issues. * fix(capture): preserve captured mcp/lsp enabled; scope release id-token to signer Round-4 full-PR review findings: - ISSUE (adversarial): #152 modeled Codex's native `enabled` into MCPServerSpec.Enabled, but capture.Capture UNCONDITIONALLY reset a re-imported server's Enabled to the source value (the "enabled is source-only" preservation), so a native `enabled = false` on an ALREADY-MANAGED server was silently dropped on reconcile/re-import — printed as a "write-back" success while the next apply re-enabled the server (violating CLAUDE.md's "never drop it silently"; a regression from the prior Extra-passthrough behavior). Capture now falls back to the source Enabled only when the ingest carried none (Enabled == nil), so a captured explicit &true/&false survives; Agents stays unconditionally source-preserved (no dest carries it). Applied symmetrically to LSP. New break-verified TestCapture_PreservesIngestedEnabled (both directions). CHANGELOG #152 entry updated to describe the write-back preservation. - NIT (api-design): release.yml granted `id-token: write` workflow-wide, but only the goreleaser job signs (keyless cosign). Moved it to that job's own permissions block (least privilege); top level keeps only contents:write for the docs job's gh-pages push. YAML validated. (The other api-design NIT — changelog headings stop at 0.7.3 while real tags run to v0.10.1 — is a pre-existing documentation gap needing release history not in this PR's scope; left as-is rather than fabricating 6 release entries.) just test green; golangci-lint 0 issues. Correctness + test-rigor lenses CLEAN. * docs: sync the enabled-is-conditionally-preserved contract after the #152 fix Round-5 review: correctness / adversarial / test-rigor all CLEAN. api-design found the R4 capture fix (eb1090a) left the CONTRACT DOCS describing MCP `enabled` as a source-only field the destination never carries and unconditionally preserves — now false (Codex's dest carries native `enabled`; capture preserves it only when the ingest carried none). Per CLAUDE.md's "keep the docs in sync" non-negotiable, corrected every stale site: - internal/capture/capture.go package doc (step 2): split agents (source-only, always restored) from enabled (dest-carried by some agents; source value restored only when the ingest carried none). - docs/architecture.md §5: the mermaid "preserve source-only fields (agents, enabled)" node + the prose beneath it (a generated contract page). - internal/source/schema.go LSPServerSpec + internal/render/report.go countLSPServers: reworded the "mirror MCPServerSpec source-only enabled" cross-references (LSP agents source-only; enabled follows the same conditional rule; no LSP adapter ingests enabled today). - internal/marketplace/loadprojected.go sameMCPRender/sameLSPRender: dropped the now-imprecise "render strips it and capture preserves it" for enabled; the real reason it's excluded from the hijack comparison is that it's targeting metadata, not the server's endpoint. - .github/workflows/release.yml id-token comment: there is no separate choco job (choco runs as steps inside goreleaser, which DOES sign) — fixed the job list. No code-behavior change. just test green; golangci-lint 0 issues. * fix(ci): skip cosign signing in the goreleaser snapshot (CI + just ci) The #138 keyless-cosign `signs:` block runs during `goreleaser release --snapshot` too, but the CI `goreleaser-snapshot` job (and local `just ci`) don't install cosign — only the release workflow does (sigstore/cosign-installer). So the snapshot failed at "signing artifacts" with `exec: "cosign": executable file not found in $PATH`. `goreleaser check` validates the signs: schema but never executes it, which is why it passed while the snapshot build did not. Add `sign` to the snapshot's `--skip` list in ci.yml (now `--skip=publish,chocolatey,sign`) and justfile `just ci` (`--skip=publish,sign`), alongside the existing chocolatey skip (same reason: a release-only CLI the CI runner doesn't install). release.yml is unchanged — it installs cosign and does NOT skip sign, so real releases are still signed; the lint job's `goreleaser check` still schema-validates the signs: block. Verified locally: `goreleaser release --snapshot --skip=publish,chocolatey,sign --clean` now reports "skipping … sign …" and "release succeeded". Only the goreleaser-snapshot check was red; all other PR checks (test-fast x3, lint, test-release) were already green. --------- Co-authored-by: Claude <noreply@anthropic.com>
spxrogers
pushed a commit
that referenced
this pull request
Sep 1, 2026
Closes round-3 review findings on PR #240. 1. The anti-vacuity check was still inferring. It matched substrings against cobra's error prose, which covers only the arg/flag layer — anything rejecting later scored as "it ran", including this repo's own enforceScopeStance, a PersistentPreRunE refusal that never reaches RunE. runBounded now WRAPS the resolved command's RunE, so "did the body start" is observed rather than deduced, and cannot drift with cobra's wording. Third version of this check; the first two were unable to fire at all. It is also falsifiable now. runBoundedE reports instead of failing, so TestRunBoundedDetectsACommandThatNeverRan can assert ran==false for a missing argument, an unknown command AND a PersistentPreRunE refusal — the case the substring list structurally could not catch — plus ran==true for a command that does run. Previously nothing failed if the check were reverted. 2. Subtest fixture bleed. The cleanup unlinked the FIFO but never restored the applied file, so the key-merge subtest ran against a home whose whole-file destination was missing, contradicting the test's own "a real applied home". Measured: with the skips deleted, `import claude` and `reconcile --auto-override` PASSED in a full run and HUNG in isolation — whoever closes #241/#242 would have inherited a row green for the wrong reason. The destination is now restored, and after the fix the row hangs both ways. 3. writeBackFileItem appended "remove or replace the non-regular file at that path" to EVERY read failure, including ENOENT. Deleting a managed file is itself drift and offers [w], so the common path produced "no such file or directory — remove or replace the non-regular file at that path": advice for a situation the user is not in. Gated on errors.Is(err, errDestNotRegular), with a test for the absent case. Third round running that this one message has been the site of a new defect. 4. Prose, instances #8 and #9. The CHANGELOG headline said a "directory" no longer hangs — a directory never hung (os.ReadFile fails it in ~18us with EISDIR); only the diagnosis changes. And destread.go justified leaving the symlink split by claiming AGENTSYNC_ALLOW_SYMLINK_DEST=1 would break, but that variable is read only in internal/iox, on the WRITE path, so a read gate cannot affect it. The real reason is that changing it changes what diff and reconcile have always reported (#229) — and the real consequence, now named, is that under that supported setup `status` reports drift no apply can clear. 5. The hash row computed its expectation with the function under test; salting hashContent left it green. Pinned to a literal digest, break-verified. Also: docs/components.md's "Enforced, not asserted" overstated a two-spelling text matcher whose own LIMITS exempt a read; softened, and the review-audit parenthetical that had survived into a website-mirrored contract page is gone. destread.go now documents that `diff` and readDestFile swallow the refusal and render a refused destination as empty — a poorer diagnosis than it deserves, left to #229 because fixing it changes what those commands print. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
agentsync statusreports drift across all managed items.agentsync diff [<path>]prints diff between source and destination.agentsync reconcileinteractive prompt loop with hotkeys + bulk +--auto-*flags.internal/source.Writerenables write-back into the canonical source on[w]/--auto-writeback.Test plan
Plan: docs/superpowers/plans/2026-05-04-agentsync-m3-drift.md
🤖 Generated with Claude Code