Added --simulate-json option to write the simulated change set as JSON

[djbclark]

Added --simulate-json option to write the simulated change set as JSON

The changes computed by a --simulate run could only be rendered as
prose meant for human eyes: the manifest and diff renderers print
free-form text, and the record files in the changes chroot are an
internal, unstable format. Any program consuming the simulated change
set had to parse text that can be reworded at any time.

With --simulate-json=FILE, cf-agent also writes the change set to FILE
as a single JSON document: which files would be created, modified or
deleted (with the type, permissions, ownership, size and SHA-256
content digest they would have after the run), which files would be
renamed, and which packages would be installed or removed. The
document carries a format_version field so that consumers can detect
future changes to its structure. The option requires --simulate and an
absolute path. The prose renderers are unchanged and remain the
default; without the new option, nothing changes.

The reduction of the recorded package operations to the net set of
install and remove operations is shared with the --simulate=diff prose
renderer: DiffPkgOperations() now renders its messages from the
reduced records at printing time instead of storing pre-rendered
messages in them.

The document is written before GenericAgentFinalize() because
computing the content digests needs the crypto (OpenSSL) library,
which is deinitialized there.

Requested in #6296.

[CLAassistant]

All committers have signed the CLA.

[djbclark]

Apologies for the force-push churn on this branch — the commit metadata is now correct, and this is what moved and why.

The commit originally carried Ticket: #6296, referring to the discussion this PR implements. I removed that trailer yesterday after convincing myself the number was invented, because GET /repos/cfengine/core/issues/6296 returned 404. That was my mistake: issues are disabled on this repository, #6296 is a Discussion, and the issues endpoint cannot see one. The reference was right all along.

So Ticket: #6296 is restored, and I have added the Changelog: Title line it should have had alongside it per CONTRIBUTING — the title is already a past-tense, user-facing description of the change. I have also added a "Requested in #6296" line to the PR description, which was missing that link entirely.

No code changed in either direction. The tree is byte-identical to what you have already seen; both rewrites touched only the commit message, and I verified the tree hashes match before pushing. Still one commit.

[djbclark]

Please hold off merging this one too — the same review found defects here, and a correction is coming. Nothing as severe as the overflow in #6293, but enough that it should not go in as written.

1. A functional bug that also affects the existing prose output. CollectPkgOperations() (cf-agent/simulate_mode.c:872–877) sets name_arch = NULL immediately before passing it to MapRemove(removed, name_arch), so the removal is a no-op. A remove-then-install sequence is therefore reported as both an install and a remove. The comment on that line, the commit body, and #6296 all state that an installation cancels a previous removal; it does not. This reaches --simulate=diff package output as well as the JSON.

2. The JSON is not safely machine-readable for non-ASCII filenames. Filenames are emitted through a per-byte \u00XX escape, so a conforming parser (json.loads, jq) decodes a different byte sequence than the path on disk — mojibake. That is the specific failure a machine-readable change set exists to prevent. The unit test does not catch it because it round-trips through this codebase's own decoder rather than a standard one, so both sides share the same non-conformance.

3. High UIDs are reported as negative. uid_t/gid_t are cast to int, so a file owned by nobody (uid 4294967294 here) reports "uid": -2.

4. A failed JSON write does not fail the run. WriteChangesJson logs at ERR and the process still exits 0, which breaks the CI-gate use that #6296 describes — the file can be absent while the run reports success. An existing output file is also silently truncated, and the open follows a same-owner symlink.

5. HashFile() is called unchecked, so a hash failure yields an all-zero digest that is still emitted as a sha256 field. That is a live consumer of the defect I reported separately in libntech (NorthernTechHQ/libntech#290); the fix there makes the failure logged, but this would still publish the zero digest as if it were real.

6. One claim in the commit text is too strong as written. "The prose renderers are unchanged" is right at file level — I had that measured independently and it holds — but DiffPkgOperations() was rewritten, so package prose now renders from CollectPkgOperations() at print time. Worth saying precisely, given item 1.

Also housekeeping: tests/acceptance/29_simulate_mode/simulate_json.cf:26 still contains a literal CFE-XXXX placeholder.

I'll follow up with a force-push carrying the fixes and tests.

[djbclark]

Now tracked in Jira as CFE-4716.

Same as #6293: we couldn't reach the CFE tracker when this PR went up, which is why the commit trailer points at discussion #6296. That's now resolved (thanks to @nickanderson — the Atlassian account alone wasn't enough, I also needed permission in the project).

The Ticket: trailer will move from #6296 to CFE-4716 in the same force-push as the correction, and the acceptance test's literal CFE-XXXX placeholder now has a real key to use. The hold-off-merging note stands until that lands.

Going forward all our CFEngine/libntech reports will go to Jira rather than email or new discussions.

[djbclark]

The fixes are up — this supersedes the hold-off-merging notice above. Force-pushed to b3a6c3d.

All seven defects from that notice are fixed, plus an eighth instance of one of them that the review missed.

what was wrong	fix
CollectPkgOperations() set name_arch = NULL before MapRemove(removed, name_arch), so the removal was never removed and a remove-then-install sequence reported both operations — contradicting what this PR's own message and the linked discussion claim. This escaped into --simulate=diff prose, not only JSON	MapRemove() moved above the conditional insert. Unit case r,pkg,,\r\ni,pkg,1.0,\r\n now asserts exactly one install and no remove
Non-ASCII path bytes were emitted as per-byte \u00XX. Well-formed JSON, but \u00XX denotes code point U+00XX, so a conformant parser decodes each byte of a multi-byte character as a separate wrong character — café did not survive the round trip	escapes that form a valid UTF-8 sequence are rewritten to raw bytes; bytes that do not stay escaped, since the document must itself be valid UTF-8 and there is no exact representation for them
uid_t/gid_t cast to int, so uid 4294967294 (the usual nobody on Linux) printed as -2	written as a 64-bit integer
the output file was truncated up front, a same-owner symlink at the destination was followed, and a failed write still exited 0 — a consumer would see success and no document	written to a temporary file created O_EXCL, then rename()d over the destination (matching SaveAsFile()'s existing discipline); rename replaces a symlink rather than following it; failure now exits non-zero
HashFile()'s result was unchecked. It cannot report failure and zeroes the digest instead, so 64 zeros were presented as a real SHA-256	the all-zero digest is detected, the sha256 field is omitted, and an error is logged. An absent field is an honest "unknown"; a zeroed digest is not
the message claimed "the prose renderers are unchanged"	rewritten — with the package-cancellation fix, that is provably false
CFE-XXXX placeholder in the acceptance test	now CFE-4716

The eighth defect: ManifestPkgOperations() contains the identical dead-MapRemove pattern, so --simulate=manifest prose reported a package as both absent and present for the same sequence, contradicting its own "Cancels any previous remove/absent message" comment. It is pre-existing in master and all three reviewers walked past it. Fixed here in one hunk, on the reasoning that anyone who greps for the pattern after reading the first fix will find it in seconds — happy to split it into its own PR if you would rather keep this one to what was reviewed.

Two things you should look at with a critical eye

The UTF-8 fix is in the wrong repo, deliberately. The root cause is libntech's JsonEncodeStringWriter(), which escapes every non-ASCII byte individually; every JsonWrite() consumer has this problem, not just this one. Fixing it there would change output for all of them and belongs in its own review, so this is kept local to the change-set writer. We are filing the libntech defect separately and will reference it here. If you would rather have the libntech fix and a much smaller diff in core, say so and we will do it that way instead.

One acceptance .expected change is a masked-defect removal, not cosmetic. The fixture file WORKDIR/tmp/perms is mode 0000, so an unprivileged run hashed nothing, emitted 64 zeros, and the normalizer's sed rewrote them to the constant SHA256 — the test passed because of the bug in the previous bullet. With the digest failure now honest, the field's presence is privilege-dependent, so the normalizer drops sha256 lines and the 12 masked lines are gone from .expected. Digest correctness is asserted exactly, against real digest values, by the unit tests instead, which is strictly stronger than the constant the acceptance test was matching.

Tests

simulate_mode_test.c goes from 13 to 17 cases, including assertions on the raw on-disk bytes (Ã absent, raw UTF-8 present, a lone 0xE9 still escaped) — JsonParseFile cannot detect the encoding bug, because libntech's decoder reverses its own non-conformant escaping, which is why the shipped test could not fail.

Discrimination was measured, not assumed: with the two source files stashed and the test file kept, test_special_characters_in_path, test_pkg_operations_cancel and test_output_symlink_not_followed all fail (binary rc=3). Two of the new cases pass either way and are noted as such — test_write_failure cannot observe the main() half from a unit test.

Full suite All 69 tests behaved as expected (4 expected failures), make -j2 clean with no new warnings, and 29_simulate_mode/simulate_json.cf passes. The sibling prose tests (manifest_mode, manifest_full_mode, diff_mode) fail on our macOS host for an environmental reason unrelated to this diff — their .expected hard-codes Uid: (0/root) and there is no fakeroot on macOS; they fail identically on an unmodified checkout.

Round-trip evidence for the encoding fix, via a conformant decoder rather than CFEngine's own:

$ python3 -c "import json; d = json.load(open('changes.json')); ..."
decoded path: .../wéird café.txt
roundtrip_matches_original_name: True

The Ticket: trailer is now CFE-4716 (https://northerntech.atlassian.net/browse/CFE-4716), replacing the discussion reference.

[djbclark]

Following up on the libntech point above: the root cause is filed as CFE-4730.

Investigating it for that ticket turned up a second, more serious half that this PR does not touch, so flagging it here for whoever reviews the core workaround. libntech does not only write per-byte \u00XX — it also misreads standard \uXXXX escapes, silently. HexStringToChar() rejects any code point above U+00FF (libutils/json.c:1063), and the case 'u': arm then breaks without advancing the cursor (:1108-1120), so the enclosing for (...; c++) lands on the u and copies the escape through as literal text with the backslash dropped:

$ cat data.json
{"city": "中国"}                       # 中国 on disk

$ python3 -c "import json; print(json.load(open('data.json')))"
{'city': '中国'}

$ cf-promises -f p.cf --show-vars       # "d" data => readjson("data.json", 4096)
default:main.city   u4e2du56fd

That is reachable from readjson(), parsejson(), augments and CMDB host_specific.json — any JSON produced by tooling that escapes non-ASCII, which is most of it.

The two halves are exact inverses, which is why nothing caught either: libntech round-trips its own output perfectly, so any write-then-read test passes while both directions are wrong. That is also why fixing only the writer would be worse than leaving it — libntech would then misread its own files. Details and repro in CFE-4730.

[djbclark]

Following up on CFE-4730, the libntech root cause behind this PR's RestoreUtf8InJson() workaround: a fix is now offered upstream as NorthernTechHQ/libntech#293.

I want to be precise about how the two interact, because it affects how you may want to review this one.

They do not conflict. GetJsonEscapedByte() here returns false for any value above 0xff, so once libntech emits a correct \u4e2d for 中, this helper simply does not recognise it and the escape passes through untouched. There is no double-processing and no corruption if both land. I read this off the source rather than inferring it from behaviour; I have not executed it, because this branch pins libntech at 5b5d04e1 and rebuilding against the fix was outside what I could verify cleanly. Treat it as a careful reading, not a measurement.

The one real consequence, if libntech#293 is taken and the submodule pin later moves: --simulate=json output would then contain \u4e2d-style escapes rather than raw UTF-8 bytes. That is still correct, conformant JSON — any conformant reader decodes it to the original filename — but this PR's acceptance .expected asserts the raw form, so it would need refreshing at that point.

I am deliberately not removing RestoreUtf8InJson() from this PR. Nothing is decided upstream, libntech#293 may be reshaped or declined, and this PR should stand on its own either way. If you would rather take the libntech fix and drop the ~140-line workaround from here, say so and I will strip it out — that is a smaller and cleaner PR, and I would be glad to make that trade.

Separately, and unrelated to this PR: while fixing the codec I found that libntech's parser double-decodes string escapes — JsonParseAsString() already unescapes, and the call sites then run JsonDecodeString() over the result — so the valid document {"p": "C:\\temp\\new"} parses to C:<TAB>emp<NL>ew. Filed as CFE-4731. It predates both PRs and neither causes nor cures it; flagging it only because it lives in the same file.