applecontainer: Inspect + FindContainerByLabel (PR-B)#60
Merged
Conversation
Third PR of M6. Replaces the InspectContainer / InspectImage /
FindContainerByLabel stubs with real implementations driven through
the bridge. Also lands the ac_bridge.h header-comment style guide
queued for this PR.
Bridge (Swift):
- Helpers.swift: shared runSync (Task + DispatchSemaphore + hard wait
timeout), encodeOK / encodeOKNull / encodeErr envelope helpers,
ISO8601 date encoding strategy on the JSONEncoder so Apple's Date
fields decode cleanly into Go's time.Time.
- inspect.swift: ac_inspect_container (ContainerClient.get),
ac_inspect_image (ClientImage.get + image.config().Labels — the
load-bearing devcontainer.metadata fast path), and
ac_find_container_by_label (list + filter + most-recently-started
wins).
- bridge.swift: refactored to use the shared helpers; ac_ping keeps
its predates-style-guide flat shape for PR-A stability.
- ac_bridge.h: full header-comment style guide describing the five
contracts every export documents (ownership, cancellation,
threading, error encoding, blocking). All exports — ac_version,
ac_ping, ac_free, plus the three new ones — documented.
cgo shim (Go side):
- shim.h / shim.c: add ac_inspect_container_p, ac_inspect_image_p,
ac_find_container_by_label_p function-pointer slots; dlsym them in
ac_load; reset and dlclose on partial-failure paths.
Runtime methods (inspect_darwin_arm64.go):
- decodeEnvelope[T] generic decoder for the canonical {ok, err, data}
shape with null-data tolerance for the find-by-label miss path.
- containerSnapshot / imageInspectPayload mirror the Apple JSON
shapes we consume; only the fields we need are listed.
- snapshotToContainer / snapshotToDetails / mapState project Apple
types into runtime.Container / runtime.ContainerDetails. Created,
FinishedAt, ExitCode are zero — not in Apple's ContainerSnapshot;
surfaced via additional XPC calls in later PRs.
- decodeUserString handles ProcessConfiguration.User's
raw/id enum variants.
- mapInspectErr / mapImageInspectErr translate Apple's "notFound"
errors into runtime.ContainerNotFoundError /
runtime.ImageNotFoundError so the engine's typed-error contracts
hold across backends.
Tests (darwin/arm64 only — no daemon needed for envelope tests):
- envelope_test.go: success / failure / null-data / malformed-JSON /
mapState. Pure Go, no cgo at runtime.
- inspect_darwin_arm64_test.go: smoke tests against a live daemon.
InspectContainer round-trip with a labeled container,
InspectContainer not-found typed-error contract, InspectImage
with a warmed alpine image, InspectImage not-found typed-error,
FindContainerByLabel hit + miss paths.
Test plan:
- `make bridge && go test ./runtime/applecontainer/...` — 11 tests
pass (5 envelope unit tests + 6 daemon smoke tests)
- `go test ./...` — full repo green
- `GOOS=linux GOARCH=amd64 CGO_ENABLED=0 go build ./runtime/applecontainer/...`
— stub still compiles
- `go vet ./...` clean
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds three Swift C-exported inspection entrypoints (container, image, label lookup) that run async work synchronously via a shared runSync helper and return deterministic JSON envelopes. The C header documents semantics; the shim resolves the symbols and the Go runtime decodes envelopes and maps them to runtime types with tests. ChangesContainer/Image Inspection via Bridge
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
applecontainer-bridge/Sources/ACBridge/Helpers.swift (1)
67-73: 💤 Low valueConsider escaping additional JSON control characters.
The escaping handles
\,", and\n, but other control characters (\r,\t,\b,\f) could also produce invalid JSON if they appear in error messages. While unlikely for typical Apple framework errors, adding them would make the encoder more robust.♻️ More complete escaping
func encodeErr(_ error: Error) -> String { let msg = String(describing: error) .replacingOccurrences(of: "\\", with: "\\\\") .replacingOccurrences(of: "\"", with: "\\\"") .replacingOccurrences(of: "\n", with: "\\n") + .replacingOccurrences(of: "\r", with: "\\r") + .replacingOccurrences(of: "\t", with: "\\t") return "{\"ok\":false,\"err\":\"\(msg)\"}" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@applecontainer-bridge/Sources/ACBridge/Helpers.swift` around lines 67 - 73, The encodeErr function only escapes backslashes, double quotes and newlines; update it to also escape JSON control characters (\r, \t, \b, \f) so error strings always produce valid JSON. In the encodeErr(_ error: Error) -> String function add replacingOccurrences calls (or a single pass normalizer) to replace "\r" with "\\r", "\t" with "\\t", "\u{08}" (backspace) with "\\b" and "\u{0C}" (form feed) with "\\f" before embedding msg into the JSON payload, keeping the existing escapes for "\\" and "\"" and "\n".runtime/applecontainer/inspect_darwin_arm64.go (1)
319-336: 💤 Low valueConsider using
strings.Containsinstead of customindexOf.The custom
indexOfimplementation adds maintenance overhead to avoid a single import. Thestringspackage is part of the standard library and commonly used. Unless there's a specific constraint (e.g., build size), usingstrings.Containswould be cleaner.♻️ Use standard library
+import "strings" + func containsAny(s string, subs ...string) bool { for _, sub := range subs { - if indexOf(s, sub) >= 0 { + if strings.Contains(s, sub) { return true } } return false } - -// indexOf is strings.Contains/strings.Index minus the import — keeps -// this file's deps narrow (and avoids re-importing strings for this -// one use). Inline implementation, O(n*m), fine for our short error -// strings. -func indexOf(s, sub string) int { - if len(sub) == 0 { - return 0 - } - if len(sub) > len(s) { - return -1 - } - for i := 0; i+len(sub) <= len(s); i++ { - if s[i:i+len(sub)] == sub { - return i - } - } - return -1 -}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@runtime/applecontainer/inspect_darwin_arm64.go` around lines 319 - 336, Replace the custom indexOf function with the standard library: add an import for "strings", remove the indexOf(s, sub string) function, and update call sites—if code only checks presence, change indexOf(s, sub) != -1 to strings.Contains(s, sub); if the code needs the integer offset, use strings.Index(s, sub) which returns -1 on no match. This keeps behavior identical while using the built-in strings package.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@runtime/applecontainer/inspect_darwin_arm64_test.go`:
- Around line 98-128: The test TestInspectImage_LabelsRoundTrip promises a
non-nil Labels contract but only logs it; add an explicit assertion after
calling rt.InspectImage to fail the test if details.Labels is nil (e.g. if
details.Labels == nil { t.Errorf("Labels is nil") }), so the contract is
enforced; place the check near the existing inspections (after verifying
details.Tags/ID and before or after the t.Logf) and reference the details
variable returned by rt.InspectImage.
- Around line 41-50: The helper cliRunStrict currently calls t.Skipf on any
non-zero exit which masks real failures; update it so that after running
exec.Command("container", args...).CombinedOutput() it checks whether the error
indicates the CLI binary is missing (use errors.Is(err, exec.ErrNotFound) or
inspect exec.ExitError vs lookup error) and only then call t.Skipf, otherwise
call t.Fatalf (or t.Fatalf-like) including the args, error and trimmed output to
fail the test on real setup failures; refer to the cliRunStrict function and the
exec.Command(...).CombinedOutput() / t.Skipf usage to locate where to change the
behavior.
---
Nitpick comments:
In `@applecontainer-bridge/Sources/ACBridge/Helpers.swift`:
- Around line 67-73: The encodeErr function only escapes backslashes, double
quotes and newlines; update it to also escape JSON control characters (\r, \t,
\b, \f) so error strings always produce valid JSON. In the encodeErr(_ error:
Error) -> String function add replacingOccurrences calls (or a single pass
normalizer) to replace "\r" with "\\r", "\t" with "\\t", "\u{08}" (backspace)
with "\\b" and "\u{0C}" (form feed) with "\\f" before embedding msg into the
JSON payload, keeping the existing escapes for "\\" and "\"" and "\n".
In `@runtime/applecontainer/inspect_darwin_arm64.go`:
- Around line 319-336: Replace the custom indexOf function with the standard
library: add an import for "strings", remove the indexOf(s, sub string)
function, and update call sites—if code only checks presence, change indexOf(s,
sub) != -1 to strings.Contains(s, sub); if the code needs the integer offset,
use strings.Index(s, sub) which returns -1 on no match. This keeps behavior
identical while using the built-in strings package.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c337338b-6a11-48b7-96e8-85a9d7d851a2
📒 Files selected for processing (10)
applecontainer-bridge/Sources/ACBridge/Helpers.swiftapplecontainer-bridge/Sources/ACBridge/bridge.swiftapplecontainer-bridge/Sources/ACBridge/inspect.swiftapplecontainer-bridge/include/ac_bridge.hruntime/applecontainer/envelope_test.goruntime/applecontainer/inspect_darwin_arm64.goruntime/applecontainer/inspect_darwin_arm64_test.goruntime/applecontainer/runtime_darwin_arm64.goruntime/applecontainer/shim.cruntime/applecontainer/shim.h
Two CodeRabbit comments on #60, both valid: cliRunStrict was masking setup failures by t.Skipf-ing on any non-zero exit. Switched to errors.Is(err, exec.ErrNotFound) to detect a missing `container` CLI (legitimate env mismatch — skip) and t.Fatalf otherwise. Real regressions now surface as test failures instead of silent skips. InspectImage's Labels-non-nil contract was described in comments but not actually enforced — the bridge would emit `null` when the underlying OCI config had no labels, leaving Go with a nil map. Tightened the Swift ImageInspectPayload to non-optional `labels: [String: String]` and `env: [String]` with empty-collection defaults, then added the missing assertion in TestInspectImage_LabelsRoundTrip. Callers (engine devcontainer.metadata fast path) can now read keys without nil- guarding. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Third PR of M6. Replaces the
InspectContainer/InspectImage/FindContainerByLabelstubs with real implementations driven throughthe bridge. Also lands the
ac_bridge.hheader-comment style guidequeued for this PR.
What lands
Bridge (Swift)
Helpers.swift— sharedrunSync(Task + DispatchSemaphore + hardwait timeout),
encodeOK/encodeOKNull/encodeErrenvelopehelpers. ISO8601 date encoding strategy so Apple's
Datefieldsdecode cleanly into Go
time.Time.inspect.swift— three new exports:ac_inspect_container(wrapsContainerClient.get)ac_inspect_image(wrapsClientImage.get+image.config()—surfaces OCI Labels, the load-bearing
devcontainer.metadatafast path)
ac_find_container_by_label(list + filter + most-recently-started wins)
bridge.swift— refactored to use the shared helpers;ac_pingkeeps its pre-style-guide flat shape for PR-A stability.
ac_bridge.h— full header-comment style guide. Every exportdocuments the five contracts: ownership, cancellation, threading,
error encoding, blocking.
cgo shim (Go side)
shim.h/shim.c— three new function-pointer slots and wrappers,resolved via
dlsyminac_load. Partial-failure path resets allpointers and
dlcloses the handle.Runtime methods (
inspect_darwin_arm64.go)decodeEnvelope[T]for the canonical{ok, err, data}shapewith null-data tolerance for the find-by-label miss path.
containerSnapshot/imageInspectPayloadmirror the Apple JSONshapes we consume; only fields we need are listed.
snapshotToContainer/snapshotToDetails/mapStateprojectApple types into
runtime.Container/runtime.ContainerDetails.Created,FinishedAt,ExitCodeare zero — not in Apple'sContainerSnapshot; later PRs can surface them via additional XPCcalls if needed.
mapInspectErr/mapImageInspectErrtranslate Apple'snotFounderrors into
runtime.ContainerNotFoundError/runtime.ImageNotFoundErrorso the engine's typed-error contractshold across backends.
Tests
envelope_test.go— pure Go unit tests for success / failure /null-data / malformed-JSON /
mapState. No daemon, no cgo atruntime.
inspect_darwin_arm64_test.go— smoke tests against a live daemon:InspectContainerround-trip with a labeled containerInspectContainernot-found typed-errorInspectImagewith a warmed alpine imageInspectImagenot-found typed-errorFindContainerByLabelhit + missTest plan
make bridge && go test ./runtime/applecontainer/...— 11 testspass (5 envelope + 6 smoke)
go test ./...— full repo green; no regression in anyexisting package
GOOS=linux GOARCH=amd64 CGO_ENABLED=0 go build ./runtime/applecontainer/...— stub still compiles
go vet ./...cleanSummary by CodeRabbit
New Features
Documentation
Tests
Chores