docs: publish architectural design records#66
Merged
Conversation
|
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 (3)
📝 WalkthroughWalkthroughAdds a new top-level design directory and README, links it from README.md, refines .gitignore, and introduces seven design specifications covering ResolvedConfig, Runtime, Apple runtime, Features, Compose orchestration, and Structured errors. ChangesArchitectural Design Documentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Six design notes that explain *why* the public API looks the way it
does. Useful for contributors and embedders; not part of the user-
facing surface. Lives under design/, indexed via design/README.md
and linked from the top-level README.
Published records:
- resolved-config.md — the ResolvedConfig type and parse-merge-
substitute pipeline.
- runtime.md — Runtime / Engine / Workspace boundaries.
- runtime-applecontainer.md — Apple's `container` backend over a
cgo + Swift bridge.
- compose-native.md — runtime-agnostic compose orchestrator.
- features.md — Dev Container Features pipeline.
- structured-errors.md — *devcontainer.Error surface.
What's NOT in this commit (kept under design/private/, gitignored):
- status.md — internal milestone tracker.
- upstream-apple-container.md — planning doc for upstream
contributions to apple/container.
- compose.md — superseded by compose-native.md (kept for context).
.gitignore narrowed from `design/` to `design/private/` so the
published records can be tracked while the in-flight planning
material stays local.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bilby91
force-pushed
the
design/publish-architecture
branch
from
19acad1 to
7067ada
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (16)
design/compose-native.md (4)
640-643: 💤 Low valueAdd language specifier to JSON error example.
The error message at lines 640-643 is valid JSON and should be marked as such for proper syntax highlighting.
📝 Proposed fix
- ``` + ```json Error: failed to bootstrap container ... (cause: "VZErrorDomain🤖 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 `@design/compose-native.md` around lines 640 - 643, The code fence containing the JSON error example ("Error: failed to bootstrap container ... (cause: \"VZErrorDomain Code=2 'The storage device attachment is invalid.'\")") is missing a language specifier; update that triple-backtick fence to include "json" (i.e., change ``` to ```json) so the JSON-like error is highlighted correctly in the compose-native.md example block.
27-50: 💤 Low valueConsider adding language specifier to ASCII diagram code block.
The ASCII diagram is in a fenced code block without a language specifier. While not critical for ASCII art, adding
textorplaintextwould satisfy linting tools and improve consistency.📝 Proposed fix
-``` +```text ┌──────────────────────────────────────────────────────────────────┐ │ Engine (devcontainer pkg) │🤖 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 `@design/compose-native.md` around lines 27 - 50, Update the fenced ASCII diagram code block (the block that starts with the box drawing characters and the compose pkg / runtime diagram) to include a language specifier like "text" or "plaintext" after the opening triple backticks so linters recognize it as plain text; e.g., change the opening fence from ``` to ```text and keep the diagram content unchanged.
291-339: 💤 Low valueAdd language specifiers to algorithm pseudocode blocks.
The Up and Down algorithm descriptions (lines 291-339 and 343-358) use fenced code blocks without language specifiers. Consider using
textorpseudocodeto satisfy linting and improve rendering.Also applies to: 343-358
🤖 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 `@design/compose-native.md` around lines 291 - 339, Update the fenced code blocks that contain the Up and Down algorithm pseudocode so they include a language specifier (e.g., replace ``` with ```text or ```pseudocode); specifically target the block beginning with "Input: Plan {" (the Up algorithm pseudocode) and the subsequent Down algorithm block so the linter and renderer recognize them as pseudocode/text.
141-148: 💤 Low valueAdd language specifiers to label example code blocks.
The label examples at lines 141-148 and 162-166 are in fenced code blocks without language specifiers. These appear to be configuration/label formats.
📝 Proposed fix
For the container labels block:
-``` +```yaml com.docker.compose.project = <projectName> # interop labelFor the network/volume labels block:
-``` +```yaml com.docker.compose.project = <projectName>Also applies to: 162-166
🤖 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 `@design/compose-native.md` around lines 141 - 148, The fenced code blocks containing the example container labels and the network/volume labels should include a language specifier for syntax highlighting; update both example blocks shown in the diff (the "container labels" example with com.docker.compose.* and dev.containers.* lines and the "network/volume labels" example) to use ```yaml as the opening fence instead of a plain fence so readers get proper YAML highlighting.design/structured-errors.md (3)
187-193: ⚡ Quick winDocument that
With()mutates the receiver.The builder pattern here is clean for the common usage (chained immediately after
New()), but the method should document that it mutates the receiver in place:// With adds a Context entry and returns the receiver for chaining. // It mutates the Error in place; the returned value points to the same instance. func (e *Error) With(key string, val any) *Error { ... }This prevents surprises if someone tries to fork an Error by calling
With()multiple times on the same base instance.🤖 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 `@design/structured-errors.md` around lines 187 - 193, Add a Go doc comment to the With method on type Error that clearly states it mutates the receiver and returns the same instance for chaining; update the comment above func (e *Error) With(key string, val any) *Error to something like: "With adds a Context entry and returns the receiver for chaining. It mutates the Error in place; the returned value points to the same instance." so callers know not to expect a copy when reusing a base Error.
153-153: 💤 Low valueSpecify the exact
Error()format string.The comment indicates the format will be
"<code>: <reason>", but consider documenting the exact implementation to ensure consistency:func (e *Error) Error() string { return string(e.Code) + ": " + e.Reason }This makes it clear whether there are spaces around the colon, how Code is converted to string, and whether Cause/Context appear in the output.
🤖 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 `@design/structured-errors.md` at line 153, Document and implement Error() for the Error type to return the exact string format "code: reason" (use string(e.Code) + ": " + e.Reason semantics) and explicitly state that neither Cause nor Context are included in Error() output; update the comment for the Error.Error() method to show that format and reference the Error type and its fields Code, Reason, Cause, and Context so readers know how Code is converted and what is omitted.
568-568: 💤 Low valueOptional: Hyphenate compound modifier.
For grammatical consistency, "Runtime-typed errors" should use a hyphen when the compound modifier precedes the noun:
-7. **Existing typed errors stay.** `config/errors.go` and - `runtime/errors.go` types continue to exist as Cause carriers. - Runtime typed errors gain `Stderr()` methods to satisfy + Runtime-typed errors gain `Stderr()` methods to satisfyThis is a minor stylistic point in a design document.
🤖 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 `@design/structured-errors.md` at line 568, Change the phrase "Runtime typed errors" to the hyphenated "Runtime-typed errors" in the sentence "Runtime typed errors gain `Stderr()` methods to satisfy" so the compound modifier is grammatically consistent; locate that exact phrase in the document and update it to "Runtime-typed errors".design/features.md (9)
302-306: ⚡ Quick winClarify mirror URL deduplication mechanism.
The spec states "content-addressed dedupes across mirror URLs" which is excellent, but the mechanism could be clearer. Currently:
index.jsonmaps ref → digest- Cache entries are keyed by digest
So two refs (e.g.,
ghcr.io/org/feat:1anddocker.io/org/feat:1) pointing to the same content would:
- Both appear in index.json with different ref keys
- Both map to the same digest
- Share the same
oci/sha256-<digest>/directoryPlease add a sentence confirming this is the intended deduplication behavior, as it's an important cache efficiency property.
🤖 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 `@design/features.md` around lines 302 - 306, The spec should explicitly state that deduplication across mirror URLs is achieved by mapping each mutable or mirror ref in index.json to a single content digest and using that digest as the cache key so multiple refs (e.g., ghcr.io/org/feat:1 and docker.io/org/feat:1) that resolve to the same digest will each have separate entries in index.json but point to and share the same on-disk cache directory (oci/sha256-<digest>/), ensuring a single stored copy per content digest.
285-300: 💤 Low valueAdd language identifier to code block.
The cache layout block is missing a language specifier.
📝 Suggested fix
-``` +```text features/ oci/🤖 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 `@design/features.md` around lines 285 - 300, The fenced code block showing the cache layout (the triple-backtick block containing "features/ oci/ sha256-<digest>/ ... index.json") is missing a language specifier; update the opening fence from ``` to ```text so the block is treated as plain text (leave the content and closing fence unchanged).
212-224: 💤 Low valueAdd language identifier to code block.
The build context layout block is missing a language specifier.
📝 Suggested fix
-``` +```text <engine-managed tmp dir>/ Dockerfile # the file above🤖 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 `@design/features.md` around lines 212 - 224, The code block showing the build context layout (starting with "<engine-managed tmp dir>/" and lines like "Dockerfile" and "build-context/") lacks a language identifier; update the opening fence from ``` to ```text (or another appropriate language) so the snippet is rendered correctly and syntax/highlighting is consistent.
19-58: 💤 Low valueAdd language identifier to code block.
The ASCII diagram code block is missing a language specifier.
📝 Suggested fix
-``` +```text devcontainer.json (features map) │🤖 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 `@design/features.md` around lines 19 - 58, The fenced ASCII diagram block is missing a language tag; update the code fence that wraps the diagram (the triple backticks before "devcontainer.json (features map)") to include a language identifier like text (i.e. change ``` to ```text) so the ASCII art renders correctly and avoids markdown highlighting issues for the diagram shown in the feature.Resolve / feature.Order / feature.GenerateDockerfile flow.
201-201: ⚡ Quick winSpecify where
FEATURE_REFvariable is defined.The trap on line 201 references
FEATURE_REFin the error message, but the script doesn't show where this variable is defined. Please clarify:
- Is it injected by the generator as a literal string substitution?
- Is it set in an environment file?
- Should the example show this variable definition?
🤖 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 `@design/features.md` at line 201, The trap message references FEATURE_REF but the document doesn't define it; update the example around the trap (the trap 'rc=$?; ... FEATURE_REF ...' line) to either (a) state explicitly that FEATURE_REF is a generator-substituted placeholder, (b) show a preceding example variable definition (e.g., export FEATURE_REF="your-feature-name") or (c) note it is expected from an environment file—choose one approach and add one clear sentence and a sample definition so readers know where FEATURE_REF comes from.
352-352: 💤 Low valueRemove space in code span element.
There's an extra space inside the code span:
>=should be>=.📝 Suggested fix
- version `>= ` requested via semver. Opt-in: + version `>=` requested via semver. Opt-in:🤖 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 `@design/features.md` at line 352, The code span contains an extra trailing space: change the inline code element text from "`>= `" to "`>=`" so the semver comparator has no trailing space; locate the occurrence of the code span "`>= `" in the features.md content and remove the space inside the backticks (replace "`>= `" with "`>=`").
148-149: ⚡ Quick winClarify
installsAfterbehavior for unlisted features.The spec states
installsAfteris a soft hint "if both listed," but doesn't specify what happens when a feature declaresinstallsAfterfor a feature that isn't in the user's configuration. Is this:
- Silently ignored (safest)?
- Treated as a warning?
- An error?
Please add a sentence clarifying this edge case.
🤖 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 `@design/features.md` around lines 148 - 149, Add a single clarifying sentence to the spec near the `installsAfter` description: state that `installsAfter` is only effective when both features are present in the user's configuration, and if a feature names an `installsAfter` target that is not listed it is ignored (does not block installation); optionally note that implementations may emit a non-fatal warning for missing targets. Ensure the sentence references `installsAfter` and contrasts it with `dependsOn` for clarity.
274-278: ⚡ Quick winClarify "resolved digest" storage for HTTPS features.
The document states "we record the resolved digest on the metadata entry" for version matching. This is clear for OCI features (which have a manifest digest), but HTTPS features use "sha256 of body" as their cache key (§2, line 67).
Please clarify:
- Is the HTTPS sha256 stored in the same
ResolvedReffield as OCI digests?- Does
StrictFeatureVersionMatchbyte-level comparison work identically for both?- Should the metadata entry distinguish between OCI digest and HTTPS content hash?
🤖 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 `@design/features.md` around lines 274 - 278, Update the design text to explicitly state that for HTTPS features the sha256(body) is stored in the same ResolvedRef field used for OCI manifest digests, that StrictFeatureVersionMatch performs identical byte-level identity comparison for both OCI manifest digests and HTTPS content hashes, and to add a short note about distinguishing the two in metadata (either by a source/type prefix on ResolvedRef, e.g. "oci:sha256:..." vs "https:sha256:...", or by adding a small discriminator field) so downstream tooling can tell manifest digest vs content hash; reference ResolvedRef, StrictFeatureVersionMatch and the metadata entry when making this clarification.
239-239: ⚡ Quick winSpecify merge semantics for maps/objects.
The merge semantics state "later overrides earlier" for scalars and "concat + dedupe" for lists, but don't address maps/objects (e.g.,
containerEnv,customizations). Are these:
- Shallow overridden (entire map replaced)?
- Deep merged (keys merged recursively)?
Please add a sentence clarifying nested object behavior, as this is common in devcontainer.json (e.g.,
containerEnv: { KEY: value }layers).🤖 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 `@design/features.md` at line 239, The doc omits merge semantics for maps/objects; add a clear sentence stating whether object/map fields are shallow-overridden or deep-merged (and how conflicts are resolved), and give the devcontainer-specific examples `containerEnv` and `customizations` to show expected behavior (e.g., "maps are deep-merged recursively by key, with later values overriding scalar leaves" or "maps are shallow-replaced: the later map completely replaces the earlier"). Update the paragraph that currently documents scalars and lists to include this single clarifying sentence and ensure it mentions recursive key-merge behavior and conflict resolution rules.
🤖 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 `@design/compose-native.md`:
- Line 92: Update the §2.1 in-scope table entry for
services.<name>.network_mode: service:<other> to reflect the Apple backend
limitation mentioned in probe R4 — change the description to indicate this is
"docker backend only" (or "Docker backend only; not supported on Apple backend")
so the table matches the probe text in §2.1 and R4; ensure any other references
in §2.1 that mention this feature are consistent with the updated phrasing.
In `@design/features.md`:
- Around line 362-369: Add a clarifying sentence that disambiguates the two
resolution layers: state that config-level resolution (e.g.,
devcontainer.Resolve) only parses the features map and intentionally leaves
per-feature fields like Metadata, Dir, ResolvedRef, and AlreadyInstalled unset,
while feature-level resolution (feature.Store.Resolve) performs the fetch and
populates those fields; also state that Engine.Up is responsible for invoking
feature.Store.Resolve for features not already installed. Reference the exact
names devcontainer.Resolve, feature.Store.Resolve, and Engine.Up in the sentence
to make the distinction clear.
- Around line 81-85: Update the "HTTPS mechanics" section to explicitly specify
the security and network policies for feature downloads: state that TLS
certificates must be validated against system/root CAs and explain whether/when
custom CA bundles are allowed, define redirect handling (follow or reject and a
max redirect depth), set a response body size limit and behavior on overflow,
document request timeouts (connect and overall) and retry/backoff policy, and
enumerate error handling semantics for network failures (which errors are
retriable vs fatal) and how EngineOptions.FeatureDownloadHeaders is applied;
include these details alongside the existing notes about filename validation,
sha256 caching, and streaming/extraction so implementers have a complete,
consistent spec.
- Around line 136-137: The doc is ambiguous about when enum/proposal validation
runs; update the text to state that enum validation occurs after feature
metadata is available (i.e., during feature.Store.Resolve() once
devcontainer-feature.json has been fetched and merged into Metadata), and that
Engine.Up will perform validation as a fallback for any features whose metadata
wasn't resolved earlier (for example when features are injected or mutated
late). Also explicitly document the pre-baked-image hot path behavior: if
features are not fetched because a pre-baked image is used, skip remote metadata
validation and either validate against embedded Metadata (if present) or treat
enums as unvalidated/default to safe behavior; reference
feature.Store.Resolve(), Resolve, Metadata, Engine.Up and the pre-baked-image
hot path in the updated prose so readers know where each validation step happens
and what to expect.
- Around line 175-178: The _REMOTE_USER_HOME value is being computed with whoami
while USER is set to root, so it captures root's home; change the logic that
writes to /tmp/dc-features/builtin.env to look up the home for the intended
target user (use the build ARG _DEV_CONTAINERS_IMAGE_USER or a default) instead
of whoami—either move the ARG _DEV_CONTAINERS_IMAGE_USER earlier so it’s
available in this RUN or defer writing _REMOTE_USER_HOME until after that ARG is
defined, and use getent passwd ${_DEV_CONTAINERS_IMAGE_USER:-root} to populate
_REMOTE_USER_HOME.
In `@design/resolved-config.md`:
- Around line 15-24: Add language identifiers to the two fenced code blocks that
currently use plain backticks so markdownlint stops failing: change the opening
fences for the ASCII diagram block (the box/arrow diagram around "files -> raw
-> merged -> resolved") and the one containing the expression "devcontainerId =
base32(sha256(localWorkspaceFolder + "\x00" + configPath))[:16]" to include a
language tag (e.g., use ```text) so the blocks are explicitly marked; update the
opening triple-backtick lines for those blocks in the document where the diagram
and the devcontainerId expression appear.
In `@design/runtime-applecontainer.md`:
- Around line 205-221: The docs list an InspectImage mapping for runtime.Runtime
but the declared Runtime interface in design/runtime.md does not include
InspectImage; update the contract so they match by either adding InspectImage to
the Runtime interface (and document expected signature/behavior consistent with
the "image service inspect" mapping and labels round-trip note) or remove the
InspectImage entry from design/runtime-applecontainer.md and any Apple-specific
mapping; ensure references to runtime.Runtime, InspectImage, and the image
service/labels behavior are updated accordingly.
- Around line 19-51: The fenced ASCII diagram block (starting with "Engine
(devcontainer pkg)" / the large box diagram) and the other unlabeled fenced
block later (the block covering the section that begins with
"container-apiserver (Apple's daemon; launchd LaunchAgent)" / lines ~258-267)
are missing a language identifier; update both triple-fenced blocks to use
```text so markdownlint passes and tooling renders them as plain text, leaving
the contents unchanged.
- Around line 161-163: The Package.swift manifest currently uses a wildcard
string with the exact requirement (e.g., exact: "0.12.x") which SwiftPM rejects;
update the dependency declaration in Package.swift to use a fully qualified
semantic version (for example change exact: "0.12.x" to exact: "0.12.3") so the
exact: requirement is a concrete version; ensure any other occurrences of exact:
with an x wildcard are replaced with the specific patch version and keep bump
policy (separate PRs + parity integration) unchanged.
In `@design/runtime.md`:
- Around line 17-42: The fenced ASCII architecture diagram in runtime.md lacks a
language token; update the opening code fence to include a language (e.g.,
change ``` to ```text) so the diagram block is recognized as plain text and
resolves markdownlint MD040 and editor rendering issues; locate the ASCII block
that starts with the "Caller (your application, CLI, tests, ...)" diagram and
add the language token to its opening fence.
---
Nitpick comments:
In `@design/compose-native.md`:
- Around line 640-643: The code fence containing the JSON error example ("Error:
failed to bootstrap container ... (cause: \"VZErrorDomain Code=2 'The storage
device attachment is invalid.'\")") is missing a language specifier; update that
triple-backtick fence to include "json" (i.e., change ``` to ```json) so the
JSON-like error is highlighted correctly in the compose-native.md example block.
- Around line 27-50: Update the fenced ASCII diagram code block (the block that
starts with the box drawing characters and the compose pkg / runtime diagram) to
include a language specifier like "text" or "plaintext" after the opening triple
backticks so linters recognize it as plain text; e.g., change the opening fence
from ``` to ```text and keep the diagram content unchanged.
- Around line 291-339: Update the fenced code blocks that contain the Up and
Down algorithm pseudocode so they include a language specifier (e.g., replace
``` with ```text or ```pseudocode); specifically target the block beginning with
"Input: Plan {" (the Up algorithm pseudocode) and the subsequent Down algorithm
block so the linter and renderer recognize them as pseudocode/text.
- Around line 141-148: The fenced code blocks containing the example container
labels and the network/volume labels should include a language specifier for
syntax highlighting; update both example blocks shown in the diff (the
"container labels" example with com.docker.compose.* and dev.containers.* lines
and the "network/volume labels" example) to use ```yaml as the opening fence
instead of a plain fence so readers get proper YAML highlighting.
In `@design/features.md`:
- Around line 302-306: The spec should explicitly state that deduplication
across mirror URLs is achieved by mapping each mutable or mirror ref in
index.json to a single content digest and using that digest as the cache key so
multiple refs (e.g., ghcr.io/org/feat:1 and docker.io/org/feat:1) that resolve
to the same digest will each have separate entries in index.json but point to
and share the same on-disk cache directory (oci/sha256-<digest>/), ensuring a
single stored copy per content digest.
- Around line 285-300: The fenced code block showing the cache layout (the
triple-backtick block containing "features/ oci/ sha256-<digest>/ ...
index.json") is missing a language specifier; update the opening fence from ```
to ```text so the block is treated as plain text (leave the content and closing
fence unchanged).
- Around line 212-224: The code block showing the build context layout (starting
with "<engine-managed tmp dir>/" and lines like "Dockerfile" and
"build-context/") lacks a language identifier; update the opening fence from ```
to ```text (or another appropriate language) so the snippet is rendered
correctly and syntax/highlighting is consistent.
- Around line 19-58: The fenced ASCII diagram block is missing a language tag;
update the code fence that wraps the diagram (the triple backticks before
"devcontainer.json (features map)") to include a language identifier like text
(i.e. change ``` to ```text) so the ASCII art renders correctly and avoids
markdown highlighting issues for the diagram shown in the feature.Resolve /
feature.Order / feature.GenerateDockerfile flow.
- Line 201: The trap message references FEATURE_REF but the document doesn't
define it; update the example around the trap (the trap 'rc=$?; ... FEATURE_REF
...' line) to either (a) state explicitly that FEATURE_REF is a
generator-substituted placeholder, (b) show a preceding example variable
definition (e.g., export FEATURE_REF="your-feature-name") or (c) note it is
expected from an environment file—choose one approach and add one clear sentence
and a sample definition so readers know where FEATURE_REF comes from.
- Line 352: The code span contains an extra trailing space: change the inline
code element text from "`>= `" to "`>=`" so the semver comparator has no
trailing space; locate the occurrence of the code span "`>= `" in the
features.md content and remove the space inside the backticks (replace "`>= `"
with "`>=`").
- Around line 148-149: Add a single clarifying sentence to the spec near the
`installsAfter` description: state that `installsAfter` is only effective when
both features are present in the user's configuration, and if a feature names an
`installsAfter` target that is not listed it is ignored (does not block
installation); optionally note that implementations may emit a non-fatal warning
for missing targets. Ensure the sentence references `installsAfter` and
contrasts it with `dependsOn` for clarity.
- Around line 274-278: Update the design text to explicitly state that for HTTPS
features the sha256(body) is stored in the same ResolvedRef field used for OCI
manifest digests, that StrictFeatureVersionMatch performs identical byte-level
identity comparison for both OCI manifest digests and HTTPS content hashes, and
to add a short note about distinguishing the two in metadata (either by a
source/type prefix on ResolvedRef, e.g. "oci:sha256:..." vs "https:sha256:...",
or by adding a small discriminator field) so downstream tooling can tell
manifest digest vs content hash; reference ResolvedRef,
StrictFeatureVersionMatch and the metadata entry when making this clarification.
- Line 239: The doc omits merge semantics for maps/objects; add a clear sentence
stating whether object/map fields are shallow-overridden or deep-merged (and how
conflicts are resolved), and give the devcontainer-specific examples
`containerEnv` and `customizations` to show expected behavior (e.g., "maps are
deep-merged recursively by key, with later values overriding scalar leaves" or
"maps are shallow-replaced: the later map completely replaces the earlier").
Update the paragraph that currently documents scalars and lists to include this
single clarifying sentence and ensure it mentions recursive key-merge behavior
and conflict resolution rules.
In `@design/structured-errors.md`:
- Around line 187-193: Add a Go doc comment to the With method on type Error
that clearly states it mutates the receiver and returns the same instance for
chaining; update the comment above func (e *Error) With(key string, val any)
*Error to something like: "With adds a Context entry and returns the receiver
for chaining. It mutates the Error in place; the returned value points to the
same instance." so callers know not to expect a copy when reusing a base Error.
- Line 153: Document and implement Error() for the Error type to return the
exact string format "code: reason" (use string(e.Code) + ": " + e.Reason
semantics) and explicitly state that neither Cause nor Context are included in
Error() output; update the comment for the Error.Error() method to show that
format and reference the Error type and its fields Code, Reason, Cause, and
Context so readers know how Code is converted and what is omitted.
- Line 568: Change the phrase "Runtime typed errors" to the hyphenated
"Runtime-typed errors" in the sentence "Runtime typed errors gain `Stderr()`
methods to satisfy" so the compound modifier is grammatically consistent; locate
that exact phrase in the document and update it to "Runtime-typed errors".
🪄 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: 95349023-cbcd-4ae7-9e87-51bb4bde8f32
📒 Files selected for processing (9)
.gitignoreREADME.mddesign/README.mddesign/compose-native.mddesign/features.mddesign/resolved-config.mddesign/runtime-applecontainer.mddesign/runtime.mddesign/structured-errors.md
- features.md: spec HTTPS security posture (TLS, redirects, size cap, timeouts, error types); clarify enum validation runs at fetch time in feature.Store.Resolve with an Engine.Up backstop; fix _REMOTE_USER_HOME to use _DEV_CONTAINERS_IMAGE_USER via getent instead of $(whoami) under USER root; disambiguate config-level vs feature-level Resolve in decision #5. - runtime.md: add InspectImage to the documented Runtime interface to match runtime/runtime.go:120 and the apple-container mapping. - runtime-applecontainer.md: pin SwiftPM dep to exact: "0.12.3" (SwiftPM rejects wildcards in exact:). - compose-native.md: mark network_mode: service:<other> as Docker-only. - Add text language tag to unlabeled fenced blocks across four files (markdownlint MD040). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
design/compose-native.md (1)
27-27: ⚡ Quick winAdd language identifiers to fenced code blocks to satisfy markdown lint.
These fences are still unlabeled (
MD040). Please tag them explicitly (e.g.,textfor diagrams/pseudocode,jsonwhere applicable) to keep docs lint-clean.Also applies to: 141-141, 162-162, 291-291, 343-343, 640-640
🤖 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 `@design/compose-native.md` at line 27, Update the unlabeled fenced code blocks in design/compose-native.md to include explicit language identifiers to satisfy MD040; locate the backtick fences referenced (lines around the reported occurrences) and change ``` to ```text for diagrams/pseudocode or to the appropriate language such as ```json where the block contains JSON so that each fenced block is labeled (apply to the occurrences noted in the review).
🤖 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 `@design/compose-native.md`:
- Around line 931-936: Sections §12 and §13 contradict §11.1 and §14.10–§14.12
about probe 3/4; update text so probe 3 is documented as having a viable
/etc/hosts workaround and probe 4 is marked "green with scoped limitation"
rather than a blocker that forces Docker-only compose-source, and remove or
reword any language that asserts a hard "no" on Apple-container service-name DNS
resolution; specifically edit the paragraphs that reference "probe 3/4" and
"compose-source is Docker-only" to match the resolved state in §11.1 and
§14.10–§14.12 and ensure mentions of "Apple-container service-name DNS
resolution" and the M7 roadmap reflect the scoped limitation and workaround.
In `@design/features.md`:
- Line 19: Three unlabeled fenced code blocks in design/features.md trigger
markdownlint MD040; update each of the three offending code fences (the blocks
showing "devcontainer.json (features map)...", the "<engine-managed tmp
dir>/..." block, and the "features/ oci/ sha256-<digest>/..." block) by adding a
language tag (e.g., ```text) to the opening fence so the blocks become labeled
and satisfy MD040.
- Line 370: The inline code span currently contains a trailing space ("`>= `")
which violates markdownlint MD038; update the markdown where the text shows
version `>= ` to remove the internal trailing space so it reads `>=` (i.e.,
replace the code span symbol `>= ` with `>=`) to fix the spacing issue.
In `@design/runtime-applecontainer.md`:
- Around line 10-13: The document contains a contradictory scope statement: the
top-level claim that this backend "inherits the Runtime interface verbatim" and
"no engine-side changes are required" conflicts with §13.8 which requires wiring
Engine.Up and possibly extending Runtime with a capability. Update the doc to
present a single consistent contract: either state that Runtime is unchanged and
show exactly how Engine.Up will be wired without API changes, or explicitly
state that Runtime will gain the new capability and list the required
engine-side changes (e.g., adding Engine.Up short-circuit wiring and the Runtime
capability addition). Reference the symbols Runtime and Engine.Up in the
reconciled statement and add a short bullet or paragraph describing which of the
two approaches is chosen and the minimal concrete change set required.
---
Nitpick comments:
In `@design/compose-native.md`:
- Line 27: Update the unlabeled fenced code blocks in design/compose-native.md
to include explicit language identifiers to satisfy MD040; locate the backtick
fences referenced (lines around the reported occurrences) and change ``` to
```text for diagrams/pseudocode or to the appropriate language such as ```json
where the block contains JSON so that each fenced block is labeled (apply to the
occurrences noted in the review).
🪄 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: fe15b46c-7d25-4b3b-8b75-f96f92732e7f
📒 Files selected for processing (5)
design/compose-native.mddesign/features.mddesign/resolved-config.mddesign/runtime-applecontainer.mddesign/runtime.md
✅ Files skipped from review due to trivial changes (1)
- design/resolved-config.md
🚧 Files skipped from review as they are similar to previous changes (1)
- design/runtime.md
- features.md: add text language tag to three more unlabeled fenced blocks (pipeline diagram, build context layout, cache layout) for markdownlint MD040; fix `>= ` code-span trailing space (MD038). - runtime-applecontainer.md: reconcile scope statement — backend implements existing Runtime interface but does require one engine-side adjustment (updateRemoteUserUID short-circuit via a capability flag, per §13.8). - compose-native.md: §12 open-question #1 and §13 out-of-scope #6 rewritten to reflect the resolved state in §14.10–§14.12 (probe 3 workaround viable, probe 4 scoped limitation) instead of stale "Apple compose may be impossible" framing. - Remove all PRD references from published design docs (compose-native, runtime). The PRD is internal-only material and the referenced points are now stated directly or via design/structured-errors.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@design/compose-native.md`:
- Around line 28-51: Several fenced code blocks in the compose-native design doc
are missing language identifiers; update each triple-backtick fence that
contains diagrams or pseudocode to use the `text` language tag (e.g., the ASCII
diagram beginning with "Engine (devcontainer pkg)", the blocks containing
"com.docker.compose.project = <projectName>", the "Input: Plan" and "Input:
DownPlan { ProjectName string, RemoveImages bool, RemoveVolumes bool }"
examples, and the error example starting with "Error: failed to bootstrap
container ... (cause: \"VZErrorDomain Code=2 ...\")"). For each of those fenced
blocks (and the additional ranges the reviewer called out), add ```text at the
start and ``` at the end so markdownlint MD040 warnings are resolved.
In `@design/features.md`:
- Line 90: The sentence about redirect header behavior is incorrect: update the
text to state that Go's net/http client forwards custom headers (like
FeatureDownloadHeaders) across cross-host redirects by default, so the spec must
require explicit rules to prevent credential leakage (either deny forwarding on
cross-host redirects or apply an allowlist tied to the original Host), and
clarify that http:// targets must still be rejected on redirect; reference the
header name FeatureDownloadHeaders and mention Go's net/http client policy for
context.
In `@design/runtime-applecontainer.md`:
- Around line 14-15: Update the doc to reconcile the inconsistency about the
short-circuit mechanism: either change the intro line that currently asserts
"driven by a small capability flag on `Runtime`, not a new method" to mark it as
"TBD" (matching §13.8), or edit §13.8 (the paragraph mentioning "TBD in PR-C —
either a capability flag on the `Runtime` interface or a type assertion") to
remove the options and explicitly confirm the capability flag on `Runtime`;
ensure both the intro and §13.8 consistently state the same chosen approach and
reference the `Runtime` symbol consistently.
🪄 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: 388a387f-93cd-4cf5-a6eb-64b6bcfc2bdc
📒 Files selected for processing (4)
design/compose-native.mddesign/features.mddesign/runtime-applecontainer.mddesign/runtime.md
🚧 Files skipped from review as they are similar to previous changes (1)
- design/runtime.md
- features.md: fix HTTPS redirect-header description. Go's net/http strips Authorization/Cookie/Proxy-Authorization on cross-host redirects but forwards arbitrary custom headers unchanged, so the client must install a CheckRedirect that drops FeatureDownloadHeaders on cross-host hops (with an opt-in allowlist for callers that need them). - runtime-applecontainer.md: §13.8 reconciled with the intro — capability flag on Runtime is the chosen mechanism, not a marker-interface type assertion. - compose-native.md: add text language tag to remaining unlabeled fenced blocks (layering diagram, label tables, Plan/DownPlan pseudocode, VZ error sample) — markdownlint MD040. 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
Publishes the architectural design notes that have been internal-only since the repo started. They explain the choices behind the public API for contributors and embedders — not user-facing docs.
Six published records, indexed via
design/README.mdand linked from the top-level README:resolved-config.md— the central data typeruntime.md— Runtime / Engine / Workspace layeringruntime-applecontainer.md— second backend over a Swift bridgecompose-native.md— runtime-agnostic compose orchestratorfeatures.md— Dev Container Features pipelinestructured-errors.md—*devcontainer.ErrorsurfaceInternal planning material (milestone trackers, draft upstream-contribution plans, the superseded shell-out compose design) is kept under
design/private/, which the new.gitignorerule excludes from the repo.What's been scrubbed from the published docs
consumer,your application).§13) was replaced with the underlying point.What remains: some shipping-plan vocabulary (milestone labels, "PR-X" references) is intact where it doesn't impede understanding. These are design records, not living documentation — the conversational language is part of how they read. The status legend in
design/README.mddescribes the convention.Test plan
.gitignorecorrectly excludesdesign/private/while tracking the rest ofdesign/.grep -nE "\bDAP\b" design/*.mdreturns no matches.design/README.mdresolve.## Designsection links intodesign/README.md.🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Chores