Conversation
This required updating to use the new `derivationAttrPath` instead of `derivation`.
📝 WalkthroughWalkthroughRefactors Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flake.nix`:
- Around line 91-111: The code dereferences attrs.derivationAttrPath,
attrs.derivation and fields on drv (drv.drvPath, drv.outputs,
drv.${outputName}.outPath) without guarding, which can throw and make
includingOutputPaths fail; fix by validating before access (e.g. use
builtins.hasAttr / builtins.tryEval or builtins.isDerivation where available) so
drv is only treated as a derivation when it actually has drvPath and outputs,
and when mapping outputs check each outputName exists on drv before reading
drv.${outputName}.outPath; update the branches that compute drv and the
derivation/outputs results to return null/nullable metadata when validation
fails instead of performing direct dereferences.
| drv = | ||
| if attrs?derivationAttrPath | ||
| then getAttrFromPath attrs.derivationAttrPath output | ||
| # TODO: remove when no longer needed | ||
| else if attrs?derivation | ||
| then attrs.derivation | ||
| else null; | ||
| in | ||
| { | ||
| derivation = if drv != null then drv.drvPath else null; | ||
| outputs = | ||
| if attrs ? derivation | ||
| then | ||
| if drv != null then | ||
| builtins.listToAttrs | ||
| ( | ||
| builtins.map | ||
| (outputName: | ||
| { | ||
| name = outputName; | ||
| value = attrs.derivation.${outputName}.outPath; | ||
| } | ||
| ) | ||
| attrs.derivation.outputs | ||
| ) | ||
| else | ||
| null; | ||
| (outputName: { | ||
| name = outputName; | ||
| value = drv.${outputName}.outPath; | ||
| }) | ||
| drv.outputs | ||
| ) else null; |
There was a problem hiding this comment.
Guard derivation path dereferences to prevent full inventory evaluation failure.
Line 93, Line 100, and Line 108 can throw when a path is stale or resolves to a non-derivation value. That makes includingOutputPaths fail hard instead of returning nullable metadata for the problematic node.
🛡️ Suggested hardening patch
- drv =
- if attrs?derivationAttrPath
- then getAttrFromPath attrs.derivationAttrPath output
+ drv =
+ if attrs ? derivationAttrPath
+ then try (getAttrFromPath attrs.derivationAttrPath output) null
# TODO: remove when no longer needed
- else if attrs?derivation
+ else if attrs ? derivation
then attrs.derivation
else null;
in
{
- derivation = if drv != null then drv.drvPath else null;
+ derivation = try (if drv != null then drv.drvPath else null) null;
outputs =
- if drv != null then
- builtins.listToAttrs
- (
- builtins.map
- (outputName: {
- name = outputName;
- value = drv.${outputName}.outPath;
- })
- drv.outputs
- ) else null;
+ try (
+ if drv != null then
+ builtins.listToAttrs
+ (
+ builtins.map
+ (outputName: {
+ name = outputName;
+ value = drv.${outputName}.outPath;
+ })
+ drv.outputs
+ )
+ else null
+ ) null;
}📝 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.
| drv = | |
| if attrs?derivationAttrPath | |
| then getAttrFromPath attrs.derivationAttrPath output | |
| # TODO: remove when no longer needed | |
| else if attrs?derivation | |
| then attrs.derivation | |
| else null; | |
| in | |
| { | |
| derivation = if drv != null then drv.drvPath else null; | |
| outputs = | |
| if attrs ? derivation | |
| then | |
| if drv != null then | |
| builtins.listToAttrs | |
| ( | |
| builtins.map | |
| (outputName: | |
| { | |
| name = outputName; | |
| value = attrs.derivation.${outputName}.outPath; | |
| } | |
| ) | |
| attrs.derivation.outputs | |
| ) | |
| else | |
| null; | |
| (outputName: { | |
| name = outputName; | |
| value = drv.${outputName}.outPath; | |
| }) | |
| drv.outputs | |
| ) else null; | |
| drv = | |
| if attrs ? derivationAttrPath | |
| then try (getAttrFromPath attrs.derivationAttrPath output) null | |
| # TODO: remove when no longer needed | |
| else if attrs ? derivation | |
| then attrs.derivation | |
| else null; | |
| in | |
| { | |
| derivation = try (if drv != null then drv.drvPath else null) null; | |
| outputs = | |
| try ( | |
| if drv != null then | |
| builtins.listToAttrs | |
| ( | |
| builtins.map | |
| (outputName: { | |
| name = outputName; | |
| value = drv.${outputName}.outPath; | |
| }) | |
| drv.outputs | |
| ) | |
| else null | |
| ) null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flake.nix` around lines 91 - 111, The code dereferences
attrs.derivationAttrPath, attrs.derivation and fields on drv (drv.drvPath,
drv.outputs, drv.${outputName}.outPath) without guarding, which can throw and
make includingOutputPaths fail; fix by validating before access (e.g. use
builtins.hasAttr / builtins.tryEval or builtins.isDerivation where available) so
drv is only treated as a derivation when it actually has drvPath and outputs,
and when mapping outputs check each outputName exists on drv before reading
drv.${outputName}.outPath; update the branches that compute drv and the
derivation/outputs results to return null/nullable metadata when validation
fails instead of performing direct dereferences.
1 participant
This required updating to use the new
derivationAttrPathinstead ofderivation.I've tested with both the 0.1.4 version and the latest, but I've kept the version bump since we might as well.
Summary by CodeRabbit