fix(amico-run): isolate the coordination-ledger contract suite from the production ledger (#642) - #644
Open
aarontrowbridge wants to merge 3 commits into
Open
Conversation
…lated partition (#642) RED tracer: the guard test constructs the coordination service the way the suite always has — bare — and asserts (a) the claim line lands in the suite's per-run tmp partition and (b) the production claims ledger (~/.amico/ledger/claims.jsonl) stays byte-identical to its module-load snapshot. It fails today: with $AMICO_CLAIMS_FILE unset the service's default-path resolution appends every fixture claim to the production ledger (reproduced under a fake HOME: 1 pristine line grew to 6 in one run; on the server this accumulated 176 rows before ops archived the ledger 2026-08-30).
…MS_FILE bridge (#642) GREEN: file-level beforeAll points $AMICO_CLAIMS_FILE at a per-run tmp partition (mkdtemp) — the seam claimsFile() reads per append, so every service construction in the suite, present and future, writes there while production callers keep their default. afterAll adds the order-robust suite-wide guard (production ledger byte-identical to its module-load snapshot, or still absent), restores the env per the repo idiom so nothing leaks across suites, and removes the tmp partition. Verified in all three shapes: pristine ledger byte-identical (sha256 equal), CI shape (no ledger) creates nothing, and the archived real path stays absent. Suite 7/7 green.
…ite the real claims ledger (#642) The runs-ledger backstop above exists because "the risk is in the test someone writes next" — the claims ledger had no such guard, which is exactly how the contract suite polluted it: 176 fixture rows before ops archived the ledger 2026-08-30 as claims.jsonl.archive-20260830-test-pollution (no in-repo action beyond this guard). Mirrors the AMICO_LEDGER pattern: set-if-unset to a /nonexistent path, so an unset claims env fails closed (mkdir at the filesystem root throws; preflight's durable append is deliberately best-effort) instead of resolving to ~/.amico/ledger/claims.jsonl. Suites that want real claim-file I/O point the env at their own tmp partition — the contract suite now does; this is the backstop for the ones that forget. Verified behaviorally with a scratch forgetful suite (bare construction, env unset): polluted the default path before this commit, fails closed after — no file created, claim semantics unaffected (preflight still ok).
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2 tasks
aarontrowbridge
marked this pull request as ready for review
August 30, 2026 11:52
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.
Closes #642
What
The coordination-ledger contract suite was writing synthetic fixture claims into the production ledger: the service's default path resolution falls back to the real claims ledger when no isolation env is set, and the suite constructed the service bare. The production ledger held 176 fixture rows and zero real claims before ops archived it (2026-08-30).
The fix routes every suite claims write through the existing isolation bridge (
AMICO_CLAIMS_FILE→ per-run tmp partition) — a test-only change, zero source edits: the seam already existed (claims-file resolution reads the env per append), so production callers keep their default path untouched. A fail-closed backstop in the shared test setup additionally refuses any future suite that forgets to isolate.Acceptance criteria — evidence
HOME(pristine ledger grew 1→6 lines in one run); GREEN commit: 7/7 tests pass with the guard asserting claim writes landed in themkdtemppartition and the production path stayed byte-identical (suite-wideafterAllre-assert).HOMEshapes: existing ledger → sha256 identical before/after; CI shape (no ledger) → file never created; realHOME→ archived path absent before and after.Director gates (run independently on this branch)
pnpm --filter amico-run typecheck— cleanpnpm --filter amico-run test— 74 files / 1187 passed / 13 skipped / 0 failedJudgment calls
AMICO_LEDGERsibling backstop and makes the whole bug class structurally impossible rather than just this suite — which is the point of the issue.agent_spawn.test.tsfails in any harness that exportsOPENCODE_CONFIG_CONTENT(e.g. sessions inside the amicode server) because theOPENCODE_-prefix allowlist forwards the var to the spawned child. Proven pre-existing on pristineorigin/main(fails identically there; passes 40/40 with the var scrubbed). Follow-up issue filed; not a regression of this branch.Merge
Awaiting human review (afk campaign — merges stay human-only). CI will run on this PR; ready-for-review flips once green.