Skip to content

REGRESSIONS in the #365-#369 fix pass: the stale-ATOC guard deletes an append-mode sign record, #366 is narrowed not closed, and #369's rewrite broke three other branches #373

Description

@alpCaner

Found by adversarially reviewing the #365-#369 fix pass

feat/v06-batch. The fixes for #365/#366/#367/#368/#369 are in the working tree and their suite is green (157 passed; the "17 failed before" reproduces). These are defects the fix pass INTRODUCED or left open. Two carry hardware consequences.


1. BLOCKER — #365's guard deletes an APPEND-mode sign record, and this repo already documents it as one

python/tan/core/setools.py:314-318 now os.removes build/app-package-map.txt and build/AppTocPackage.bin before spawning app-gen-toc, so their reappearance proves this run wrote them.

But python/tan/core/flash_plan.py:1198-1201 states, citing measured bench scripts:

last match wins: a re-signed re-run APPENDS a fresh block rather than truncating the file, per scripts/bench/aen/flash-jlink.sh / flash-jlink-mramxip.sh / flash-update-log-dual.sh

That is why parse_atoc_start_address takes the last match and why the bench awk ends | tail -1. app-package-map.txt is not per-run scratch — it is the accumulated sign record for that install, including hand-runs done outside tan. The new comment at setools.py:310-313 treats it as an equal per-run output of app-gen-toc. The two claims are in the same repo about the same file and contradict each other.

Failure scenario. A manifest with two Flow D entries. Entry A sets flash_args.atoc_map to <SETOOLS_DIR>/build/app-package-map.txt — where the documented pre-#353 hand-sign workflow leaves it — plus its own flash_args.atoc. Entry B carries nothing signing-related, so it auto-signs. The os.remove wipes the map; app-gen-toc recreates it holding only B's block; entry A's _resolve_flow_d_atoc_address then parses B's address and pairs it with A's blob. A mismatched ATOC/placement written into on-die MRAM — the re-provision-over-SE-UART case. And if app-gen-toc then fails, the entire prior record is gone with nothing to regenerate it.

Fix. #365's own first acceptance option — record st_mtime_ns + size before, refuse if unchanged — detects the soft failure equally well on an append-mode file, because an appended block changes both, and destroys nothing. If the unlink is kept at all, scope it to AppTocPackage.bin and stat the map. Either way setools.py:310-313 and flash_plan.py:1198-1201 must stop contradicting each other.


2. MAJOR — #369's remediation rewrite replaced the shared fallback for four branches and deleted correct advice

python/tan/commands/flash_cmd.py:1199-1209 is the fallback for the wrong-DP-ID case and for unrecognised-banner, target-level cannot-connect, and wrong-jlink_serial (_CONNECT_FAILED_TARGET_RE at :1248 matches Cannot connect to J-Link, so that banner falls through here too).

The old sentence — "If jlink_serial is unset the script selects NO probe, which on a host carrying more than one J-Link cannot connect at all (tan-cli#353)" — was deleted. It now survives only as a code comment at flash_plan.py:1485-1492 and reaches no customer.

Failure scenario. A bench with three J-Links, flash_args.jlink_serial unset, expect_dpidr + jlink_device armed. The preflight script emits no SelectEmuBySN; JLinkExe answers Connecting to J-Link via USB...FAILED: Cannot connect to J-Link.; this fallback fires and tells the customer "do NOT treat pinning flash_args.jlink_serial alone as the fix" — when SelectEmuBySN is precisely the fix.

#369 scoped itself explicitly: "#312 itself is respected... Only this remediation sentence is wrong" — the wrong-DP-ID branch.

Corollary, and it is the defect #367 was filed over, reintroduced in the PR that fixes it: python/tests/commands/test_flash_command.py:1153-1166, test_flow_d_preflight_a_wrong_jlink_serial_keeps_the_wiring_message, whose docstring reads "a real probe-selection problem, so this keeps the original wiring/jlink_serial remediation", now asserts a message saying the opposite.

Fix. Branch: keep the probe-selection sentence for the connect-failure and unrecognised-banner cases; use the cloned-serial / SW-DP-ID text only where a different DP ID was actually reported.


3. MAJOR — #366 is narrowed, not closed

The setools_preview early return at flash_cmd.py:1042-1050 still bypasses meta.build, so everything plan_alif_mram_jlink validates beyond validate_flow_d_shape remains unchecked on a SETOOLS-resolving --dry-run. #366's acceptance criterion was that the dry-run path falls through to meta.build.

Measured:

flash_args: {jlink_flash_device: PART_PROFILE, slot0_load_address: "0x80010000",
             jlink_speed: "fast"}
SETOOLS_DIR set, tan flash --format json --dry-run
  -> ok=True  exit=0  status=ok   "would sign ...\build\zephyr.bin with SETOOLS at ..."

same manifest + atoc/atoc_address supplied (so meta.build runs)
  -> ok=False exit=1  status=failed
     flash_args.jlink_speed must be a bare number (unquoted; got String("fast"))
     -- refusing to silently fall back to a default -- this plans a real flash write.

confirm: "true" (quoted) reproduces identically. On the real run that same manifest spawns app-gen-toc, deletes the map and blob, and writes into the customer's SETOOLS install before meta.build refuses on confirm.

(#366's .hex reproduction is fixed — measured ok=False exit=1, message names zephyr.hex.)

Fix. Make the preview a marker on the entry and let it fall through to meta.build, or hoist the remaining fa_*_checked reads into validate_flow_d_shape.


4. MINOR — is_elf_artefact is narrower than #367(a) specified

#367(a) said "no extension, .elf, .out". flash_plan.py:1222-1232 accepts .elf only, so output_artefact: app (no extension) or app.out with a real sibling app.bin now refuses where it previously resolved. The docstring calls .elf "The one shape #353/#367 agreed is safe"#367 named three. State the narrowing and why, or accept the other two.

5. MINOR — #368's README criterion is unmet, and the precedence flip is invisible on success

grep -i setools README.md returns nothing, and docs/setools.md is referenced from exactly one place: the --setools-dir help string (flash_cmd.py:1539). A customer still discovers the license-gated tool only by failing first.

SetoolsSource.source reaches the customer only via missing_tool_message (a failure) or the --dry-run preview — so a customer with a working flash_args.setools_dir and a stale SETOOLS_DIR in their shell now signs against the stale install and is never told which won.

No ## [0.6.0] — Unreleased CHANGELOG entry for either behaviour change (the .hex refusal, the precedence flip), in a section that documents this wave in detail.

6. MINOR — tan doctor does not know about --setools-dir

python/tan/commands/doctor_cmd.py:2771 reads only os.environ["SETOOLS_DIR"], so a customer configured via the new highest-precedence flag gets tan doctor reporting $SETOOLS_DIR is unset. And :1071 still emits "does not look like an app-release-exec-linux directory" — the verdict-without-evidence phrasing #369 removed from setools.py, one file over.

7. NIT — two overclaims and a slip

  • setools.py:280-285: "their reappearance below can only be explained by THIS run of app-gen-toc" is false under two concurrent tan flash processes sharing one SETOOLS_DIR — the paths are install-wide and fixed.
  • docs/setools.md:20-25 calls app-package-map.txt one of "the executables tan ... look for". tan looks for app-gen-toc only; a report file is not an executable.
  • commander_path (flash_plan.py:826-838): a path containing a double quote renders broken; a path ending in a backslash escapes the closing quote. Both yield a SEGGER parse error under -ExitOnError 1, never a mis-write — bounded, but SETOOLS diagnosis names a conclusion it did not check, never tries app-gen-toc.exe, and an unquoted path with a space reaches J-Link Commander #369 offered refusing at resolve time as the alternative.

Test gaps

No test for the residual dry-run asymmetry (item 3); none for resolve_slot0_binary on a no-extension or .out artefact; none that jlink_commander_script (the swd_probe generator, also changed) quotes a spaced path.

Related: #365, #366, #367, #368, #369, #353, #312.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions