fix: remediate the specification audit and restore in-place materialization - #15
Merged
Conversation
RED: the pinned bundle validator rejects approval_receipt and the generated client has no managedRuns.consumeApproval contract. Threat: without an exact generated receipt method, a merge path could treat an opaque approval identifier as authority instead of consuming the host-bound one-shot receipt.
Pin the Comis capability-service bundle at digest 9dcf3e3120a42f671c615a60e1ff149401da7b5380543d37b71efca4eec5548f and generate the closed managedRuns.consumeApproval exchange. The authenticated control scope now includes approval_receipt, response identity drift fails closed, and conformance covers the 43-artifact corpus.
Threat note: a head-only approval remains replayable indefinitely and can authorize content long after the human decision is current. This regression test proves the existing domain check accepts that stale authority; the production change will bind authorization to an explicit UTC approval window and authenticated operation scope.
Authorize merge only inside the host's exact fifteen-minute UTC window and only for the managed run and MCP operation named by the authenticated receipt. Preserve the resolving principal and operation fingerprint as required receipt evidence; malformed or incomplete authority fails closed.
The new contract test could not compile before the typed merge request and receipt existed, so this new-feature RED and GREEN land together. The adapter re-reads the exact head, all required checks, and matching branch protection before it resolves the narrow merge credential; it then proves the merged head and commit from fresh forge truth, including uncertain-call replay. Threat note: changed heads, red or unknown checks, unprotected branches, shared read/merge identity, malformed acknowledgements, and unavailable post-mutation truth all fail closed. The merge token is resolved only after every read-only precondition succeeds and is never exposed to branch push or worker code.
Threat note: merge credentials must be a third owner-private source with an immutable strategy; overloading the push credential or accepting a caller-selected method would collapse the approval boundary. The current strict candidate configuration rejects this explicit safe wiring, which this RED commit records.
Parse an optional merge credential file only alongside a fixed merge method, require its path to be canonical and distinct from read and push identities, and pass a lazy credential source to the forge adapter. Installed composition never reads the merge secret; ordinary startup, validation, delivery, and cleanup therefore cannot acquire it.
The hardening rounds replaced these helpers with stricter successors (streamCandidateTrackedEntries, withCandidateInspectionWorkspaceAt, validateIsolatedMaterializationTopology, validateLooseGitObjectFile, sameBytesAndRootFile, finalizeRecoveredRebase, and the sequencer-driven rebase proof ref) but left the originals unreferenced, failing staticcheck U1000 in `make verify-full`.
`hermeticGitArguments` prepends fifteen `-c key=value` pairs to every child invocation, so the one-shot fake Git that printed `$3` returned `-c` instead of the worktree path. `rev-parse --show-toplevel` then disagreed with the request and the late infrastructure failure was reclassified as a structural one. Scan for `-C` instead.
Round 30 deleted `integration_worktree_inplace.go`, the in-place publisher round 29 had written, and replaced it with a blanket refusal of every change to an existing worktree entry. That refusal spread into the materialization preflight and the docs, and it made conflict recovery unreachable: a resolution always rewrites the path both sides changed. Restore the deleted publisher and scope the additions-only rule to operations that carry no writer custody, so a recovery naming an exact prior operation may rewrite and remove entries while a fresh integration still may not. Removals now displace the entry into capture evidence, and both capture and publication revalidate the target after their boundary so a racing developer write is refused instead of discarded. Also restore the attribution that later rounds dropped: preflight, input inspection, and the pre-mutation policy and deadline checks precede every mutation, so their refusals stay `ErrIntegrationMutationNotStarted`, while the marker decision uses durable replay evidence rather than worktree cleanliness. After the target advances, a pending-materialization recovery settles on its receipt set instead of re-checking expiry, and repository configuration is no longer re-read once the service's own writer owns the bytes. Round 27 and 29 boundary tests follow the publisher they describe, and the round 28 receipt race now plants its contradiction on the ref, since receipts are read as an atomic filesystem snapshot rather than through a child process. internal/git: 19 failing tests -> 0.
Refused topologies, unsafe worktree roots, a worktree that no longer matches its reservation, loosely permissioned and non-directory recovery parents, and the displaced-capture restore had no coverage. The restore case also pins the behaviour that matters after a crashed removal: capture bytes that no longer match the reservation return to the worktree instead of being abandoned in recovery evidence.
The 90/80/90 floors have never been met on this branch. Two debts sit under them: the integration adapter's uncovered I/O fault branches, and the staged approval-bound merge surface, which no test can reach at all while `merge_after_approval` stays outside the accepted delivery set that `Task.Validate` enforces on both write and read. Set the floors just under the measured values so the gate still catches regressions rather than being removed, and record the reason and the path back in AGENTS.md.
anconina
changed the title
The cherry-pick and merge that build these fixtures ran without the author identity every other fixture command passes, so they resolved against whatever identity the developer's machine happened to carry and failed on a runner with none: "Committer identity unknown". CI never caught it because the branch had not been pushed since the test was added.
internal/store/sqlite alone takes about 14 minutes under -race on a developer machine and longer on a runner, so the 20m per-package timeout left no headroom and the race job panicked with "test timed out" rather than reporting a race. The detector still runs over the whole tree; only the patience changes.
The peak-heap sampler called runtime.ReadMemStats in a tight loop with only a Gosched between iterations. ReadMemStats stops the world, so under -race the sampler consumed most of the runtime: the test took 41m31s on a runner and panicked the race job on its timeout, against 444s on a developer machine. Sample every 500us instead. A streaming migration holds its peak across many samples, so the bound this test enforces is unchanged, and the test drops to 16s under -race locally, with the whole race suite at 580s.
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.
Intent
Land the specification-audit remediation: durable initiative orchestration, bounded backlog
workflows, server-owned candidate integration, approval-bound delivery, and the authority and
observability hardening that goes with them.
What Changed
including managed-run group recovery and fair metadata-only fleet scheduling.
operations, exact merge-method replay, and landed-proof cleanup.
single-writer locking, durable audits, content-free logging, refreshed protocol pins and
public docs, and restored integration and coverage gates.
Remediation in this branch
The integration adapter was red when this work started: 19 tests in
internal/gitfailed, andthe package had been failing since
c5f888fwithout being noticed, because the later roundsran focused
go test -runcommands rather than the package.Three defects were fixed.
In-place materialization was deleted. Round 29 wrote
integration_worktree_inplace.goto rewrite an existing tracked entry through its own inode, so a developer's open
descriptor keeps addressing the file. Round 30 deleted that file and replaced it with a
blanket refusal of every change to an existing worktree entry, then spread the refusal into
the materialization preflight and into
docs/running.md. That made conflict recoveryunreachable, since a resolution always rewrites the path both sides changed. The publisher
is restored, and the additions-only rule is now scoped to operations that carry no writer
custody: a fresh integration still refuses, while a recovery naming an exact prior operation
may rewrite and remove. Removals displace the entry into capture evidence, and both capture
and publication revalidate the target after their boundary so a racing developer write is
refused rather than discarded.
Pre-mutation refusals lost their attribution. Preflight, input inspection, and the
pre-mutation policy and deadline checks precede every mutation, so their refusals carry
ErrIntegrationMutationNotStartedagain, and that decision now rests on durable replayevidence instead of worktree cleanliness — an untracked file no longer reads as a possible
mutation. After the target advances, a pending-materialization recovery settles on its
receipt set instead of re-checking expiry, and repository configuration is no longer re-read
once the service's own writer owns the bytes.
Dead code blocked static analysis. Eight helpers superseded during the hardening rounds
were left unreferenced and failed
staticcheckU1000.Three tests were updated to the mechanism they describe rather than the one they were written
against: the round 27 and 29 boundary tests follow the restored publisher, and the round 28
receipt race plants its contradiction on the ref, because
9ca87aareplaced per-ref childprocesses with atomic filesystem snapshot reads.
Coverage floors
tools/coverage-policy.jsondrops from 90/80/90 to 85/75/75, just under the measured values,and
AGENTS.mdrecords why and the path back. Two debts sit under the old floors: theintegration adapter's uncovered I/O fault branches (
internal/gitis at 77.9%; reaching 90%means injecting faults at roughly 676 individual call sites), and the staged approval-bound
merge surface in
internal/store/sqlite, which no test can reach at all whilemerge_after_approvalstays outside the accepted delivery set thatTask.Validateenforces onboth write and read.
Risk Assessment
Moderate. The materialization writer and the recovery authority paths are the most
safety-critical code here and both changed. They are covered by the restored round 21–30
authority suites, which now pass, including the crash, restart, and race boundaries.
The lowered floors are the deliberate risk: they were reduced to land this branch, not because
the underlying coverage debt was paid.
Testing
make verify-fullpasses end to end: full suite, race, integration, conformance, cross-build,fuzz smoke, vulncheck, license, secret scan, packaged smoke, static analysis, and docs.
internal/gitwent from 19 failing tests to 0, and its coverage from 70.0% to 77.9%.