feat: search, load and validates manifests #1

upils · 2025-11-26T10:01:54Z

Have you signed the CLA?

Signed-off-by: Paul Mars <paul.mars@canonical.com>

Copilot

Pull request overview

This PR implements functionality to search, load, and validate manifests from a rootfs directory. The main purpose is to enable chisel to detect and reuse existing rootfs installations by reading and validating their manifests.

Key changes:

Added manifest verification logic to validate rootfs contents against manifest entries
Implemented manifest loading and consistency checking across multiple manifest files
Integrated rootfs detection into the cut command to merge existing slice keys with newly requested ones

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
internal/manifestutil/verify.go	New file implementing rootfs validation against manifest entries, including path grouping, hardlink verification, and file integrity checks
internal/manifestutil/manifestutil.go	Extended with functions to find manifests in releases, load and validate manifest files, check consistency between manifests, and extract reports from rootfs
cmd/chisel/cmd_cut.go	Modified to detect pre-existing rootfs and merge slice keys from existing manifests with requested slices

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/manifestutil/verify.go

internal/manifestutil/manifestutil.go

cmd/chisel/cmd_cut.go

Signed-off-by: Paul Mars <paul.mars@canonical.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/manifestutil/verify.go

internal/manifestutil/manifestutil.go

cmd/chisel/cmd_cut.go

Signed-off-by: Paul Mars <paul.mars@canonical.com>

TheSignPainter98

Here are a few things to watch out for :)

cmd/chisel/cmd_cut.go

internal/manifestutil/manifestutil.go

internal/manifestutil/verify.go

Signed-off-by: Paul Mars <paul.mars@canonical.com>

TheSignPainter98

This is looking much better now, thanks for the changes! Just a few minor things then I think this looks good

internal/manifestutil/verify.go

internal/manifestutil/manifestutil.go

Signed-off-by: Paul Mars <paul.mars@canonical.com>

upils · 2026-01-16T08:00:14Z

cmd/chisel/cmd_cut.go

+		if mfest != nil {
+			err = manifestutil.CheckDir(mfest, cmd.RootDir)
+			if err != nil {
+				return err


[Note to reviewer]: At this point a valid manifest was extracted from the root dir but the dir is invalid (or we failed to validate it) so we return the error.

upils · 2026-01-16T08:01:32Z

internal/manifestutil/manifestutil.go

+			}
+			return nil, fmt.Errorf("cannot read manifest %q from the root directory: %v", manifestPath, err)
+		}
+		err = Validate(mfest)


[Note to reviewer]: The manifest validation was moved out of the load function to be called here instead.

upils · 2026-01-16T14:47:29Z

cmd/chisel/cmd_cut.go


+	manifestPaths := manifestutil.FindPathsInRelease(release)
+	if len(manifestPaths) >= 0 {
+		mfest, mfestPath, err := manifestutil.FromDir(manifestPaths, cmd.RootDir)


[Note to reviewer]: I am not very happy about having to also return the manifest path so I am open to suggestions to avoid that.

I am not very happy about having to also return the manifest path so I am open to suggestions to avoid that.

I have the same suggestion as in all the comments above :)

upils · 2026-01-22T09:19:12Z

public/manifest/manifest.go

 	return nil
 }
+
+func (manifest *Manifest) Schema() string {


[Note to reviewer]: This is needed to compare manifests by schema versions but exposing it while the format of this value is not precisely defined might cause issues in the future. Should we hold on exposing and for now internally rely on the assumption that all valid manifests are necessarily using the 1.0 schema?

upils · 2026-01-22T10:02:34Z

internal/slicer/slicer.go

+		}
+		defer r.Close()
+
+		mfest, err = manifest.Read(r)


[Note to reviewer]: This call can return 2 kinds of errors:

Errors actually about reading from the reader, such as the OS unable to read a piece of the file from disk.

Unmarshalling and invalid schema errors. So "business logic" errors.

Ideally I would like to differentiate these errors: return in the first case and continue in the second. But I cannot see a proper solution that would not require changing the API of manifest.Read to do that.

So my approach here is to continue, because in that case this file is not a good fit to get the manifest and the main goal is to get one valid manifest, not identify bad candidates.

invalid schema errors

As I said on MM, I think we named a named error for this one in particular if we want to ignore them. Old versions of Chisel will not parse correctly new versions of the manifest, so it is useful to know if the error is related to schema version and skip. If it is not then fail.

letFunny

Thanks for this Paul, I wrote a lot of comments mainly minor, which is always a good sign :). This is looking much better already. I think we are at a stage where you need to write the tests so that we know how the feature looks like, how it is called and what is implemented.

letFunny · 2026-01-26T09:45:21Z

internal/manifestutil/manifestutil.go

 	return nil
 }
+
+// CompareSchemas compare two manifest schema strings


How does it compare them? What is the return value? This looks a bit artificial at the moment, it is saying:

If you give me two manifests with the same version and it is manifest.Schema then 0.

Else -1.

Which is a little bit weird for a comparator, right? Normally, either -1, 0 and 1 are returned based on a total order or true, false is returned if the order is partial.

It is a little weird on purpose. My goal was to have a signature consistent with other comparison functions in the project and avoid changing it when actually supporting multiple schema versions. The end goal is to provide a total order, but since the schema format is not yet formally defined, I did not want to implement a complex comparison making assumptions (that could be wrong in the future). When we actually support multiple schema, the format will be properly defined and we should naturally change this implementation because it will look obviously wrong.

For now it indeed feels a bit artificial but that may be a good thing(?).

letFunny · 2026-01-26T09:51:57Z

internal/manifestutil/manifestutil.go

+			allSlices = append(allSlices, slice)
+		}
+	}
+	manifestMap := FindPaths(allSlices)


This is a bit unfortunate that we have to iterate over all slices to collect them just to iterate over them again. Because the real business logic takes a path + info and returns the manifest path by validating then appending a filename, what about extracting only that bit into an auxiliary function instead? Then have both functions call the auxiliary one. Thoughts?

After implementing a naive version of FindPathsInRelease I thought the same. I did the extraction and ended up with something longer and harder to understand. So I opted for simplicity at this time but now I think I may have underestimated the problem when in the future the number of slices is more than 50x what we have today. I will rework that.

Reworked in 4ecb555

letFunny · 2026-01-26T09:53:35Z

internal/slicer/check.go

+// the manifest.
+// This function works under the assumption the manifest is valid.
+// Files not managed by chisel are ignored.
+func checkDir(mfest *manifest.Manifest, rootDir string) error {


I think checkRootDir might be a more appropriate name as we expect the directory to be the root dir of the installation that the manifest describes.

I am still not happy with having it here, but let's postpone that discussion. It is not blocking and it is not major (for now if we don't introduce a chisel validate).

I think checkRootDir might be a more appropriate name as we expect the directory to be the root dir of the installation that the manifest describ

Good point, and this is in line with the rootDir argument. Fixed in 15550ad.

I am still not happy with having it here, but let's postpone that discussion. It is not blocking and it is not major (for now if we don't introduce a chisel validate).

I agree.

letFunny · 2026-01-26T09:54:05Z

internal/slicer/check.go

+			inode := stat.Ino
+
+			if path.Inode == 0 {
+				// This path must not be linked to any other


Make sure all comments are punctuated.

See 15550ad

internal/slicer/check.go

letFunny · 2026-01-26T10:15:45Z

internal/slicer/slicer.go

+	targetDir = filepath.Clean(targetDir)
+	if !filepath.IsAbs(targetDir) {
+		dir, err := os.Getwd()
+		if err != nil {
+			return nil, fmt.Errorf("cannot obtain current directory: %w", err)
+		}
+		targetDir = filepath.Join(dir, targetDir)
+	}


You are doing this every time for an internal function. If the function is internal then you can expect an absolute path and you can document it or check with IsAbs and return an error, either one of them work. If the function is public then you have to choice to make it absolute or fail. In that case I prefer the former to follow convention.

letFunny · 2026-01-26T10:16:52Z

internal/slicer/slicer.go

+			defer r.Close()
+			mfest, err := manifest.Read(r)
+			if err != nil {
+				return nil


See my other other comment. Here it is even more important. And manifest.Validate has to be an error.

letFunny · 2026-01-26T10:19:55Z

internal/slicer/slicer.go

+				return nil
+			}
+
+			if selected == nil || manifestutil.CompareSchemas(mfest.Schema(), selected.Schema()) > 0 {


Compare is never > 0. In my opinion, in terms of logic the code should check all manifests of the same version have to be equal. You don't have to use the content has because you have both manifests in memory and you need to return selected anyways, so you can check whether they match or not (in a separate function please).

letFunny · 2026-01-26T10:20:53Z

internal/slicer/slicer.go

+				if !ok {
+					schemaManifest[mfest.Schema()] = manifestHash{mfestPath, mfestHash}
+				} else if refMfest.hash != mfestHash {
+					return fmt.Errorf("inconsistent manifests: %q and %q", refMfest.path, mfestPath)


This will likely change, but this error message doesn't state that the contents of the manifests should be equal and they are not.

public/manifest/manifest.go

upils · 2026-02-04T08:27:57Z

Closing in favor of canonical#264

upils added 5 commits November 26, 2025 11:01

feat: search, load and validates manifests

60320ba

Signed-off-by: Paul Mars <paul.mars@canonical.com>

fix: revert unintentional change

c67c661

Signed-off-by: Paul Mars <paul.mars@canonical.com>

refactor: move things around

22ae256

Signed-off-by: Paul Mars <paul.mars@canonical.com>

refactor: move logic to manifestutil

6cb437f

Signed-off-by: Paul Mars <paul.mars@canonical.com>

feat: validate rootfs with manifest

00825de

Signed-off-by: Paul Mars <paul.mars@canonical.com>

upils marked this pull request as ready for review November 27, 2025 10:45

upils added 6 commits November 27, 2025 13:34

fix: debugging

442bcc6

Signed-off-by: Paul Mars <paul.mars@canonical.com>

fix: improve hardlinks check

e19f6f1

Signed-off-by: Paul Mars <paul.mars@canonical.com>

feat: trying alternative approach

b6ecc27

Signed-off-by: Paul Mars <paul.mars@canonical.com>

feat: refine first approach

1406b18

Signed-off-by: Paul Mars <paul.mars@canonical.com>

refactor: dedicated verify.go

8aba035

Signed-off-by: Paul Mars <paul.mars@canonical.com>

refactor: rework verify* funcs

70b2c5e

Signed-off-by: Paul Mars <paul.mars@canonical.com>

upils requested a review from Copilot December 2, 2025 10:54

Copilot AI reviewed Dec 2, 2025

View reviewed changes

internal/manifestutil/verify.go Outdated Show resolved Hide resolved

internal/manifestutil/manifestutil.go Outdated Show resolved Hide resolved

internal/manifestutil/manifestutil.go Outdated Show resolved Hide resolved

cmd/chisel/cmd_cut.go Outdated Show resolved Hide resolved

fix: typos and inconsistencies

c8b7a39

Signed-off-by: Paul Mars <paul.mars@canonical.com>

upils requested a review from Copilot December 2, 2025 13:13

Copilot AI reviewed Dec 2, 2025

View reviewed changes

upils added 5 commits December 2, 2025 15:46

refactor: improve readability

1181871

Signed-off-by: Paul Mars <paul.mars@canonical.com>

refactor: improve consistency

31f1d3d

Signed-off-by: Paul Mars <paul.mars@canonical.com>

style: respect internal best practices

df49dfc

Signed-off-by: Paul Mars <paul.mars@canonical.com>

style: minor consistency fixes

f4169b9

Signed-off-by: Paul Mars <paul.mars@canonical.com>

fix: ignore size check on manifest

aade454

Signed-off-by: Paul Mars <paul.mars@canonical.com>

TheSignPainter98 suggested changes Dec 5, 2025

View reviewed changes

upils added 5 commits December 8, 2025 08:21

style: apply suggestions

e0bd4a4

Signed-off-by: Paul Mars <paul.mars@canonical.com>

style: apply suggestions

2e572fb

Signed-off-by: Paul Mars <paul.mars@canonical.com>

style: apply suggestions

3c5de1c

Signed-off-by: Paul Mars <paul.mars@canonical.com>

style: apply suggestions

7eff973

Signed-off-by: Paul Mars <paul.mars@canonical.com>

style: apply suggestions

b7c7c66

Signed-off-by: Paul Mars <paul.mars@canonical.com>

TheSignPainter98 suggested changes Dec 15, 2025

View reviewed changes

fix: apply suggestions

c7196e9

Signed-off-by: Paul Mars <paul.mars@canonical.com>

upils added 3 commits January 15, 2026 14:59

refactor: simplify based on PR suggestions

71c4496

style: tweak error messages

4696dcc

fix: fail if dir inconsistent with manifest

089109d

upils commented Jan 16, 2026

View reviewed changes

upils requested a review from letFunny January 16, 2026 08:01

fix: use loaded manifest hash as reference

09faa6a

upils commented Jan 16, 2026

View reviewed changes

upils added 6 commits January 19, 2026 14:00

fix: raise error when path should not be hardlinked

bd7a707

fix: collect and record reference manifest size

3ea70ce

feat: log root directory processing

3efb7d2

refactor: more experiment around API design

a421818

refactor: experiment to improve API

e88de2d

refactor: more refining

acac421

upils commented Jan 22, 2026

View reviewed changes

upils added 8 commits January 22, 2026 14:17

refactor: CheckDir in slicer

0262cb2

refactor: add check in slicer

7445fcd

refactor: simplify and reduce slicer API

5fca0c5

ci: rerun

fb6500b

wip

d7fa155

refactor: cleaning

43e8a71

fix: check manifest consistency at selection

46d2119

refactor: return a manifest from inspection

164c68b

letFunny suggested changes Jan 26, 2026

View reviewed changes

upils added 2 commits January 26, 2026 13:23

refactor: apply PR suggestions

15550ad

refactor: avoid iterating twice on slices

4ecb555

upils closed this Feb 4, 2026

letFunny mentioned this pull request Feb 4, 2026

feat: upgrade existing filesystem canonical/chisel#264

Open

1 task

feat: search, load and validates manifests #1

feat: search, load and validates manifests #1

Uh oh!

Conversation

upils commented Nov 26, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheSignPainter98 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheSignPainter98 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

letFunny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

upils Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

upils Jan 26, 2026 •

edited

Loading