Deprecate DAG modelling of server setups - #84
Open
Arcadi4 wants to merge 13 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change retires topology-based runtime modeling in favor of executable runtime detection, runtime components, effective ecosystem offers, and admission verdicts. It adds core-package normalization and installation flows, updates artifact compatibility, status rendering, routing, and bootstrap integration, and removes topology APIs, commands, tests, and dependencies. ChangesRuntime detection and installation migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ExecutableDetector
participant Workspace
participant ServerInstance
participant Install
participant Status
ExecutableDetector->>Workspace: produce PrimaryRuntime and RuntimeComponents
Workspace->>ServerInstance: materialize and normalize runtime data
ServerInstance->>Install: provide EffectiveEcosystems
Install->>ServerInstance: evaluate runtime admission
ServerInstance->>Status: provide runtime offers and components
Status-->>Status: render runtime and package sections
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Arcadi4
force-pushed
the
refactor/retire-topology
branch
from
July 18, 2026 19:33
7d04d13 to
af20bda
Compare
Arcadi4
marked this pull request as ready for review
July 26, 2026 11:23
![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: 7116d7f4f5
ℹ️ 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".
| if server == nil || !server.IsValid() { | ||
| return types.EcoUnspecified | ||
| } | ||
| for _, component := range server.RuntimeComponents { |
There was a problem hiding this comment.
When the detected server is CatServer or Youer, its loader is represented by EffectiveEcosystems rather than RuntimeComponents (both detectors emit only the Minecraft component), so this loop returns EcoUnspecified. Consequently the Fabric and Forge bootstrap guards treat an existing Forge/NeoForge hybrid as loader-free and proceed with a conflicting, platform-altering installation. Include the runtime's direct effective loader offers here, or materialize those loader components during detection.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install/install_recursive_verify.go (1)
59-79: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftFolia gate rejects hash-matched artifacts that couldn't be introspected.
ensureFoliaArtifactCompatibilityis invoked before thelen(infos) == 0hash-match fallback (Lines 80-92). Wheninfosis empty for an expected Bukkit/Paper artifact (e.g. the analyzer can't extract plugin descriptors from that jar format),foundFamilystaysfalse, so neitherreturn nilcondition at Line 153 is satisfied, and the function falls through to the "does not declare folia-supported" error — even though this artifact would otherwise pass via the hash-match fallback. This turns "we have no evidence either way" into an outright rejection, breaking previously-working hash-verified installs whenever the runtime is Folia.Only reject when a Bukkit-family descriptor was actually found and it fails to declare Folia support; when no family evidence exists at all, defer to the existing fallback logic instead.
🛠️ Proposed fix: don't reject purely on missing evidence
- if foundFamily && allFamilyInfosSupported { + if !foundFamily || allFamilyInfosSupported { return nil } return fmt.Errorf( "install: artifact verification failed for %s: bukkit plugin does not declare folia-supported: true", path, )Also applies to: 129-160
🤖 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 `@install/install_recursive_verify.go` around lines 59 - 79, Update ensureFoliaArtifactCompatibility so it returns without rejecting when no Bukkit-family descriptor is found, allowing the existing len(infos) == 0 hash-match fallback to decide the result. Preserve rejection only when a Bukkit-family artifact was identified and its Folia support is incompatible; do not alter unrelated ecosystem or verification handling.
🧹 Nitpick comments (12)
install/install.go (1)
47-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
contextshadows the importedcontextpackage inside this function.It compiles today only because
context.Contextisn't referenced after line 47. Rename toerrCtx/errFieldsto avoid a confusing trap for the next edit.♻️ Suggested rename
- context := map[string]any{"package": id.StringFull()} + errFields := map[string]any{"package": id.StringFull()} if err := ctx.Err(); err != nil { - return installError(CategoryApply, err, context) + return installError(CategoryApply, err, errFields) }(apply the same rename to the remaining uses in this function)
🤖 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 `@install/install.go` at line 47, Rename the local context map in the affected install function to errCtx or errFields, and update every remaining reference to that map within the function. Keep the imported context package unshadowed.install/install_core_requests.go (2)
102-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReusing
regularRootPolicyfor core conflict tracking is confusing.
prepareCoreRequeststracks core-package policies with a type named for regular roots. A neutral name (e.g.requestPolicy) would keep both call sites honest.🤖 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 `@install/install_core_requests.go` around lines 102 - 146, Rename regularRootPolicy to a neutral request-policy type, such as requestPolicy, and update its declaration and all usages, including the policies map and local policy in prepareCoreRequests. Preserve the existing fields and conflict-comparison behavior for both regular-root and core-package requests.
67-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
classifyInstallRequestsnever returns a non-nil error.Every caller (
Install,InstallMany,Plan) pays for an error branch that cannot trigger. Either drop the error from the signature or document what future failure it reserves.🤖 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 `@install/install_core_requests.go` around lines 67 - 100, Update classifyInstallRequests to remove its unused error return, since the function has no failure path, and adjust every caller (Install, InstallMany, and Plan) to consume only the classified core and regular request slices without error branches.bootstrap/bootstrap.go (1)
36-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate loader-detection logic vs.
workspace'sselectedLoaderEcosystems.This mirrors the RuntimeComponents→ecosystem mapping already implemented internally in
workspace/workspace_server_instance_ecosystems.go'sselectedLoaderEcosystems, just unexported and scoped differently. Two independent implementations of the same mapping risk drifting apart when a new loader is added to one but not the other.Consider exporting a canonical component-to-ecosystem mapping from
workspacefor both call sites to share.🤖 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 `@bootstrap/bootstrap.go` around lines 36 - 54, Replace the duplicate mapping in bootstrap’s selectedLoader with the canonical exported mapping from workspace.selectedLoaderEcosystems. Update the workspace symbol and its callers so RuntimeComponents-to-ecosystem detection has one shared implementation, preserving invalid-server and unspecified fallback behavior.workspace/workspace_server_instance_ecosystems.go (1)
50-66: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGate the Bukkit offer on loader components, not on
len(offers).
len(offers) > 0only means "a loader was found" becauseoffershappens to be empty at this point. Any future offer appended before the switch would silently add a Bukkit offer for Arclight without a loader.♻️ Suggested change
case "arclight": - for _, ecosystem := range selectedLoaderEcosystems( - s.RuntimeComponents, - ) { + loaders := selectedLoaderEcosystems(s.RuntimeComponents) + for _, ecosystem := range loaders { offers = appendEffectiveEcosystem( offers, ecosystem, types.CompatCompatible, ) } - if len(offers) > 0 { + if len(loaders) > 0 { offers = appendEffectiveEcosystem( offers, types.EcoBukkit, types.CompatCompatible, ) }🤖 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 `@workspace/workspace_server_instance_ecosystems.go` around lines 50 - 66, In the arclight branch, gate the Bukkit offer on whether selectedLoaderEcosystems(s.RuntimeComponents) returns at least one loader component, rather than checking len(offers). Preserve appending compatible loader offers and add the Bukkit offer only when loader components are present.cmd/cmd_status.go (1)
159-222: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueEmpty runtime label hides Mods/Plugins entirely.
The whole Platform tree — including the Components, Offers, Mods and Plugins children — is only emitted when
statusRuntimeLabel(primary)is non-empty. For a runtime whose name normalizes to an empty string, the user loses the package listings, not just the platform name. Consider falling back to a placeholder label rather than skipping the tree.🤖 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 `@cmd/cmd_status.go` around lines 159 - 222, Update the Platform tree construction around statusRuntimeLabel(primary) so an empty runtime label uses a non-empty placeholder instead of suppressing output. Preserve the existing Components, Offers, Mods, and Plugins child generation and emit the tree with the fallback label and primary.Version annotation.workspace/workspace_runtime_types.go (1)
54-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
selectedLoaderEcosystemsinstead of re-matching loader names here.
selectedLoaderEcosystemsinworkspace_server_instance_ecosystems.goperforms the same fabric/forge/neoforge component matching but normalizes withstrings.ToLower(strings.TrimSpace(...)). This copy matches raw names, so an un-normalized component name (e.g."Fabric-Loader") yields different results in the two paths.♻️ Suggested consolidation
func (s *ServerInstance) DerivedModLoader() types.Ecosystem { if s == nil || !s.IsValid() { return types.EcoUnspecified } - for _, component := range s.RuntimeComponents { - if component.Eco == types.EcoFabric && - (component.Name == "fabric-loader" || - component.Name == "fabricloader") { - return types.EcoFabric - } - if component.Eco == types.EcoForge && component.Name == "forge" { - return types.EcoForge - } - if component.Eco == types.EcoNeoforge && - component.Name == "neoforge" { - return types.EcoNeoforge - } - } + if loaders := selectedLoaderEcosystems(s.RuntimeComponents); len(loaders) > 0 { + return loaders[0] + } return types.EcoMinecraft }🤖 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 `@workspace/workspace_runtime_types.go` around lines 54 - 73, Update ServerInstance.DerivedModLoader to reuse selectedLoaderEcosystems for loader detection instead of independently matching RuntimeComponents names. Preserve the existing nil/invalid and default EcoMinecraft behavior, and ensure the shared helper’s normalization handles casing and surrounding whitespace consistently.workspace/internal/detector/detector_bukkit_executable_test.go (1)
48-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
wantIdentityis now unused.The loop only asserts
wantCore; dropwantIdentity(or assert it) so the table doesn't carry stale expectations.♻️ Proposed cleanup
tests := []struct { name string path string - wantIdentity string wantCore string }{ { name: "paper fixture projects official paper runtime", path: filepath.Join(fixtureRoot, "test_paper", "paper"), - wantIdentity: "paper", wantCore: "paper", },🤖 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 `@workspace/internal/detector/detector_bukkit_executable_test.go` around lines 48 - 71, Remove the unused wantIdentity field and its values from the test table in the detector executable test, unless the loop is updated to assert it. Keep the existing wantCore expectations and assertions unchanged.workspace/internal/detector/detector_modloader_install.go (1)
238-259: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRaw
types.BarePackageName(name)here vsinput.ToProjectName(...)in the sibling detectors.Callers control
nametoday, so behaviour is fine, but normalizing consistently avoids a future caller passing"NeoForge"and silently missing downstream name matches.🤖 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 `@workspace/internal/detector/detector_modloader_install.go` around lines 238 - 259, Normalize the loader name consistently in the ExecutableEvidence construction by replacing both types.BarePackageName(name) usages with input.ToProjectName(name). Keep the platform, versions, and Minecraft runtime component unchanged.workspace/internal/detector/detector_arclight.go (2)
74-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
primaryduplicated intoRuntimeComponents.Other detectors vary here (geyser passes
nil, bukkit omits the primary, forge/fabric include it). Worth settling one convention so consumers iterating components don't double-count the runtime identity.🤖 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 `@workspace/internal/detector/detector_arclight.go` around lines 74 - 83, Update the component construction in the Arclight detector so primary is not included alongside the Minecraft runtime component, matching the chosen convention used by detectors such as Bukkit and preventing consumers from double-counting the runtime identity.
173-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
arclightLoaderEcosystemreturns on the first non-matchinglaunch.mainClassvalue.The
defaultarm returns instead ofcontinue, which is fine given only onelaunch.mainClasskey is expected, but an unknown/future main class silently aborts the scan. Behaviourally equivalent today; flagging only for intent clarity.🤖 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 `@workspace/internal/detector/detector_arclight.go` around lines 173 - 198, Update arclightLoaderEcosystem so an unrecognized launch.mainClass value in the switch default arm continues scanning subsequent lines instead of returning immediately. Preserve the existing ecosystem returns for recognized Fabric, NeoForge, and Forge main classes, and retain the final unspecified result when no recognized value is found.workspace/internal/detector/detector_catserver.go (1)
44-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Provenanceleft unset here (and in arclight/geyser/fabric/forge) while the bukkit detector populates it.If provenance is meant for diagnostics of ambiguous candidates, the gap makes it unusable uniformly. Otherwise it's fine as-is.
🤖 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 `@workspace/internal/detector/detector_catserver.go` around lines 44 - 58, Update the executable evidence construction in the catserver detector, and the corresponding arclight, geyser, fabric, and forge detectors, to populate the Provenance field consistently with the bukkit detector. Reuse the same provenance value or construction pattern established by the bukkit detector while preserving the existing package and version data.
🤖 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 `@artifact/reader_bukkit.go`:
- Around line 100-125: Update the platform detection logic in the descriptor
classification function to avoid substring matching on signals. Tokenize the
normalized descriptor values and match “paper”, “folia”, and “leaves” only as
exact identifiers, while preserving the existing PaperPluginLoader and Libraries
checks and Bukkit fallback.
In `@cmd/cmd_bisect.go`:
- Around line 388-390: Update the empty-result message in the bisect flow to say
“core packages” instead of “identity packages,” while leaving the IsCorePackage
filtering logic unchanged.
In `@install/ambient.go`:
- Around line 108-109: Update the Fabric component handling in the ambient
detection logic, including the corresponding branch around the other Fabric
component cases, so observing either fabric-loader or fabric-api marks Fabric as
present and creates the EcoFabric/minecraft alias. Ensure API-only detection
follows the same alias-creation path as loader detection.
In `@workspace/internal/detector/detector_arclight.go`:
- Around line 84-100: Update the Arclight detection flow around
arclightLoaderEcosystem and parseArclightLoaderComponent so a loader component
is still appended when META-INF/installer.json is missing or does not provide a
concrete version. Preserve the resolved version when available, and otherwise
emit the identified loader using types.VersionUnknown so EffectiveEcosystems
continues to offer the arclight ecosystem.
In `@workspace/internal/detector/detector_executable_evidence.go`:
- Around line 73-78: The nil-primary contract is inconsistent: update resolved()
in workspace/internal/detector/detector_executable_evidence.go to discard
candidates without PrimaryRuntime, preserving the documented behavior that nil
does not establish a runtime; in
workspace/internal/detector/detector_robustness_regression_test.go, initialize
forgeEvidence with a real Forge PrimaryRuntime so the ambiguity regression test
does not rely on nil-primary behavior.
In `@workspace/workspace_environment.go`:
- Around line 56-69: Update the mcdreforged version probe around exec.Command
and Output to use a bounded context with an explicit timeout, ensuring a hung
binary cannot block workspace probing indefinitely. Preserve the existing
version parsing and warning behavior, including reporting command errors from
the timed execution.
---
Outside diff comments:
In `@install/install_recursive_verify.go`:
- Around line 59-79: Update ensureFoliaArtifactCompatibility so it returns
without rejecting when no Bukkit-family descriptor is found, allowing the
existing len(infos) == 0 hash-match fallback to decide the result. Preserve
rejection only when a Bukkit-family artifact was identified and its Folia
support is incompatible; do not alter unrelated ecosystem or verification
handling.
---
Nitpick comments:
In `@bootstrap/bootstrap.go`:
- Around line 36-54: Replace the duplicate mapping in bootstrap’s selectedLoader
with the canonical exported mapping from workspace.selectedLoaderEcosystems.
Update the workspace symbol and its callers so RuntimeComponents-to-ecosystem
detection has one shared implementation, preserving invalid-server and
unspecified fallback behavior.
In `@cmd/cmd_status.go`:
- Around line 159-222: Update the Platform tree construction around
statusRuntimeLabel(primary) so an empty runtime label uses a non-empty
placeholder instead of suppressing output. Preserve the existing Components,
Offers, Mods, and Plugins child generation and emit the tree with the fallback
label and primary.Version annotation.
In `@install/install_core_requests.go`:
- Around line 102-146: Rename regularRootPolicy to a neutral request-policy
type, such as requestPolicy, and update its declaration and all usages,
including the policies map and local policy in prepareCoreRequests. Preserve the
existing fields and conflict-comparison behavior for both regular-root and
core-package requests.
- Around line 67-100: Update classifyInstallRequests to remove its unused error
return, since the function has no failure path, and adjust every caller
(Install, InstallMany, and Plan) to consume only the classified core and regular
request slices without error branches.
In `@install/install.go`:
- Line 47: Rename the local context map in the affected install function to
errCtx or errFields, and update every remaining reference to that map within the
function. Keep the imported context package unshadowed.
In `@workspace/internal/detector/detector_arclight.go`:
- Around line 74-83: Update the component construction in the Arclight detector
so primary is not included alongside the Minecraft runtime component, matching
the chosen convention used by detectors such as Bukkit and preventing consumers
from double-counting the runtime identity.
- Around line 173-198: Update arclightLoaderEcosystem so an unrecognized
launch.mainClass value in the switch default arm continues scanning subsequent
lines instead of returning immediately. Preserve the existing ecosystem returns
for recognized Fabric, NeoForge, and Forge main classes, and retain the final
unspecified result when no recognized value is found.
In `@workspace/internal/detector/detector_bukkit_executable_test.go`:
- Around line 48-71: Remove the unused wantIdentity field and its values from
the test table in the detector executable test, unless the loop is updated to
assert it. Keep the existing wantCore expectations and assertions unchanged.
In `@workspace/internal/detector/detector_catserver.go`:
- Around line 44-58: Update the executable evidence construction in the
catserver detector, and the corresponding arclight, geyser, fabric, and forge
detectors, to populate the Provenance field consistently with the bukkit
detector. Reuse the same provenance value or construction pattern established by
the bukkit detector while preserving the existing package and version data.
In `@workspace/internal/detector/detector_modloader_install.go`:
- Around line 238-259: Normalize the loader name consistently in the
ExecutableEvidence construction by replacing both types.BarePackageName(name)
usages with input.ToProjectName(name). Keep the platform, versions, and
Minecraft runtime component unchanged.
In `@workspace/workspace_runtime_types.go`:
- Around line 54-73: Update ServerInstance.DerivedModLoader to reuse
selectedLoaderEcosystems for loader detection instead of independently matching
RuntimeComponents names. Preserve the existing nil/invalid and default
EcoMinecraft behavior, and ensure the shared helper’s normalization handles
casing and surrounding whitespace consistently.
In `@workspace/workspace_server_instance_ecosystems.go`:
- Around line 50-66: In the arclight branch, gate the Bukkit offer on whether
selectedLoaderEcosystems(s.RuntimeComponents) returns at least one loader
component, rather than checking len(offers). Preserve appending compatible
loader offers and add the Bukkit offer only when loader components are present.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 60d01895-2f3f-4281-9454-9413df4c16b4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (83)
README.mdREADME_CN.mdartifact/artifact.goartifact/reader_bukkit.goartifact/types.gobootstrap/bootstrap.gobootstrap/fabric.gobootstrap/forge.gobootstrap/mojang.gobootstrap/neoforge.gocmd/cmd_add.gocmd/cmd_bisect.gocmd/cmd_status.gocmd/cmd_topology.gocmd/topology_renderer.gogo.modinput/syntax.goinstall/ambient.goinstall/install.goinstall/install_batch.goinstall/install_core_requests.goinstall/install_helpers.goinstall/install_identity_sort.goinstall/install_options.goinstall/install_recursive_verify.gostate/manifest.gotypes/AGENTS.mdtypes/core_capability_test.gotypes/type_compat.gotypes/type_core.gotypes/type_core_package.gotypes/type_core_package_test.gotypes/type_id.gotypes/type_identity.gotypes/type_server_topology.goupstream/routing/routing.goupstream/routing/routing_policy.goupstream/routing/routing_policy_test.goupstream/routing/routing_policy_test_helpers.goupstream/upstream_types.goworkspace/internal/detector/detector_arclight.goworkspace/internal/detector/detector_bukkit_executable.goworkspace/internal/detector/detector_bukkit_executable_test.goworkspace/internal/detector/detector_catserver.goworkspace/internal/detector/detector_executable_evidence.goworkspace/internal/detector/detector_fabric_executable.goworkspace/internal/detector/detector_forge_runtime_helpers.goworkspace/internal/detector/detector_forge_runtime_test.goworkspace/internal/detector/detector_geyser.goworkspace/internal/detector/detector_geyser_test.goworkspace/internal/detector/detector_modloader_install.goworkspace/internal/detector/detector_neoforge_install_test.goworkspace/internal/detector/detector_robustness_regression_test.goworkspace/internal/detector/detector_sponge.goworkspace/internal/detector/detector_sponge_test.goworkspace/internal/detector/detector_vanilla.goworkspace/internal/topology/topology_connection_definition.goworkspace/internal/topology/topology_connection_registry.goworkspace/internal/topology/topology_connection_source.goworkspace/workspace.goworkspace/workspace_environment.goworkspace/workspace_exec.goworkspace/workspace_interpretation.goworkspace/workspace_paper_integration_test.goworkspace/workspace_runtime_admission.goworkspace/workspace_runtime_admission_test.goworkspace/workspace_runtime_materialize.goworkspace/workspace_runtime_types.goworkspace/workspace_server_instance_components.goworkspace/workspace_server_instance_cores.goworkspace/workspace_server_instance_ecosystems.goworkspace/workspace_server_instance_ecosystems_test.goworkspace/workspace_test_helpers_test.goworkspace/workspace_topology_data.goworkspace/workspace_topology_enrich.goworkspace/workspace_topology_enrich_test.goworkspace/workspace_topology_evaluation.goworkspace/workspace_topology_evaluation_test.goworkspace/workspace_topology_registry.goworkspace/workspace_topology_registry_data.goworkspace/workspace_topology_registry_data_test.goworkspace/workspace_topology_registry_test.goworkspace/workspace_workspace_types.go
💤 Files with no reviewable changes (25)
- workspace/internal/topology/topology_connection_definition.go
- upstream/routing/routing_policy_test_helpers.go
- workspace/workspace_server_instance_cores.go
- workspace/workspace_topology_registry_data.go
- cmd/topology_renderer.go
- workspace/internal/topology/topology_connection_source.go
- workspace/internal/topology/topology_connection_registry.go
- workspace/workspace_topology_registry_data_test.go
- workspace/workspace_topology_evaluation.go
- types/type_identity.go
- types/core_capability_test.go
- workspace/workspace_topology_registry.go
- install/install_identity_sort.go
- upstream/upstream_types.go
- workspace/workspace_test_helpers_test.go
- cmd/cmd_topology.go
- workspace/workspace_topology_data.go
- types/type_core.go
- upstream/routing/routing.go
- workspace/workspace_topology_enrich.go
- workspace/workspace_topology_evaluation_test.go
- types/type_id.go
- workspace/workspace_topology_registry_test.go
- workspace/workspace_topology_enrich_test.go
- types/type_server_topology.go
Comment on lines
+108
to
+109
| case component.Eco == types.EcoFabric && name == "fabric-api": | ||
| ambient.Add(component) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add the Fabric Minecraft alias for Fabric API.
fabric-api is added as an ambient Fabric component, but only fabric-loader sets the flag that creates the EcoFabric/minecraft alias. Treat either observed Fabric component as Fabric presence so API-only evidence does not leave its scoped Minecraft identity unresolved.
Proposed fix
- var hasFabricLoader bool
+ var hasFabric bool
var hasForge bool
var hasNeoForge bool
...
- hasFabricLoader = true
+ hasFabric = true
case component.Eco == types.EcoFabric && name == "fabric-api":
ambient.Add(component)
+ hasFabric = true
...
- if hasFabricLoader {
+ if hasFabric {Also applies to: 119-128
🤖 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 `@install/ambient.go` around lines 108 - 109, Update the Fabric component
handling in the ambient detection logic, including the corresponding branch
around the other Fabric component cases, so observing either fabric-loader or
fabric-api marks Fabric as present and creates the EcoFabric/minecraft alias.
Ensure API-only detection follows the same alias-creation path as loader
detection.
Comment on lines
73
to
78
| func isVanillaEvidence(cand *ExecutableEvidence) bool { | ||
| if cand == nil { | ||
| return false | ||
| } | ||
| if cand.Topology != nil && cand.Topology.PrimaryNode == types.RuntimeNodeMinecraft { | ||
| return true | ||
| } | ||
| if cand.TopologySeed != nil && cand.TopologySeed.PrimaryNode == types.RuntimeNodeMinecraft { | ||
| return true | ||
| } | ||
| return false | ||
| return cand != nil && | ||
| cand.PrimaryRuntime != nil && | ||
| cand.PrimaryRuntime.Eco == types.EcoMinecraft && | ||
| cand.PrimaryRuntime.Name == "minecraft" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Nil PrimaryRuntime is treated as authoritative "specific" evidence. The doc comment states a nil primary does not establish a runtime, but isVanillaEvidence returns false for it, so nil-primary candidates survive resolution and can force ambiguity.
workspace/internal/detector/detector_executable_evidence.go#L73-L78: decide the contract — either drop nil-primary candidates inresolved()or relax the doc comment.workspace/internal/detector/detector_robustness_regression_test.go#L332-L332: giveforgeEvidencea real forgePrimaryRuntimeso the ambiguity case doesn't depend on the nil-primary loophole.
📍 Affects 2 files
workspace/internal/detector/detector_executable_evidence.go#L73-L78(this comment)workspace/internal/detector/detector_robustness_regression_test.go#L332-L332
🤖 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 `@workspace/internal/detector/detector_executable_evidence.go` around lines 73
- 78, The nil-primary contract is inconsistent: update resolved() in
workspace/internal/detector/detector_executable_evidence.go to discard
candidates without PrimaryRuntime, preserving the documented behavior that nil
does not establish a runtime; in
workspace/internal/detector/detector_robustness_regression_test.go, initialize
forgeEvidence with a real Forge PrimaryRuntime so the ambiguity regression test
does not rely on nil-primary behavior.
Comment on lines
+56
to
69
| version := types.VersionUnknown | ||
| output, err := exec.Command("mcdreforged", "--version").Output() | ||
| if err != nil { | ||
| log.ReportWarn( | ||
| fmt.Errorf( | ||
| "cannot execute mcdr, it is in your $PATH?: %w", | ||
| "cannot execute mcdr, is it in your $PATH?: %w", | ||
| err, | ||
| ), | ||
| ) | ||
| } else if fields := strings.Fields(string(output)); len(fields) > 1 { | ||
| version = types.BareVersion(fields[1]) | ||
| } else { | ||
| log.ReportWarn(fmt.Errorf("cannot parse mcdr version output")) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Bound the mcdreforged --version call with a timeout.
exec.Command(...).Output() has no deadline; a hung/misbehaving mcdreforged binary would block workspace probing indefinitely. Static analysis also flags this call.
🕐 Proposed fix: use a bounded context
- output, err := exec.Command("mcdreforged", "--version").Output()
+ ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ output, err := exec.CommandContext(ctx, "mcdreforged", "--version").Output()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| version := types.VersionUnknown | |
| output, err := exec.Command("mcdreforged", "--version").Output() | |
| if err != nil { | |
| log.ReportWarn( | |
| fmt.Errorf( | |
| "cannot execute mcdr, it is in your $PATH?: %w", | |
| "cannot execute mcdr, is it in your $PATH?: %w", | |
| err, | |
| ), | |
| ) | |
| } else if fields := strings.Fields(string(output)); len(fields) > 1 { | |
| version = types.BareVersion(fields[1]) | |
| } else { | |
| log.ReportWarn(fmt.Errorf("cannot parse mcdr version output")) | |
| } | |
| version := types.VersionUnknown | |
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| defer cancel() | |
| output, err := exec.CommandContext(ctx, "mcdreforged", "--version").Output() | |
| if err != nil { | |
| log.ReportWarn( | |
| fmt.Errorf( | |
| "cannot execute mcdr, is it in your $PATH?: %w", | |
| err, | |
| ), | |
| ) | |
| } else if fields := strings.Fields(string(output)); len(fields) > 1 { | |
| version = types.BareVersion(fields[1]) | |
| } else { | |
| log.ReportWarn(fmt.Errorf("cannot parse mcdr version output")) | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 57-57: os/exec.Command must not be called. use os/exec.CommandContext
(noctx)
🤖 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 `@workspace/workspace_environment.go` around lines 56 - 69, Update the
mcdreforged version probe around exec.Command and Output to use a bounded
context with an explicit timeout, ensuring a hung binary cannot block workspace
probing indefinitely. Preserve the existing version parsing and warning
behavior, including reporting command errors from the timed execution.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@bootstrap/bootstrap_test.go`:
- Around line 55-60: Update the “fabric via component” test fixture and expected
result so the primary runtime does not already identify as Fabric, while the
configured fabric-loader component is the sole source of Fabric detection. Keep
the test anchored to EffectiveEcosystems() and preserve the intended Fabric
expectation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a9cac021-4801-43fd-83b2-11b8904afcdb
📒 Files selected for processing (6)
bootstrap/bootstrap.gobootstrap/bootstrap_test.goinstall/install.goinstall/install_batch.goinstall/install_core_requests.goinstall/install_recursive_verify.go
🚧 Files skipped from review as they are similar to previous changes (2)
- install/install_recursive_verify.go
- install/install_batch.go
The ServerTopology DAG combined runtime detection, package identity, and compatibility policy into one graph. Separate them: observed evidence (flat ServerInstance), canonical package identity (CorePackage catalog), and derived compatibility (EffectiveEcosystems by transient policy).
Replace the scope-routed catalog with a plain alias-to-canonical index. Every route was SourceAuto, so the per-scope route keys, the inferredAuto and ambiguousAuto maps, and the non-auto fallback lookup were dead paths. Static data invariants move from init-time error plumbing into tests, so NormalizeCorePackage loses its error return and callers simplify.
The detector attributes paper-lineage brands from artifact evidence, but EffectiveEcosystems duplicated that list in a case arm. Export detector.IsPaperFamilyBrand backed by one brand map, drive the fixture test from the same map, and add direct EffectiveEcosystems coverage for the fork, hybrid, sponge, and proxy paths.
CompatResult and the incompatible/unresolved verdicts had no consumers; EffectiveEcosystems only ever emits compatible or degraded offers. Rename routing's topologyResolution to providerSelection and delete the redundant decl-use guard in reader_bukkit.go.
strings.Contains over the joined descriptor fields classified any plugin whose dependency names merely contained paper/folia/leaves (newspaper, my-leaves-addon) as EcoPaper. Compare each api/depend/softdepend entry as a whole token instead. Also update the bisect empty-result message to say core packages.
The loader component was only emitted when META-INF/installer.json existed and carried a concrete version, so an Arclight jar without it produced zero effective ecosystem offers and every mod install was rejected. Emit the loader identified from launch.mainClass with VersionUnknown and let installer metadata upgrade the version. Runtime component normalization now admits VersionUnknown and lets a concrete duplicate win over it; ambient dependencies still require concrete versions.
CatServer and Youer emit only a minecraft runtime component, so the component scan returned EcoUnspecified and the fabric/forge/neoforge bootstrap guards treated a hybrid runtime as loader-free, allowing a conflicting platform-altering install. Use the runtime's compatible modding offers instead, which cover both loader components and hybrid identities.
The gate rejected any expected Bukkit/Paper artifact whose descriptors could not be parsed, before the hash-match fallback could vouch for it. Reject only artifacts whose parsed bukkit-family descriptor lacks folia-supported: true.
The fabric case satisfied itself via the primary runtime identity, so a regression in component-based detection would not have failed it. Split into a primary-identity case and an arclight case where the fabric-loader component is the only fabric signal.
Arcadi4
force-pushed
the
refactor/retire-topology
branch
from
July 27, 2026 03:51
0a9875a to
02759a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
workspace/internal/detector/detector_geyser_test.go (1)
25-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert
PrimaryPathandVersion.The fixture manifest pins
Implementation-Version: 2.5.0, andmaterializeRuntimeInfonow treats an emptyPrimaryPathasUnknownExecutable— both are currently unasserted, so a regression in either would pass this test.💚 Proposed assertion tightening
if runtime.PrimaryRuntime == nil || runtime.PrimaryRuntime.Name.String() != "geyser" { t.Fatalf( "expected primary geyser runtime, got %+v", runtime.PrimaryRuntime, ) } + if runtime.PrimaryRuntime.Version.String() != "2.5.0" { + t.Fatalf("expected version 2.5.0, got %+v", runtime.PrimaryRuntime.Version) + } + if runtime.PrimaryPath != jarPath { + t.Fatalf("expected primary path %q, got %q", jarPath, runtime.PrimaryPath) + } }Adjust the
Versioncomparison to match its actual type/accessor.🤖 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 `@workspace/internal/detector/detector_geyser_test.go` around lines 25 - 31, Add assertions in the detector test alongside the existing PrimaryRuntime checks for PrimaryPath and Version, expecting the fixture’s Implementation-Version of 2.5.0 and the UnknownExecutable value for an empty PrimaryPath. Use Version’s actual type or accessor when comparing it, while preserving the existing geyser runtime assertions.cmd/cmd_status.go (1)
158-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the
runtimeComponentslocal.
data.Server.RuntimeComponentsis re-read here while the identical value was already captured at Line 69; using the local keeps the two consumers (sections filter and tree rendering) in sync.♻️ Suggested tweak
- if len(data.Server.RuntimeComponents) > 0 { + if len(runtimeComponents) > 0 { components := make( []string, 0, - len(data.Server.RuntimeComponents), + len(runtimeComponents), ) - for _, component := range data.Server.RuntimeComponents { + for _, component := range runtimeComponents {🤖 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 `@cmd/cmd_status.go` around lines 158 - 174, Update the tree-rendering logic near statusRuntimeLabel to use the existing runtimeComponents local instead of rereading data.Server.RuntimeComponents, including its length check and iteration, so the sections filter and tree rendering share the same value.
🤖 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 `@cmd/cmd_status.go`:
- Around line 97-113: Update the logo selection logic around logoEco so
compatible plugin-only ecosystems are supported: after the modding-offer search,
fall back to the first compatible offer, then to
data.Server.PrimaryRuntime.Identity.Eco when available, before leaving logoEco
as EcoUnspecified. Preserve the existing Minecraft and MCDR fallbacks and use
the existing compatibility and runtime state.
In `@workspace/workspace_runtime_materialize.go`:
- Around line 11-16: Update finalizeProbedRuntime’s unknown-evidence path to
return a newly allocated empty ServerInstance instead of the shared
UnknownExecutable sentinel. Preserve the existing validation conditions and
behavior for valid runtime evidence.
---
Nitpick comments:
In `@cmd/cmd_status.go`:
- Around line 158-174: Update the tree-rendering logic near statusRuntimeLabel
to use the existing runtimeComponents local instead of rereading
data.Server.RuntimeComponents, including its length check and iteration, so the
sections filter and tree rendering share the same value.
In `@workspace/internal/detector/detector_geyser_test.go`:
- Around line 25-31: Add assertions in the detector test alongside the existing
PrimaryRuntime checks for PrimaryPath and Version, expecting the fixture’s
Implementation-Version of 2.5.0 and the UnknownExecutable value for an empty
PrimaryPath. Use Version’s actual type or accessor when comparing it, while
preserving the existing geyser runtime assertions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 22fe11da-7ed0-4384-9807-bd0204a7fcdd
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (85)
README.mdREADME_CN.mdartifact/artifact.goartifact/reader_bukkit.goartifact/types.gobootstrap/bootstrap.gobootstrap/bootstrap_test.gobootstrap/fabric.gobootstrap/forge.gobootstrap/mojang.gobootstrap/neoforge.gocmd/cmd_add.gocmd/cmd_bisect.gocmd/cmd_status.gocmd/cmd_topology.gocmd/topology_renderer.gogo.modinput/syntax.goinstall/ambient.goinstall/install.goinstall/install_batch.goinstall/install_core_requests.goinstall/install_helpers.goinstall/install_identity_sort.goinstall/install_options.goinstall/install_recursive_verify.gostate/manifest.gotypes/AGENTS.mdtypes/core_capability_test.gotypes/type_compat.gotypes/type_core.gotypes/type_core_package.gotypes/type_core_package_test.gotypes/type_id.gotypes/type_identity.gotypes/type_server_topology.goupstream/routing/routing.goupstream/routing/routing_policy.goupstream/routing/routing_policy_test.goupstream/routing/routing_policy_test_helpers.goupstream/upstream_types.goworkspace/internal/detector/detector_arclight.goworkspace/internal/detector/detector_bukkit_executable.goworkspace/internal/detector/detector_bukkit_executable_test.goworkspace/internal/detector/detector_catserver.goworkspace/internal/detector/detector_executable_evidence.goworkspace/internal/detector/detector_fabric_executable.goworkspace/internal/detector/detector_forge_runtime_helpers.goworkspace/internal/detector/detector_forge_runtime_test.goworkspace/internal/detector/detector_geyser.goworkspace/internal/detector/detector_geyser_test.goworkspace/internal/detector/detector_modloader_install.goworkspace/internal/detector/detector_neoforge_install_test.goworkspace/internal/detector/detector_robustness_regression_test.goworkspace/internal/detector/detector_sponge.goworkspace/internal/detector/detector_sponge_test.goworkspace/internal/detector/detector_vanilla.goworkspace/internal/topology/topology_connection_definition.goworkspace/internal/topology/topology_connection_registry.goworkspace/internal/topology/topology_connection_source.goworkspace/workspace.goworkspace/workspace_environment.goworkspace/workspace_exec.goworkspace/workspace_interpretation.goworkspace/workspace_paper_integration_test.goworkspace/workspace_runtime_admission.goworkspace/workspace_runtime_admission_test.goworkspace/workspace_runtime_materialize.goworkspace/workspace_runtime_types.goworkspace/workspace_server_instance_components.goworkspace/workspace_server_instance_components_test.goworkspace/workspace_server_instance_cores.goworkspace/workspace_server_instance_ecosystems.goworkspace/workspace_server_instance_ecosystems_test.goworkspace/workspace_test_helpers_test.goworkspace/workspace_topology_data.goworkspace/workspace_topology_enrich.goworkspace/workspace_topology_enrich_test.goworkspace/workspace_topology_evaluation.goworkspace/workspace_topology_evaluation_test.goworkspace/workspace_topology_registry.goworkspace/workspace_topology_registry_data.goworkspace/workspace_topology_registry_data_test.goworkspace/workspace_topology_registry_test.goworkspace/workspace_workspace_types.go
💤 Files with no reviewable changes (25)
- workspace/workspace_topology_registry_data_test.go
- workspace/workspace_server_instance_cores.go
- types/type_id.go
- cmd/cmd_topology.go
- workspace/workspace_topology_evaluation.go
- workspace/internal/topology/topology_connection_source.go
- workspace/internal/topology/topology_connection_definition.go
- workspace/workspace_topology_registry_data.go
- upstream/routing/routing_policy_test_helpers.go
- types/type_identity.go
- upstream/upstream_types.go
- upstream/routing/routing.go
- workspace/workspace_topology_data.go
- types/core_capability_test.go
- types/type_core.go
- cmd/topology_renderer.go
- workspace/internal/topology/topology_connection_registry.go
- workspace/workspace_topology_evaluation_test.go
- workspace/workspace_topology_registry_test.go
- workspace/workspace_topology_registry.go
- workspace/workspace_topology_enrich.go
- workspace/workspace_test_helpers_test.go
- install/install_identity_sort.go
- types/type_server_topology.go
- workspace/workspace_topology_enrich_test.go
🚧 Files skipped from review as they are similar to previous changes (54)
- bootstrap/mojang.go
- types/type_compat.go
- workspace/workspace_server_instance_components_test.go
- upstream/routing/routing_policy.go
- install/install_helpers.go
- types/type_core_package_test.go
- bootstrap/bootstrap_test.go
- cmd/cmd_bisect.go
- install/install.go
- README_CN.md
- workspace/internal/detector/detector_neoforge_install_test.go
- types/AGENTS.md
- workspace/internal/detector/detector_catserver.go
- workspace/workspace_runtime_admission.go
- workspace/internal/detector/detector_sponge_test.go
- workspace/internal/detector/detector_geyser.go
- install/install_options.go
- bootstrap/forge.go
- types/type_core_package.go
- artifact/types.go
- bootstrap/bootstrap.go
- workspace/internal/detector/detector_modloader_install.go
- bootstrap/fabric.go
- install/install_recursive_verify.go
- workspace/internal/detector/detector_sponge.go
- workspace/internal/detector/detector_vanilla.go
- workspace/internal/detector/detector_forge_runtime_helpers.go
- install/install_core_requests.go
- workspace/workspace_server_instance_components.go
- workspace/internal/detector/detector_robustness_regression_test.go
- workspace/workspace.go
- install/ambient.go
- bootstrap/neoforge.go
- workspace/internal/detector/detector_fabric_executable.go
- workspace/internal/detector/detector_executable_evidence.go
- workspace/workspace_runtime_admission_test.go
- state/manifest.go
- workspace/internal/detector/detector_forge_runtime_test.go
- workspace/internal/detector/detector_bukkit_executable_test.go
- workspace/workspace_interpretation.go
- artifact/reader_bukkit.go
- workspace/workspace_exec.go
- workspace/workspace_server_instance_ecosystems_test.go
- workspace/workspace_runtime_types.go
- workspace/workspace_server_instance_ecosystems.go
- upstream/routing/routing_policy_test.go
- artifact/artifact.go
- workspace/workspace_paper_integration_test.go
- workspace/internal/detector/detector_bukkit_executable.go
- workspace/workspace_workspace_types.go
- go.mod
- input/syntax.go
- workspace/internal/detector/detector_arclight.go
- install/install_batch.go
Comment on lines
97
to
+113
| var logoEco types.Ecosystem | ||
| if serverPlatform == types.EcoVanilla { | ||
| if hasMcdr { | ||
| logoEco = types.EcoMcdr | ||
| } else if hasLucy { | ||
| // logoEco = | ||
| // lucy is not supposed to be a platform, needs refactor | ||
| // also need structural support for all other custom server clients | ||
| } else { | ||
| logoEco = types.EcoVanilla | ||
| for _, offer := range effectiveEcosystems { | ||
| if offer.Verdict == types.CompatCompatible && | ||
| offer.Ecosystem.IsModding() { | ||
| logoEco = offer.Ecosystem | ||
| break | ||
| } | ||
| } else if serverPlatform.IsModding() { | ||
| } | ||
| if hasServer && | ||
| logoEco == types.EcoUnspecified && | ||
| data.Server.PrimaryRuntime.Identity.Eco == types.EcoMinecraft { | ||
| logoEco = types.EcoMinecraft | ||
| } | ||
| if logoEco == types.EcoUnspecified && hasMcdr { | ||
| logoEco = types.EcoMcdr | ||
| } | ||
| if logoEco != types.EcoUnspecified { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect IsModding() and Satisfy() semantics for Bukkit/Paper/Velocity
ast-grep run --pattern 'func ($_ $_) IsModding() bool { $$$ }' --lang go
rg -nP -C5 '\bfunc \(.*Ecosystem\) (IsModding|Satisfy)\b'
rg -nP -C3 'EcoPaper|EcoVelocity|EcoBungeecord' --type=go -g 'types/*.go' | head -60Repository: mclucy/lucy
Length of output: 312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section in cmd/cmd_status.go
sed -n '70,140p' cmd/cmd_status.go
# Inspect ecosystem definitions and helpers
sed -n '1,220p' types/type_id.go
# Find where effectiveEcosystems is built/ordered
rg -n -C3 'effectiveEcosystems|hasServer|hasMcdr|PrimaryRuntime.Identity.Eco' cmd/cmd_status.goRepository: mclucy/lucy
Length of output: 8470
Logo selection skips plugin-only ecosystems. logoEco only comes from modding offers, Minecraft, or MCDR, so compatible Paper/Bukkit/Velocity/BungeeCord servers can end up with no logo at all. Fall back to the first compatible offer or the primary runtime eco before leaving it unspecified.
🤖 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 `@cmd/cmd_status.go` around lines 97 - 113, Update the logo selection logic
around logoEco so compatible plugin-only ecosystems are supported: after the
modding-offer search, fall back to the first compatible offer, then to
data.Server.PrimaryRuntime.Identity.Eco when available, before leaving logoEco
as EcoUnspecified. Preserve the existing Minecraft and MCDR fallbacks and use
the existing compatibility and runtime state.
Comment on lines
+11
to
+16
| if evidence == nil || | ||
| evidence.PrimaryRuntime == nil || | ||
| evidence.PrimaryRuntime.PackageRef == (types.PackageRef{}) || | ||
| evidence.PrimaryPath == "" { | ||
| return UnknownExecutable | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find callers of materializeRuntimeInfo and uses of UnknownExecutable
rg -nP -C5 '\bmaterializeRuntimeInfo\s*\(' --type=go
rg -nP -C5 '\bUnknownExecutable\b' --type=go
# Look for mutations on ServerInstance fields
ast-grep run --pattern '$X.Packages = $_' --lang go
ast-grep run --pattern '$X.RuntimeComponents = $_' --lang go
ast-grep run --pattern '$X.PrimaryRuntime = $_' --lang goRepository: mclucy/lucy
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant definitions and nearby code
rg -n -C 4 'materializeRuntimeInfo|UnknownExecutable|type ServerInstance|Packages|RuntimeComponents|PrimaryRuntime' workspace --type go
# Show the file containing materializeRuntimeInfo with line numbers
sed -n '1,120p' workspace/workspace_runtime_materialize.go
# Show the type definition for ServerInstance and the sentinel
sed -n '1,120p' workspace/workspace_runtime_types.goRepository: mclucy/lucy
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the flow where ServerInstance values are passed around and mutated.
sed -n '1,120p' workspace/workspace_interpretation.go
sed -n '1,220p' workspace/workspace.go
sed -n '1,120p' workspace/workspace_runtime_types.go
# Find any writes to ServerInstance fields in the workspace package.
rg -n --type=go '\.(Packages|RuntimeComponents|PrimaryRuntime)\s*=' workspaceRepository: mclucy/lucy
Length of output: 10891
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect methods that could read Packages from the shared sentinel.
rg -n -C 3 'func \(s \*ServerInstance\) EffectiveEcosystems|Packages' workspace/workspace_runtime_types.go workspace/workspace_server_instance_ecosystems.go workspace/workspace_server_instance_cores.go
# Show the ecosystem projection implementation.
sed -n '1,240p' workspace/workspace_server_instance_ecosystems.goRepository: mclucy/lucy
Length of output: 1914
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check all references to the sentinel and whether any code reads its fields directly.
rg -n -C 2 'UnknownExecutable' workspace --type=go
# Inspect the build path that assigns and finalizes runtime state.
sed -n '220,290p' workspace/workspace.go
# Inspect the ecosystem projection to see whether Packages is used only after validity checks.
sed -n '1,220p' workspace/workspace_server_instance_ecosystems.goRepository: mclucy/lucy
Length of output: 7355
Return a fresh instance for unknown executables
finalizeProbedRuntime() writes Packages back onto any non-nil runtime, so returning the shared UnknownExecutable sentinel here mutates global state and can leak data into later calls. Return a new &ServerInstance{} instead.
🤖 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 `@workspace/workspace_runtime_materialize.go` around lines 11 - 16, Update
finalizeProbedRuntime’s unknown-evidence path to return a newly allocated empty
ServerInstance instead of the shared UnknownExecutable sentinel. Preserve the
existing validation conditions and behavior for valid runtime evidence.
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
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.
Closes #66
Summary by CodeRabbit
lucy topologycommand references.