Add docs, ADRs, domain glossary, architecture reference, and actionable error messages - #23
Conversation
de7e0e5 to
9ba4719
Compare
2e333e6 to
81d5cd8
Compare
9ba4719 to
1715919
Compare
81d5cd8 to
2b11f85
Compare
1715919 to
c4545bb
Compare
2b11f85 to
921cff5
Compare
006651f to
ed1aab9
Compare
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR prepares Packref v1 for release by adding a clear documentation set (domain glossary, architecture reference, ADRs), tightening CLI help text, and making core error messages more actionable, with tests to keep the CLI help surface stable.
Changes:
- Added project docs:
CONTEXT.mdglossary,docs/architecture.md, and ADR index + three ADRs. - Refined CLI command descriptions and extended typed error messages with recovery guidance.
- Added CLI help tests and minor tooling updates (
typecheckscript, parallel test runner, ANSI-stripping for test output).
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/workspace/integration.ts | Updates Packref guidance text embedded into integrations/agent guidance. |
| src/lib/core/errors.ts | Adds actionable recovery instructions to core typed error messages. |
| src/commands/sync.ts | Refines sync command description to reflect dependency-tracked behavior. |
| src/commands/remove.ts | Refines remove command description. |
| src/commands/prune.ts | Refines prune command description. |
| src/commands/list.ts | Refines list command description. |
| src/commands/init.ts | Refines init command description. |
| src/commands/clean.ts | Refines clean command description. |
| src/commands/add.ts | Refines add command description. |
| src/commands/tests/init.test.ts | Updates init guidance assertion to match new wording. |
| src/commands/tests/helpers.ts | Normalizes CLI output for tests by stripping VT control sequences. |
| src/commands/tests/help.test.ts | Adds coverage ensuring v1 commands and optional args/flags appear in --help. |
| README.md | Rewrites README with installation, command reference, storage model, and boundaries. |
| package.json | Adds typecheck script; runs tests with bun test --parallel. |
| docs/architecture.md | Adds an architecture overview (module seams, flows, storage model, boundaries). |
| docs/adr/README.md | Adds ADR index. |
| docs/adr/0003-prefer-repository-source-with-bounded-tarball-fallback.md | Documents bounded tarball fallback decision. |
| docs/adr/0002-commit-lock-metadata-and-materialize-source-locally.md | Documents lockfile-vs-materialized-source split decision. |
| docs/adr/0001-use-packref-for-dependency-source-inspection.md | Documents Packref as the dependency source inspection policy. |
| CONTEXT.md | Introduces authoritative domain glossary for Packref terms. |
| AGENTS.md | Updates contributor/agent guidance links and changeset guidance. |
| .plans/v2-features.md | Removes items promoted/clarified as no longer v2 candidates. |
| .plans/README.md | Adds orientation for plans directory as history vs current guidance. |
| .plans/implementation-strategy.md | Adds “historical reference” notice and corrects sync behavior note. |
| .plans/09-cli-polish-docs-release.md | Marks Plan 09 as implemented and updates scope language. |
| .changeset/first-minor-release.md | Adds first release changeset describing Packref capabilities. |
Suppressed comments (1)
src/commands/tests/help.test.ts:23
- Same brittleness here: asserting exact spacing makes the test fail on harmless formatting changes. Prefer regex that anchors on the command name followed by whitespace and the description.
expect(result.output).toContain("list List package source references")
expect(result.output).toContain("prune Remove global store entries")
expect(result.output).toContain("remove Remove package source references")
expect(result.output).toContain("sync Update dependency-tracked references")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Implemented. Command help and errors are actionable, the complete v1 workflow is documented, and | ||
| the executable package contents have been verified. The package remains at the pre-release `0.0.0` | ||
| baseline, so no changeset was created. |
| - `packref add [package]` — add source references from manifest dependencies or by explicit name/version | ||
| - `packref remove [package]` — remove package references | ||
| - `packref install` — materialize every reference recorded in the committed lockfile | ||
| - `packref sync` — update dependency-tracked lock entries to match current `package.json` versions |
| expect(result.output).toContain("add Add a package source reference") | ||
| expect(result.output).toContain("clean Clear current-project references") | ||
| expect(result.output).toContain("init Initialize Packref files") | ||
| expect(result.output).toContain("install Install Packref source references") |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed1aab949f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }> { | ||
| override get message() { | ||
| return `Failed to resolve installed dependency versions for \`${this.path}\`.` | ||
| return `Failed to resolve installed dependency versions for \`${this.path}\`. Run your package manager's install command, then retry.` |
There was a problem hiding this comment.
When package-manager detection or checking/reading a lockfile fails because of permissions or another filesystem error, PackageManagerResolver wraps that failure in ManifestResolutionError (for example, javascript.ts lines 311–318 and 397–404). Running the package manager's install command will not resolve those failures and may unnecessarily modify project state; tailor the recovery advice to the underlying cause or use guidance that also covers access/read errors.
Useful? React with 👍 / 👎.
| }> { | ||
| override get message() { | ||
| return `Failed to fetch source snapshot from \`${this.source}\`.` | ||
| return `Failed to fetch source snapshot from \`${this.source}\`. Check your network access and retry.` |
There was a problem hiding this comment.
When repository materialization fails because the global store is unwritable or out of space, materializeStoreEntry maps those filesystem errors into SnapshotFetchError through the callback in repository/fetch.ts, so checking network access cannot fix the reported failure. Make this advice cover filesystem permissions and disk space as well, or preserve enough cause information to select the appropriate recovery step.
Useful? React with 👍 / 👎.
| }> { | ||
| override get message() { | ||
| return `Failed to fetch or extract package tarball from \`${this.url}\`.` | ||
| return `Failed to fetch or extract package tarball from \`${this.url}\`. Check your network access and retry.` |
There was a problem hiding this comment.
When a tarball downloads successfully but extraction rejects an invalid path, unsupported entry type, multiple roots, corrupt gzip data, or a local filesystem write, all of those paths raise TarballFetchError in sources/tarball/fetch.ts. Telling users only to check network access is therefore misleading for these common extraction failures; use cause-sensitive guidance or mention archive and filesystem failures too.
Useful? React with 👍 / 👎.
ed1aab9 to
6a942d4
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a942d4233
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }> { | ||
| override get message() { | ||
| return `Failed to parse project manifest at \`${this.path}\`.` | ||
| return `Failed to parse project manifest at \`${this.path}\`. Fix the invalid manifest, then retry.` |
There was a problem hiding this comment.
When readNodeModulesVersion cannot read an installed package's node_modules/<pkg>/package.json for any reason other than NotFound, javascript.ts lines 444–452 converts that filesystem failure into ManifestParseError. For a permissions or I/O failure, telling the user to fix an invalid manifest cannot resolve the problem; preserve the read failure or include filesystem-access guidance.
Useful? React with 👍 / 👎.
| ? `Network request failed for \`${this.url}\`. Check your connection and retry.` | ||
| : "Network request failed. Check your connection and retry." |
There was a problem hiding this comment.
When the npm registry returns a non-2xx response or a body that fails JSON/schema decoding, NpmRegistryClient.getPackageMetadata maps it to NetworkError (client.ts lines 49–64). With a 401/403/5xx response or malformed metadata the connection can be healthy, so checking it and retrying may never help; mention authentication, registry status, or invalid responses, or retain enough cause information to select the recovery advice.
Useful? React with 👍 / 👎.
| }> { | ||
| override get message() { | ||
| return `Global store entry at \`${this.path}\` is missing valid source metadata.` | ||
| return `Global store entry at \`${this.path}\` is missing valid source metadata. Run \`packref clean --global\`, then retry.` |
There was a problem hiding this comment.
When valid store metadata is unreadable, readStoreEntry wraps the readFileString failure—including EACCES—in StoreCorruptedError (store.ts lines 63–70). The new recovery step can therefore direct users to delete every global snapshot even though correcting permissions is sufficient, and deletion may still succeed when the parent directory is writable; distinguish access failures or avoid prescribing a global wipe for them.
Useful? React with 👍 / 👎.
| }> { | ||
| override get message() { | ||
| return `Failed to materialize project reference from \`${this.source}\` to \`${this.target}\`.` | ||
| return `Failed to materialize project reference from \`${this.source}\` to \`${this.target}\`. Check filesystem permissions and available disk space, then retry.` |
There was a problem hiding this comment.
When locked repository.directory metadata escapes the stored snapshot, createProjectReference explicitly raises ReflinkError (project.ts lines 82–87); a missing package subdirectory can also surface through the same error. In those cases permissions and disk space are unrelated, so the new advice leaves the user without a viable recovery path; distinguish invalid or missing source-directory metadata from actual filesystem copy failures.
Useful? React with 👍 / 👎.
…le error messages
6a942d4 to
457a1d2
Compare
Add documentation, domain glossary, architecture records, and actionable error messages for v1
Introduces
CONTEXT.mdas the authoritative domain glossary defining terms such as "package source reference," "dependency-tracked reference," "materialization," and "global store" that are used consistently across the CLI, specification, and implementation.Adds
docs/architecture.mddescribing the module seams, add/sync and install flows, storage model, consistency boundaries, and source layout. Adds three architecture decision records covering dependency source inspection via Packref, the commit-lockfile/ignore-source-directory split, and the bounded tarball fallback strategy, along with an ADR index.Rewrites
README.mdwith a full command reference, source resolution explanation, operational boundaries, and storage layout. Moves thepastryattribution to the footer.Adds
.plans/README.mdto orient contributors to the plans directory as implementation history rather than current guidance, and adds a historical-reference notice toimplementation-strategy.md. Updates plan 09 to reflect its completed status and corrects thesyncbehavior note to match the implemented behavior: sync reconciles existing dependency-tracked references only and does not adopt unreferenced manifest dependencies.Updates
AGENTS.mdto point toCONTEXT.md,docs/architecture.md, and the ADR index, and corrects theinstalldescription to say "restores locked references exactly" rather than "restores the lockfile exactly."Sharpens all command descriptions to be specific and action-oriented, and extends every error message in
src/lib/core/errors.tswith a concrete recovery instruction.Adds a
typecheckscript topackage.jsonand a CLI help test that asserts every v1 command appears with its description and thatadd,remove, andcleandocument their optional arguments and flags.