Skip to content

fix(scope): per-record log attribution, subject-bound OAuth callback logging, canonical container ownership (Spec 105 PR E, FR-007) - #1284

Draft
Dumbris wants to merge 18 commits into
mainfrom
105-e-logs
Draft

Dumbris wants to merge 18 commits into
mainfrom
105-e-logs

Conversation

@Dumbris

@Dumbris Dumbris commented Sep 16, 2026

Copy link
Copy Markdown
Member

Summary

Spec 105 (agent-token scope hardening), planning artifacts and PR A merged in #1279. This PR closes FR007-G1..G6 · roadmap scope-log-attribution · research D8, D9. Gap ids, evidence and test sketches: specs/105-agent-scope-hardening/gap-map.md §1; the refusal shapes it must satisfy: contracts/refusals.md.

Commits

  • 4d358c140 fix(scope): PR E critique round 1 — over-long lines skipped, container_count evidence, audit widened, OAuth stop fields
  • 3d3872cf0 feat(scope): Spec 105 PR E — per-record log attribution, subject-bound OAuth stop, canonical container ownership (FR-007)
  • 81811f73f test(scope): Spec 105 PR E red phase — log attribution, OAuth stop routing, container ownership (FR-007)

Tests

Red-first per gap id (each confirmed failing on origin/main by assertion), pinned pre-105 tests inverted (never deleted), admin controls kept. Full list in the red-phase commit message and the test names above.

Verification

  • go build both editions · go test -race on the touched packages, internal/server (CI skip regex), -tags server serveredition · goldens byte-identical to main · gofmt/vet · ./scripts/test-api-e2e.sh 65/65.
  • golangci-lint v2 could not run locally (binary built with go1.25 vs the go 1.26 target) — CI covers it.
  • Internal adversarial review (security lens + SC-005/test-quality lens), every finding verified by probe before fixing; verdict table in the worktree's .review-tmp/critique-r1.md.
  • Cross-model review: pending — codex gpt-5.6-sol rounds run after this draft opens (Copilot quota for opencode astra is exhausted until 2026-10-01); the PR leaves draft when the reviewer returns CLEAN.

Follow-ups / Spec 105 gaps

See gap-map.md §8 (REST replay has no tool gate; UI active profile bounds no MCP session). Remaining Spec 105 PRs: C → F → G → H1 after this batch.

🤖 Generated with Claude Code

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 16, 2026

Copy link
Copy Markdown

Deploying mcpproxy-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: ccf9833
Status: ✅  Deploy successful!
Preview URL: https://91fc6be3.mcpproxy-docs.pages.dev
Branch Preview URL: https://105-e-logs.mcpproxy-docs.pages.dev

View logs

@codecov-commenter

codecov-commenter commented Sep 16, 2026

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

📦 Build Artifacts

Workflow Run: View Run
Branch: 105-e-logs

Available Artifacts

  • archive-darwin-amd64 (30 MB)
  • archive-darwin-arm64 (27 MB)
  • archive-linux-amd64 (18 MB)
  • archive-linux-arm64 (16 MB)
  • archive-windows-amd64 (30 MB)
  • archive-windows-arm64 (26 MB)
  • frontend-dist-pr (0 MB)
  • installer-dmg-darwin-amd64 (24 MB)
  • installer-dmg-darwin-arm64 (21 MB)
  • smart-mcp-proxymcpproxy-goGH8ZFH.dockerbuild (0 MB)

How to Download

Option 1: GitHub Web UI (easiest)

  1. Go to the workflow run page linked above
  2. Scroll to the bottom "Artifacts" section
  3. Click on the artifact you want to download

Option 2: GitHub CLI

gh run download 35271913259 --repo smart-mcp-proxy/mcpproxy-go

Note: Artifacts expire in 14 days.

Dumbris and others added 17 commits September 17, 2026 22:11
…uting, container ownership (FR-007)

Failing tests for gaps FR007-G1..G6 (tasks T048-T053), all red on HEAD by
assertion; every file compiles against HEAD:

- internal/logs/logger_attributed_test.go: colliding `a/b`/`a_b` writers
  under both encoders, child stderr/launcher text cannot forge the stamp
  (D8 left-to-right boundary rule), legacy unstamped + torn lines withheld,
  subject-evidence rule for historical container/callback records
  (container_owner, sanitised name is never evidence), filter-before-limit,
  case-only names (branches on FS case sensitivity) and forced-rotation
  shared history with administrator outcomes recorded.
- internal/logs/logger.go: ReadUpstreamServerLogTailAttributed signature
  scaffold delegating to the whole-file reader (T054 fills the body).
- internal/server/mcp_tail_log_scope_test.go: colliding-file fixture with
  real stamped writers (closers closed); scoped a_b token gets only own
  records with lines_returned == filtered count; SC-001 differential with
  and without hidden a/b; administrator whole-file pin (green).
- internal/oauth/callback_stop_logger_test.go: StopCallbackServer routes
  the stop/dropped-waiter records through the stopped server's recorded
  logger in both start orders (private manager, observer per server).
- internal/upstream/core/docker_ownership_test.go: sh+awk fake docker via
  SetWellKnownDockerPathsForTest + ResetDockerPathCacheForTest; connect,
  disconnect name-pattern and image-name fallback never mutate or log a
  foreign container; ownership matcher table a vs a-b vs a/b vs A driven
  through ensureNoExistingContainers.
- internal/upstream/core/upstream_logger_audit_test.go (T054a, green pin):
  every upstreamLogger.* call passes a constant message.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d OAuth stop, canonical container ownership (FR-007)

Green phase for gaps FR007-G1..G6 (tasks T054-T059):

- internal/logs/attribution.go: ReadUpstreamServerLogTailAttributed — no new
  field (D8); the existing `server=<raw>` stamp is the ownership signal.
  Console encoder: scan ` | {` boundaries left to right and accept the first
  whose suffix decodes as exactly one complete JSON object; JSON encoder: the
  whole line; every top-level `server` value must equal the requested name;
  subject-evidence rule for container records (container_owner required and
  equal, sanitised names are never evidence) and callback records naming
  another server; filter before the tail limit; lines with no accepted
  boundary withheld. Whole-file reader untouched (SC-005).
- internal/server/mcp.go handleTailLog: attributed reader for scoped callers,
  whole file for administrators; lines_returned counts the authorized tail.
- internal/oauth/config.go: stopCallbackServerLocked logs through the stopped
  server's recorded logger; StopCallbackServerWithLogger no longer adopts a
  logger as the manager logger on stop (signature kept).
- internal/upstream/core/docker_ownership.go + docker.go: every cleanup path
  (ensureNoExistingContainers, disconnect name-pattern fallback, image-name
  fallback, exact-name kill) filters by label com.mcpproxy.server=<raw> AND
  ^mcpproxy-<sanitised>-[a-z0-9]{4}$ server-side and again in Go; foreign
  containers are neither mutated nor logged; every housekeeping record that
  names a container carries container_owner (D9).
- connection_launcher.go loggerWriter: one record per line — a child write
  carrying a line break can no longer start a fresh line (D8 rule 2 relies on
  line boundaries; research.md D8 records the launcher-path finding).
- Inverted pinned tests (T058): mcp_tail_log_scope_test.go fixture writes the
  canary through the real stamped writer plus an unstamped legacy line that
  scoped callers must not see; mcp_secret_redaction_test.go tail_log fixture
  uses stamped records in both encoder shapes for scoped and admin callers.
  Fixture fixes: callback_stop_logger_test waits for both stop records and
  judges the foreign observer by records about the stopped server;
  tailLogLineSignature strips the caller segment.
- docs: tail_log attribution rule + retained effects (agent-tokens),
  container ownership rule and correct label names (docker-isolation);
  tasks.md Phase 5 ticked; ROADMAP.md regenerated.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r_count evidence, audit widened, OAuth stop fields

Spec 105 FR-007 critique round 1 (security + parity/tests):

- internal/logs: the attributed reader no longer turns a >1 MiB line into a
  scoped-caller error (bufio.ErrTooLong); over-long lines are consumed as
  non-attributable so the response class is independent of hidden
  co-owners (SC-001). `container_count` is a container subject under D8
  rule 3 (pre-105 sweep counts included co-owners' containers).
- internal/upstream/core: the sweep's count record carries container_owner;
  producer audit now covers every zap level call on any receiver across
  core, launcher and oauth (oauthLogger tee) with two reviewed exceptions;
  loggerWriter newline split pinned; ownsContainer tested directly and
  through a filter-blind fake-docker mode.
- internal/oauth: stop/dropped-waiter records no longer re-add
  server/bind_host/port the recorded logger already carries.
- internal/server: differential gains a true absent-co-owner arm; T048
  oracle justification recorded.
- docs/specs: GET /api/v1/servers/{id}/logs is not an administrator
  reader (whole-file, agent-reachable) — documented and recorded as a
  gap-map §8 follow-up with the whole-file 64 KiB cap; docker-isolation
  documents cidfile-only tracking for user `docker run` servers and stale
  containers after a rename; research D8 records the over-long rule,
  profile-scoped administrator decision and audit scope.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rship on cidfile and exact-name container paths

Codex round 1 raised three findings against PR E (Spec 105 FR-007); all
three reproduced red and are fixed here.

1. internal/logs: a torn foreign console record (partial final write, no
   terminator) followed by an appended complete `a_b` record shares one
   physical line. The reader rejected the fragment's ` | {` boundary and
   scanned on to a_b's, attributing the whole line — a/b fragment included —
   to a_b. The accepted boundary is now the FIRST ` | {` on the line; a
   first boundary that does not decode marks the line non-attributable. A
   `{`-prefixed line is judged as one JSON object and no longer falls
   through to the console scan (a torn JSON-era fragment before a console
   record was disclosed the same way). Trade-off pinned in
   ChildTextCannotForgeOwner: a launcher-pumped child line containing
   ` | {` under the console encoder is now withheld from its own writer
   too, never misattributed. Residual documented in attribution.go: a tear
   inside the message part leaves only message text ahead of the later
   record and is indistinguishable from that record's message.
   Regression: ConcatenatedTornFragmentWithheld (3 shapes × 2 encoders).

2. internal/upstream/core, cidfile path: the id read from --cidfile was
   recorded as owned, written into the per-server log with a fabricated
   container_owner=<requesting server>, and stopped/killed on disconnect
   without ownsContainer. A user-configured direct `docker run --name custom`
   gets a cidfile but neither the label nor a canonical name, so under D9 it
   is not ours. trackCidfileContainer and killDockerContainerWithContext now
   inspect the container (docker ps -a --filter id= with the ownership
   format) and apply ownsContainer before acting and before writing any
   record; container_owner is the label read back. A container failing
   ownership is left alone and never named in the per-server log.
   Consequence (docs/features/docker-isolation.md): MCPProxy no longer stops
   a user `docker run` server's container at all — use --rm.

3. internal/upstream/core, exact-name paths (cidfile name recovery and
   killDockerContainerByNameWithContext): filtered by label and tracked name
   only, never applied ownsContainer, and attributed with the requested
   server. Both now go through lookupOwnedContainerByName (label + anchored
   QuoteMeta name filter, Go-side ownsContainer) and record the label read
   back. Production only ever tracks the generated canonical name, so the
   reachable hole was nil; fixed as a contract violation.

Every lookup now flows through listOwnedContainersFiltered, so no path can
act on or log a row the predicate did not admit. stopOwnedContainer reports
success and writes the outcome to the per-server log (keeping the records
the cidfile path wrote before). cidfile poll interval/attempts are package
vars so the recovery fallback is unit-testable. Fake docker gains an id=
filter.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…safe deprecated OAuth start, child output as a container subject

Logs (attribution.go):
- A console record is accepted only when the text in front of its first
  ` | {` boundary is exactly one record header at offset 0 (a foreign
  record torn inside its message left only text in front of the later
  record and was handed to the later writer); a suffix that is itself a
  JSON-encoder record (level+ts+msg) is rejected.
- Line cap measured on content: exactly 1 MiB stays eligible.
- Child output is a container subject when it names one: producers stamp
  child_output=true and the reader withholds a child-output record that
  mentions a container id, a canonical container name or Docker's
  name-conflict phrase unless container_owner matches.

OAuth (config.go): the deprecated StartCallbackServer no longer resolves
its logger through adoptLoggerLocked(nil) — a caller without a logger
records the zap global, never the last-installed server's tee, so a
server started that way cannot write its start/stop records into
another server's log.

Docker: the pre-spawn "Docker isolation configured" record carries no
container_owner for the generated, unverified name; loggerWriter writes
the launcher-pumped child line as the `message` field of a constant
"launcher" record (audit exception removed); monitorStderr stamps
child_output. Fixture: a/b vs hidden a-b, fake docker run answering with
the daemon's conflict text naming a-b's container, asserted through the
real per-server file, the attributed reader and tail_log.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…est.go already declares shellQuote under the linux tag

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…xture

Ubuntu runners ship /usr/bin/docker, and the resolver's PATH lookup won
over the well-known-path seam, so the owned-container cells saw the real
daemon's empty ps. The fixture now builds a PATH dir linking only sh,
awk, printf and cat. Verified on ubuntu:24.04 with a decoy docker.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two lumberjack sinks on one file cannot rotate there: the rename fails
while the co-owner holds the file open, so the fixture premise never
holds. The attributed reader under test is platform-neutral.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d-back container owner

The whole-manager sweeps (cleanupAllManagedContainers, ForceCleanupAllContainers)
stamped container_owner only on their intent records; the stop/kill/rm outcome
records — success and failure — named the container's id or name without it,
and the kill loop threaded a bare id slice that dropped the owner altogether.
Every record a sweep writes that names a container now carries the owner Docker
reported for it at selection (Spec 105 FR-007 / research D8, D9).

The kill loop iterates the selected rows directly; its guard was always true
past the early return, so removing it is behaviour-preserving.

Test: the fake docker gains a Running column (ps -q answers running rows) and a
fail file (stop/kill/rm exit 1) so both the force-kill and the error branches
fire; every main.log record whose field values carry the own container's id or
name must carry container_owner, on both arms.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ion time, disconnect-timeout records name the container only with evidence

Codex round 5, docker chunk (Spec 105 FR-007 / D8, D9):

1. forceCleanupClient (disconnect timeout) named the tracked container id
   before ownership was verified and on every outcome without
   container_owner. core.Client.ForceRemoveTrackedContainerIfOwned now hands
   back the owner label it read at the rm -f; the manager's pre-verification
   intent names the server only, the removed / rm-failed outcomes carry the
   id with that owner, and a rejected or unverifiable container is never
   named by id.

2. The shutdown and emergency sweeps stopped, killed and removed containers
   on the ownership their initial docker ps established. Every mutation now
   goes through reverifyOwnedManagedContainer, which re-reads the name and
   label immediately before the stop/kill/rm and re-applies
   core.ContainerOwnedByAny: a container renamed or relabelled since the
   listing is left alone and the refusal recorded without its id or name;
   the container_owner on every record is the value read at mutation time.

Tests: TestForceCleanupClient_NamesTheContainerOnlyWithOwnershipEvidence,
TestSweeps_ReverifyOwnershipAtMutationTime (fake docker gains a post-listing
fixture swap); TestForceRemoveTrackedContainerIfOwned_AppliesOwnership
asserts the returned owner. Docs: research D9, docker-isolation.md.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…wnership through one helper

Codex round 6 found the round-5 moment-of-mutation rule applied only in the
manager sweeps: the core's image-fallback, name-pattern and pre-creation
cleanups stopped / rm -f'd the rows a listing returned, the kill after a
failed `docker stop` was a second mutation with no read at all, and the
docker-logs monitor read the cidfile itself on its wait timeout and named
that id with an executable `docker logs` command.

- core.ContainerMutator.Mutate is now the single verify-then-mutate
  implementation: `docker ps -a --no-trunc --filter id=` immediately before
  the command, exact FULL-id match (every listing runs --no-trunc, so a
  replacement whose id extends the listed one cannot be admitted), the
  ownership predicate re-applied, the row read handed back as the only
  source of container_id/container_owner on the caller's records; a refusal
  names no id or name.
- core: stopOwnedContainer takes an id and re-verifies before the stop AND
  before the escalating kill; ensureNoExistingContainers re-verifies per
  row (a/b vs a-b collision); the per-row listing records become a count.
- monitoring: the docker-logs monitor never reads the cidfile; it names
  only the tracked, verified container (new Client.containerOwner) and
  records nothing but the timeout otherwise.
- manager: the private reverify is deleted; the three sweep sites go
  through the same helper; the fake docker's --filter id= is a prefix match
  like docker's and the round-5 test gains the id-extending arm.
- tests: swapFixtureAfterPs(n) + failVerbs on the core fake docker;
  TestDockerMutations_ReverifyOwnershipAtMutationTime (4 paths x 4 arms),
  TestDockerCleanup_PreCreationReverifiesEachRow_SlashVsDashCollision,
  TestDockerStopEscalation_ReverifiesBeforeKill,
  TestMonitorDockerLogs_NamesOnlyAVerifiedContainer.
- docs: research D9 and docs/features/docker-isolation.md.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…th checks name no id without ownership evidence

Two MUST-FIX findings from codex round 7 (docker.go chunk; the logs chunk
stayed clean and untouched):

1. trackCidfileContainer's err!=nil and !ok branches logged
   shortContainerID(containerID) into the main logger even though the
   container's ownership could not be verified or had failed the
   predicate — the same refusal rule mutateOwnedContainer already applies
   (server + reason only, never an id) was missing here. Both branches now
   log only the server name and the reason.

2. Manager.verifyContainerHealthy decided health from `docker inspect
   <stored-id>` alone. inspect answers by id regardless of name or label,
   so a container another Docker client relabelled or renamed after
   tracking still read back Running=true under the same id, and
   ForceReconnectAll treated it as healthy — skipping recovery for a
   container that was no longer canonically this server's. The check is
   now split into a pure verifyDockerContainerHealthy that re-establishes
   ownership through the same read+predicate ContainerMutator uses for
   every mutation (a new ContainerMutator.Verify, factored out of Mutate
   so both share one implementation) before trusting inspect: a container
   that fails the predicate now is reported unhealthy with no id in the
   record, and ForceReconnectAll's existing rebuild (RemoveServer +
   AddServer) both recovers and clears the stale tracked id/owner as a
   byproduct. A container ownership confirms is named, with
   container_owner from that same read.

Tests: TestDockerCleanup_CidfileRefusal_MainLogRecordsNoID (not-owned and
docker-read-failure arms) and
TestVerifyDockerContainerHealthy_ReverifiesOwnershipBeforeInspect
(relabelled, renamed, read-failure, unchanged-running, unchanged-stopped)
via the fake docker shims, extended with an `inspect` branch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… name a container only with evidence

Two MUST-FIX findings from codex round 8 (Spec 105 D8):

1. Four lifecycle housekeeping log sites (Connect failure, connectStdio
   init failure, initialize() init failure, Disconnect by-ID/by-name)
   named the cached container id, or a merely GENERATED canonical name
   never observed from Docker, without container_owner. containerID is
   assigned only by trackCidfileContainer or the cidfile-timeout
   name-recovery fallback, both of which verify ownership via Docker's
   read-back before ever setting it and always pair it with
   containerOwner. A new dockerContainerLogFields helper is the single
   place all four sites build their fields from: nil when containerID is
   empty (so an unverified generated name never gets logged as evidence),
   otherwise container_id/container_name/container_owner from the tracked,
   already-verified state.

2. GetConnectionDiagnostics published the cached container id and
   inspected it by id alone, the same stale-ownership gap
   verifyDockerContainerHealthy closed for the manager's health path in
   round 8. It now routes through ContainerMutator.Verify first: a
   container that no longer passes the ownership predicate is reported as
   absent (container_running=false, no id), and only a container ownership
   confirms right now is published, with the owner read back at that same
   moment.

Tests via the fake docker shim and a failing MCP transport, covering both
arms at every site: TestDockerContainerLogFields (the helper),
TestInitializeFailure_DockerCleanupLog, TestDisconnectWithContext_DockerCleanupLog,
TestConnectStdioDirectDockerRun_ContainerEvidence (a real Connect ->
connectStdio -> initialize chain for a direct `docker run` upstream), and
TestGetConnectionDiagnostics_ReverifiesOwnershipBeforePublishing. All shown
red on the pre-fix code.

Swept every remaining container_id/container_name zap field across
internal/upstream/core and manager.go; every other site already pairs with
container_owner from a same-moment Docker read.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…xture

On Linux the spawn keeps the login-shell wrap unless DOCKER_HOST or
DOCKER_CONTEXT is already in the process env; the Landlock job has
neither, so the fixture's poisoned SHELL was exec'd and the connection
failed at start — before the initialization-failure lifecycle site the
evidence test expects. Set DOCKER_HOST in the fixture so every job runs
the shim through the same direct-exec path.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…covery failure carry no unowned container evidence

Two MUST-FIX findings from codex round 9 review (docker chunk; logs
chunk clean):

- Three housekeeping records (image cleanup, name-pattern cleanup, the
  pre-creation sweep's main-log record) published container_count
  without container_owner. D8 rule 3 treats a container count as
  container-subject evidence, so each now pairs the count with the
  label Docker reported on the listed rows (the pre-creation sweep's
  paired upstreamLogger record already did this).
- The terminal cidfile-recovery failure named c.containerName — a
  generated name never read back from Docker — as orphaned. Under a
  suffix collision that name can currently belong to a colliding
  server, so the record now names only the server, matching the
  round-9 lifecycle fixes for every other generated-name-only state.

Red tests first (docker_review_round11_test.go), all four failed
pre-fix and pass post-fix. Full existing PR suite green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…cal ownership, sweep counts are owner-grouped

codex round 10 docker findings (.review-tmp/codex-r10-docker.txt), both confirmed
by trace and both MUST-FIX per the maintainer's decision:

1. HasDockerContainers selected by the shared, copyable com.mcpproxy.managed /
   com.mcpproxy.instance labels alone, unlike the sweeps' listOwnedManagedContainers
   selection (label + canonical name via core.ContainerOwnedByAny). A foreign
   container that copies those labels, or an orphan of a server removed from
   config, therefore read as "still running" and drove the runtime/server
   shutdown path into its 15s cleanup-verification wait, a second force-clean,
   and a false "Some containers may still be running after force cleanup"
   report for a container mcpproxy neither started nor can act on (D9).

   Fix: readManagedContainers/listOwnedManagedContainers take an includeStopped
   bool (the same convention core.Client.listOwnedContainersFiltered already
   uses) so a caller can ask for the running-only `docker ps` real Docker
   defaults to. HasDockerContainers now calls listOwnedManagedContainers(ctx,
   false, ...) and reports len(owned) > 0 — the sweeps pass true, unchanged.

2. The shutdown sweep's "Found mcpproxy-managed containers to cleanup" and the
   emergency sweep's "Force removing managed containers" records published a
   bare aggregate count with no container_owner. A sweep can select containers
   belonging to more than one configured server, so the count could not be
   bound to a subject (D8).

   Fix: logOwnerGroupedCounts replaces the aggregate with one record per
   Docker-read owner, each carrying container_owner and its own count.

Tests: TestHasDockerContainers_AppliesCanonicalOwnership (foreign-label,
orphaned-owner, canonically-owned-running, canonically-owned-stopped,
docker-unavailable) and TestSweepCounts_AreOwnerGrouped (two owners, two
records) — both red before the fix. The manager's fake-docker shim gained
`-a` handling (a plain `ps` now answers running rows only, matching real
docker) to make the running/stopped distinction testable.

Swept manager.go and internal/upstream/core for any other container count/
id/name field published without container_owner: none found — the one
intentionally unpaired count (listOwnedManagedContainers' "Skipping
containers..." Warn) counts rows that by definition have no established
owner, per its existing comment.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR E review round 14 — listOwnedManagedContainers still logged a bare
"count" of rows that failed canonical ownership. Those rows' labels are
untrusted (that's exactly why they were rejected), so no owner could be
attributed to the tally, violating D8's rule that every container count
must carry the owner it counts. Drop the log line entirely rather than
fabricate an owner; nothing downstream consumes the field.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ad, scoped log tail bounded by budget

codex round 13 docker findings (.review-tmp/codex-r13-docker.txt), both confirmed
by trace and both MUST-FIX per the maintainer's decision:

1. TOCTOU: GetConnectionDiagnostics and the manager's health check
   (verifyDockerContainerHealthy) each verified ownership with one
   ContainerMutator.Verify (docker ps) read, then issued a SEPARATE, later
   `docker inspect <id>` to decide running/status. Between the two, another
   Docker client could relabel or rename the container into a colliding
   server's namespace; the inspect then reported the NOW-FOREIGN container's
   state while the code kept attributing it to the original server.

   Fix: containerRowFormat gained a fourth field, {{.Status}} — docker ps's
   own human STATUS text — captured in the SAME docker ps -a read
   ContainerMutator.read already makes. ContainerRow gained a Status field
   and a Running() bool method deriving running state from Docker's own "Up"
   prefix convention (verified against a live daemon: docker ps --format has
   no .Running field at all, and .State is State.Status, not State.Running —
   a paused container shows State.Status="paused" but State.Running=true,
   and its docker ps STATUS text is "Up ... (Paused)", so the "Up" prefix
   carries the same information .State.Running would). Both call sites now
   derive running/status from the Verify read's row alone; the separate
   `docker inspect` calls are deleted entirely.

2. Timing-class (SC-005): ReadUpstreamServerLogTailAttributed scanned the
   shared log file from byte 0, filtering attributable lines, before taking
   the last N — so a scoped caller's response time was proportional to a
   hidden co-owner's entire earlier volume in the shared file, a
   response-time side channel the non-disclosing-refusal definition (status,
   body AND timing class; SC-005) forbids.

   Fix: the scan now Stats the file and Seeks to scopedBackwardStartOffset
   (at most scopedBackwardReadBudget, 16 MiB, bytes before EOF) before the
   unchanged filter-then-limit forward scan — readBoundedLine,
   recordAttributableTo and every D8 boundary/subject-evidence rule are
   untouched, only where the scan starts is new. Below the budget (the
   common case) this returns byte-identical results to before; past it, a
   request whose own recent records sit further back returns fewer than
   requested rather than reading further — bounded and fail-closed, never an
   error.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.

2 participants