Notes generated by mach cargo vet are unexpectedly deleted by running mach vendor rust

[ashleyz]

Notes fields generated with mach cargo vet are being unexpectedly deleted during mach vendor rust. I discussed with @bholley , who suggested that I file an issue here.

I'm able to reproduce the behavior with the Cargo.toml line from this patch:

+ cubeb-pulse = { git = "https://github.com/mozilla/cubeb-pulse-rs", rev="cf48897be5cbe147d051ebbbe1eaf5fd8fb6bbc9", optional = true, features=["pulse-dlopen"] }

Here's some command output:

# This first command doesn't seem to have an effect, but leaving it in just in case the output is helpful at all
$ ./mach cargo vet
ERROR 
  × 'cargo metadata' exited unsuccessfully
  ╰─▶ `cargo metadata` exited with an error:     Updating git repository `https://github.com/mozilla/cubeb-pulse-rs`
      error: the lock file /home/ashley/git/mozilla-unified/Cargo.lock needs to be updated but --locked was passed to prevent this
      If you want to try to generate the lock file without accessing the network, remove the --locked flag and use --offline instead.

$ ./mach vendor rust
# ...
    Removing cubeb-pulse v0.4.0 (https://github.com/mozilla/cubeb-pulse-rs?rev=79096eb59b0bfcedb660716a67c0de34a25ea29e#79096eb5)                                                                                   
      Adding cubeb-pulse v0.4.1 (https://github.com/mozilla/cubeb-pulse-rs?rev=cf48897be5cbe147d051ebbbe1eaf5fd8fb6bbc9#cf48897b)                                                                                   
    Removing pulse v0.3.0 (https://github.com/mozilla/cubeb-pulse-rs?rev=79096eb59b0bfcedb660716a67c0de34a25ea29e#79096eb5)                                                                                         
      Adding pulse v0.3.0 (https://github.com/mozilla/cubeb-pulse-rs?rev=cf48897be5cbe147d051ebbbe1eaf5fd8fb6bbc9#cf48897b)                                                                                         
    Removing pulse-ffi v0.1.0 (https://github.com/mozilla/cubeb-pulse-rs?rev=79096eb59b0bfcedb660716a67c0de34a25ea29e#79096eb5)                                                                                     
      Adding pulse-ffi v0.1.0 (https://github.com/mozilla/cubeb-pulse-rs?rev=cf48897be5cbe147d051ebbbe1eaf5fd8fb6bbc9#cf48897b)
# ...

# Notes are removed by this point!
$ ./mach cargo vet
Vetting Succeeded (132 fully audited, 70 partially audited, 252 exempted)

# snip from diff output
diff --git a/supply-chain/audits.toml b/supply-chain/audits.toml
index 0ce3dbc2e61e2..efaf9ba795b75 100644
--- a/supply-chain/audits.toml
+++ b/supply-chain/audits.toml
@@ -266,9 +266,6 @@ delta = "0.10.1 -> 0.10.2"
 who = "Paul Adenot <paul@paul.cx>"
 criteria = "safe-to-deploy"
 delta = "0.10.2 -> 0.10.3"
-note = """
-Mozilla-developed package.
-"""
 
 [[audits.cubeb-core]]
 who = "Matthew Gregan <kinetik@flim.org>"
@@ -287,9 +284,6 @@ delta = "0.10.1 -> 0.10.2"
 who = "Paul Adenot <paul@paul.cx>"
 criteria = "safe-to-deploy"
 delta = "0.10.2 -> 0.10.3"
-note = """
-Mozilla-developed package.
-"""

[bholley]

@mystor wdyt?

[mystor]

The notes are disappearing because the key for these comments is note = """ rather than notes = """, so it's not recognized by cargo-vet, and therefore discarded. By default serde discards unrecognized fields when deserializing without an error, and when we re-serialize after the command, it's gone.

Perhaps it's worth having some form of CI check that running ./mach cargo vet --locked --frozen not only doesn't fail, but also doesn't modify audits.toml or config.toml due to incorrect formatting?

[bholley]

Could we override the default to have cargo-vet just fail if there's an unrecognized field? That would result in this issue being caught by the person who added note rather than later by @ashleyz

[mystor]

If the person who added note ran cargo vet, it would've also deleted the notes for them, so the place where it would be caught differently is specifically in CI.

I wonder a bit if perhaps there should be a --check-fmt flag to cargo vet which makes sure that cargo vet succeeds without changing the audits.toml or config.toml files at all.

This feels somewhat similar in some ways to other formatting issues which could be caught after the fact, such as the order of audits being re-written.

[bholley]

That's a good idea. We can introduce that flag and then hook it up to run in CI.

[bholley]

I wonder if we should default to that if you pass --locked, rather than introducing a separate flag that people would need to learn about?

[mystor]

Yeah, I'm unsure, as theoretically one might want to run with --locked locally to prevent fetching new upstream audits, but would still want to be able to write out updated lockfiles. I'm not sure if that's common enough to warrant requiring CI to set a new flag.

[bholley]

My inclination is that we shouldn't anchor on locked just meaning "don't fetch imports" but interpret is more broadly as "we expect that everything is in a green state, please verify".