Validate full-server fx dependency graph - #25
Conversation
Extract the full-server module composition into app.FullServerModule so the running `piri serve full` command and the tests share one source of truth for the dependency graph. Add TestFullServerModule_ValidateApp, which runs fx.ValidateApp across each branch of store.StorageModule (in-memory, filesystem, S3). fx validates the graph in dry-run mode without invoking any constructors, so no Lotus/eth endpoints are dialed. Signed-off-by: Miroslav Bajtoš <oss@bajtos.net> Assisted-by: Claude:claude-opus-4-8
PR #18 (attested signatures) changed ProvideIdentity to return the concrete identity.Identity struct and switched consumers such as manager.NewPieceAccepter to depend on ucan.Issuer. The fx graph no longer provided ucan.Issuer or ucan.Signer, so the node failed to start: missing type: ucan.Issuer (did you mean to Provide it?) identity.Identity embeds multikey.Issuer, which satisfies both ucan.Issuer and ucan.Signer. Expose all three shapes via fx.Annotate so every consumer resolves. Add an fx.ValidateApp test asserting the identity module provides identity.Identity, ucan.Issuer and ucan.Signer. Signed-off-by: Miroslav Bajtoš <oss@bajtos.net> Assisted-by: Claude:claude-opus-4-8
There was a problem hiding this comment.
Pull request overview
This PR strengthens the fx wiring for the full Piri server by (1) making the identity module explicitly provide all interface shapes required by downstream UCAN/PDP components and (2) introducing a single “full server” fx module that can be validated in tests and reused by the CLI entrypoint.
Changes:
- Annotate the identity fx provider so
identity.Identityis also supplied asucan.Issueranducan.Signer. - Add a
FullServerModulecomposition function as the single source of truth for the full-server fx dependency graph. - Add
fx.ValidateApptests to validate both the identity shape exports and the full-server graph across storage configuration branches.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/fx/identity/provider.go | Exposes identity provider under identity.Identity, ucan.Issuer, and ucan.Signer via fx.Annotate/fx.As. |
| pkg/fx/identity/provider_test.go | Adds a wiring validation test ensuring the identity module provides all required shapes. |
| pkg/fx/app/full.go | Introduces FullServerModule(cfg) that composes the complete full-server fx graph (health mode + common + UCAN + PDP). |
| pkg/fx/app/full_test.go | Validates the full-server dependency graph across storage variants using fx.ValidateApp. |
| pkg/fx/app/common.go | Updates the comment to reflect the identity module now provides all required identity shapes. |
| cmd/cli/serve/full.go | Switches CLI wiring to use app.FullServerModule(appCfg) to keep the CLI and tests aligned. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
This new test file is probably redundant and not needed after we added full_test.go that calls fx.ValidateApp.
Let me know if you prefer to keep or remove it.
There was a problem hiding this comment.
I say keep it in case we change/remove full_test.go.
Document that pkg/fx/app/full_test.go must be kept in sync whenever config options change the fx dependency graph, so the validation test keeps covering every graph shape. Symlink CLAUDE.md to AGENTS.md so both agent toolchains read the same guidelines. Signed-off-by: Miroslav Bajtoš <oss@bajtos.net> Assisted-by: Claude:claude-opus-4-8
There was a problem hiding this comment.
I say keep it in case we change/remove full_test.go.
| @@ -0,0 +1 @@ | |||
| AGENTS.md No newline at end of file | |||
There was a problem hiding this comment.
IIRC imports are prefixed with @?
There was a problem hiding this comment.
I used a symlink, not a text reference (import).
output of git diff - notice that file mode is 120000:
diff --git a/CLAUDE.md b/CLAUDE.md
new file mode 120000
index 0000000..47dc3e3
--- /dev/null
+++ b/CLAUDE.md
@@ -0,0 +1 @@
+AGENTS.mdQuoting Claude:
That output tells us CLAUDE.md is being added as a symbolic link, not a regular file.
The key is the mode 120000:
- 100644 — normal file
- 100755 — executable file
- 120000 — symlink
- 160000 — gitlink (submodule)
I don't have a strong opinion whether it's better to use a symlink or an import, I am happy change this. Is therre any existing convention in fil-forge repos to follow?
In https://github.com/fil-forge/smelt, there is only CLAUDE.md and no AGENTS.md 🤔
test: Validate full-server fx dependency graph
Extract the full-server module composition into app.FullServerModule so the running
piri serve fullcommand and the tests share one source of truth for the dependency graph.Add TestFullServerModule_ValidateApp, which runs fx.ValidateApp across each branch of store.StorageModule (in-memory, filesystem, S3). fx validates the graph in dry-run mode without invoking any constructors, so no Lotus/eth endpoints are dialed.
The new tests fail - the fix follows in the next commit.
fix: Provide ucan.Issuer and ucan.Signer from identity module
PR #18 (attested signatures) changed ProvideIdentity to return the concrete identity.Identity struct and switched consumers such as manager.NewPieceAccepter to depend on ucan.Issuer. The fx graph no longer provided ucan.Issuer or ucan.Signer, so the node failed to start:
identity.Identity embeds multikey.Issuer, which satisfies both ucan.Issuer and ucan.Signer. Expose all three shapes via fx.Annotate so every consumer resolves.
Add an fx.ValidateApp test asserting the identity module provides identity.Identity, ucan.Issuer and ucan.Signer.
docs: Add AGENTS.md with CLAUDE.md symlink
Document that
pkg/fx/app/full_test.gomust be kept in sync whenever config options change the fx dependency graph, so the validation test keeps covering every graph shape.Symlink CLAUDE.md to AGENTS.md so both agent toolchains read the same guidelines.