[Tidehyc]

Fixes #3076

Currently, include.Optional is only checked in NewNode. For remote Taskfiles, the offline cache lookup happens later in Node.Read(). When running with --offline and an optional remote include is not cached locally, Node.Read() fails with a cache miss (*errors.TaskfileCacheNotFoundError), so include() in reader.go exits with code 106 instead of skipping it.

In this PR, I updated include() in reader.go to check the local cache before adding the node to the graph or recursing. When include.Optional is true, offline mode is enabled, and includeNode is a RemoteNode, it calls NewCacheNode(...).Read() first and returns nil if the error is os.ErrNotExist. Checking this upfront avoids leaving unused vertices in the DAG and makes sure cache miss errors from nested non-optional remote includes aren't swallowed by an outer optional: true parent.

I added unit tests and testdata in TestIncludesRemoteOptionalOfflineCacheMiss covering both direct and nested optional: true / optional: false cases in offline mode, and ran ./taskfile/... tests and golangci-lint.

AI Disclosure: I used AI tools to help find the root cause, draft the changes in reader.go, and set up the unit tests. I reviewed and tested all the code changes myself, and hand-wrote this PR description without AI.

• I have read and followed the Contribution Guide.
• I have disclosed the use of any AI-generated content in this pull request per the AI Usage Policy.
• I fully understand the changes and have hand-written the description (No AI) of this pull request.

[vmaerten]

Hi!

Thanks for the PR!

The description feels quite AI-generated, especially the very structured sections and some of the wording. Of course, we can't really tell for sure.

I also noticed that the checklist item was reworded and no longer mentions "No AI".

Just a reminder that our contribution guidelines require PR descriptions to be written by hand, without AI assistance.

Could you please rewrite the description in your own words? It doesn't need to be that detailed or formal, just explain what the issue is and how your changes fix it.

Thanks!

[Tidehyc]

Sorry about that! Because English is not my first language, I originally wrote down my rough notes and used an AI tool to translate and format them into a structured description—which is why I mistakenly thought it counted as writing it myself, and why the checklist text got altered when I copied the formatted markdown over.
I now understand that the policy requires the text itself to be written directly by hand without AI formatting or polishing. I have rewritten the description in my own words and restored the exact checklist items from the template. Please take another look when you have time!

[vmaerten]

A couple of things on the implementation:

A cache miss anywhere in the subtree skips the whole optional branch

r.include() is recursive, so the error caught at reader.go:221 can come from a nested include, not only from includeNode itself. For example, with --offline:

# Taskfile.yml
includes:
  opt:
    taskfile: ./opt.yml
    optional: true

# opt.yml
includes:
  remote:
    taskfile: https://example.com/Taskfile.yml # not optional, not cached

The cache miss for remote bubbles up, include.Optional (from opt) is true, and opt's vertex is removed. So a non-optional include is silently skipped. Also, the vertex for remote stays in the graph with a nil Taskfile and no edges. Since TopologicalSort seeds its queue from a map iteration, Merge() may then pick that vertex as hashes[0] and return a nil root Taskfile. And if opt has another include that succeeded, it already has an edge, so the removal fails and the error is returned instead: the outcome depends on sibling includes.

Could we check the cache before recursing instead of removing the vertex afterwards?

Adding the vertex and then removing it on failure is what causes both the dangling vertex in (1) and the race described in "Known Limitations" (which can also surface as a vertex not found error from AddEdge in the other goroutine). Checking the cache up front, the same way we already skip optional includes when NewNode fails, would avoid touching the graph at all:

if include.Optional && r.offline {
        if remote, ok := includeNode.(RemoteNode); ok {
                if _, err := NewCacheNode(remote, r.tempDir).Read(); errors.Is(err, os.ErrNotExist) {
                        return nil
                }
        }
}

This only looks at includeNode itself, so it would also fix (1), and the vertex removal could go away entirely.

Tests

• Could you add a test for the nested case above?
• Nit: rather than matching on the error message, errors.As on *errors.TaskfileCacheNotFoundError would be more robust.

Comments

The comment block is quite long for the code it describes, and "The failed read already added a vertex" isn't accurate: the vertex is added by include() before the read. With (2), most of it goes away anyway; a one-liner explaining why we skip on a cache miss would be enough.

[Tidehyc]

Good catch on the nested include case! I added a test for an optional: true local Taskfile including a non-optional uncached remote Taskfile, and reproduced the exact issue on the old code (it either swallowed the error or panicked depending on map iteration).
I switched to your suggested approach and pre-checked NewCacheNode(...).Read() with errors.Is(err, os.ErrNotExist) before adding the vertex or recursing. The old post-recursion catch, vertex cleanup, and errors.AsType check are all gone now.
I also added two nested subtests to TestIncludesRemoteOptionalOfflineCacheMiss and updated the PR description. PTAL when you have time!

[vmaerten]

This approach looks good to me! A few small things left before merging:

• The comments in the test cases can go, the subtest names already say it.
• "nested optional include is skipped" goes through the exact same code path as "optional include is skipped" (the check is per include, whatever the depth), so I think it can be dropped along with its testdata.
• The remaining subtests are nearly identical: could you make it a table-driven test (dir, wantErr), like TestIncludesRemote?

[Tidehyc]

Thanks for the review! I've updated the PR with those changes:

• Removed the comments in the test.
• Removed the nested optional subtest and its testdata since it's the same path as the direct optional case.
• Changed the remaining subtests to a table-driven test (dir, wantErr) like TestIncludesRemote.
• Shortened the reader.go comment to the one-liner you suggested.
• Renamed the testdata folder to includes_remote_optional_parent_offline.
  Pushed to the branch and the tests are passing locally. Let me know if this looks good to merge!

[vmaerten]

Thanks for the contribution!