Phase 1: internal package layout and its enforcement (ADR-0005) - #22
Merged
Conversation
Phase 1 opens with three separate records — package graph, Session IR, and machine contracts — so that revising the IR later does not force a supersede of the document carrying the public compatibility promise. This is the first. It merges with the code that implements it, not ahead of it. The graph is a star, and permissions follow from a package's class rather than from a row per package. A row per package would make widening the star an append that satisfies every check; a class means widening is an edit to a rule that reads like a decision. Because components may not import each other, the root necessarily carries the program's control flow, so it splits into one package that owns the object graph and one that owns flags, output, and the exit-code mapping. That mapping returns an int rather than an error: exit codes 70 to 73 report a verdict from a run that succeeded, and an error return cannot express them. Enforcement is depguard plus an architecture test. They are not redundant — depguard permits by default any file no rule matches, while the test is default-deny, so a package added without classifying itself does not build. The test also scans source text, because the failure this exists to prevent, comparing against a harness name in a shared package, imports nothing. What the guard cannot do is stated in the record rather than glossed. It checks topology and vocabulary; it cannot see a harness-shaped field on the neutral representation. Closing that needs a second, deliberately impoverished implementation in the conformance kit, and the record says so. Also adds the missing 0004 entry to the index. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ew64uotJqtvVDN3twMAVEQ
Run returns an int rather than an error: exit codes 70 to 73 report a verdict from a run that succeeded, and an error return cannot express them — a REGRESSED verdict would arrive as a nil error and be mapped to 0. The whole 0/2/70-73 mapping now sits inside the coverage gate, which measures internal/... only, and main is one line. The reservation comment moves with the constants rather than being dropped in the shuffle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ew64uotJqtvVDN3twMAVEQ
Default-deny: a package that appears without a classification row fails, which is the half depguard cannot do — depguard permits by default any file no rule matches, so a package added in a hurry passes it unremarked. Permitted imports follow from a package's class rather than a row of its own. A row per package would make widening the star an append that satisfies every check; a class means widening is an edit to allowedInternal. Third-party imports are checked against a deliberately empty allowlist, since the vocabulary scan catches a vendor SDK only when its module path happens to contain a listed word. The walker uses go/parser and so ignores //go:build, which is over-inclusive and fails safe; a fixture pins that behaviour so a later switch to go/packages fails rather than silently narrowing the guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ew64uotJqtvVDN3twMAVEQ
The failure this prevents is import-free — comparing against a harness name in a shared package imports nothing and satisfies any import-based rule — so the scan reads source text rather than the import graph. Matching is case-insensitive substring, not segment-wise. Segments were specified first and are wrong: "claudecode" is one segment and is not equal to "claude", so it would pass while "claude-code" fails, and "claudecode" is this project's own directory spelling for the reference adapter. Both spellings are now pinned by fixtures. Every file type under the scan roots is read, not only Go source: //go:embed is a comment and an embedded defaults file holding a harness transcript path would otherwise sit unwatched beside core. Go comments stay unscanned, since prose may name a harness and prose cannot branch. internal/arch exempts itself, without which the guard flags its own word list. The exemption count is asserted so a fourth one is a decision, not an append. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ew64uotJqtvVDN3twMAVEQ
A package-level var plus an adapter init is how an idiomatic Go registry gets built, in the style of database/sql. Every edge in it is legal and it names no vendor, and it makes the set of known harnesses mutable at startup — the coupling the star was drawn to prevent. Exported fields typed any or map[string]any are the same hole with a different shape. Also forbids branching in the adapter registration file. That file is the only one outside internal/adapter/** allowed to name a vendor, so it must be the one file not allowed to make decisions, or the exemption becomes somewhere to hide logic. A fixture pins that a real table still passes. These fire against packages that do not exist yet, which is the point: the check is in place before the code it constrains. What this does not close, stated in the ADR rather than implied away: a field named for one harness's concept uses no vendor word and crosses no illegal edge. That needs the stub harness in the conformance kit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ew64uotJqtvVDN3twMAVEQ
One source of truth rather than set equality between two hand-maintained copies that can both be wrong together. The test renders the block and asserts it verbatim, so a hand edit fails and prints what to paste. Parsing the config instead would have needed a YAML library — this module has no third-party dependencies, and the arch class's own "imports nothing" rule would then have been broken by the test's own import. list-mode is lax with a deny on the module prefix. The alternatives were tried and do not work: an empty allow list is a hard config error, strict mode denies the standard library so every package importing fmt fails, and lax with only an allow list enforces nothing. Adapter rules exclude test files from the broad rule, since overlapping depguard rules combine rather than override and the conformance-kit import would otherwise be rejected by the rule above it. Removes the comment explaining why depguard was deferred, as it promised. Verified both mechanisms reject independently: planting an import of internal/buildinfo into cmd/assayer fails golangci-lint and the arch test separately, each naming the class and the record. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ew64uotJqtvVDN3twMAVEQ
Five: the ADR count, which was already wrong at four; the claim that no working software exists, narrowed to no examining software, since the binary still only prints its version; the project-layout block, which omitted internal/cli and internal/arch; the sentence promising the package layout would be recorded later; and the design-decisions list, missing 0004 and 0005. Every relative link in the file was resolved against the tree before commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ew64uotJqtvVDN3twMAVEQ
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Opens Phase 1. The ADR ships in this PR rather than ahead of it, so the record
and the mechanisms it describes land together.
What this does
package mainintointernal/cli, inside thecoverage gate.
Runreturns anint, not anerror: exit codes 70–73 reporta verdict from a run that succeeded, so an
errorreturn would deliver aREGRESSEDverdict asniland map it to 0.internal/arch, a test-only package that reads the tree as source textand is default-deny — a package that appears without classifying itself fails.
asserted verbatim, so the linter config and the architecture test cannot drift.
Permitted imports follow from a package's class, not from a row of its own.
A row per package would make widening the star a thirty-second append that
satisfies every check; a class means widening is an edit to a rule that reads
like a decision.
Verified against the real tree, not only fixtures
Each was planted, observed, and reverted:
cmd/assayerimportinginternal/buildinfointernal/scratchwith no classification rowconst defaultHarness = "claude-code"ininternal/cli"claudecode"claudecodeis this project's own directory spellingmake verifyis green: coverage 100%, 0 lint issues, 0 vulnerabilities.What the guard cannot do, stated in the record rather than implied away
It checks topology and vocabulary. It cannot see a harness-shaped field on the
neutral representation —
SidechainUUIDuses no vendor word and crosses noillegal edge. The structural rules close the generic form (no package-level
state, no
init, noany-typed exported fields in the leaf, which togetheralso rule out a
database/sql-style registry mutable at startup). The namedform needs a second, deliberately impoverished implementation in the adapter
conformance kit, recorded as a Phase 1 deliverable.
Also corrects five statements in the README that this change makes false, and
adds the missing 0004 entry to the ADR index.