mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 15:05:40 +00:00
fix: the remap must preserve bytes; round-18 web findings (TASK-2878)
Three changes: one is a regression I introduced two commits ago and the full suite caught, two are codex round 18 (web + internal/models). 1. remapFieldIDs PRESERVES EVERY BYTE IT DOES NOT DELIBERATELY CHANGE.3e7a6751fixed the substring-corruption defect by unmarshalling the fields blob, walking it, and re-marshalling. That fixed the defect and broke TestImportRepairsANULInsideAFieldsBlob: re-encoding rewrites the WHOLE blob, so a stored `�` ESCAPE came back as the literal replacement CHARACTER. stored fields = "{\"note\":\"x<U+FFFD>y\"}", want "{\"note\":\"x\\ufffdy\"}" The rule I broke is one my own harness memory already carries in another costume: an escape sequence and the character it denotes are not the same artifact, and a round-trip through a decoder silently converts one into the other. A function whose job is to substitute ids must not be the thing that re-encodes everything else. Now a textual substitution of the QUOTED JSON token — `"c-1"` does not occur inside `"c-10"`, which is the whole-value property the fix needed, and it still reaches ids inside arrays for a multi-valued relation. Untouched bytes are untouched. WORTH KEEPING: my four targeted tests all passed on the broken build. Only the full SQLite suite failed, in a test named for NUL handling that has nothing to do with relations, in a package I was not editing. This is the argument for running the whole gate rather than the tests I judged relevant — the fix and the test that catches it were three directories apart. Mutant: replace the quoted token with the raw id -> FAIL, the dangling value is rewritten. Both the prefix-collision test and the NUL test are green on this tip. 2. THE `referent_not_portable` MESSAGE ASSERTED SOMETHING THE RESPONSE DOES NOT SAY (codex round 18). The dialog rendered "it points at something in the source workspace". That reason is emitted for EVERY carried cross-workspace relation WITHOUT resolving the target, so the value may name a live item, a deleted one, one the caller cannot see, or nothing at all — and `github_pr` reaches the same reason, where the referent is not in any workspace. The sentence claimed both existence and location. Now "this reference cannot be carried to the destination". Same class as round 12's `not_found` overclaim, and the neutral-wording comment explaining why THAT one is careful sits four lines below the one that was not. A rule written next to its own exception is easy to read as already applied everywhere it belongs. SHIPS WITHOUT A TEST, named here rather than left looking covered: the reason-to-message mapper is inline in the .svelte component and not exported, and there is no test file for the dialog at all. Extracting it is a refactor this unit should not take mid-flight — filed as an idea instead, because two separate review rounds have now found defects in this one unexported function and nothing can regression-test either fix. 3. THE WEB `Item` TYPE NOW MIRRORS `models.ItemWriteWarnings` (codex round 18). The Go item has carried `warnings` on create/update responses since BUG-2850, and TASK-2878 added `dropped_fields` and `unresolved_relations` to it. Nothing on the TypeScript side mirrored any of it, so typed frontend code could not read a warning the server was already sending. Additive and fully optional, matching the Go struct's `omitempty` on every member. Gates on this tip: build, go vet, gofmt, `npm run check` 0 errors (6 pre-existing warnings in unrelated files), `make web-test` 124 files / 2108 tests passed. The full SQLite suite and Postgres are owed on this tip and are NOT claimed here — the SQLite run on3e7a6751is the one that failed above. Claude-Session: https://claude.ai/code/session_01Xk9M5UVPdc84xL5E1mZkm8
This commit is contained in:
xarmian
+25
-47
@@ -4,6 +4,7 @@ import (
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"strings"
|
||||
"time"
|
||||
"unicode/utf8"
|
||||
|
||||
@@ -851,59 +852,36 @@ func remapFieldIDs(fieldsJSON string, itemMap, collMap map[string]string) string
|
||||
}
|
||||
// WHOLE-VALUE matches only, never a substring (codex round 17).
|
||||
//
|
||||
// This was `strings.ReplaceAll` over the raw JSON text for every id in the
|
||||
// bundle, which corrupts a value that merely CONTAINS one: with a bundle
|
||||
// carrying ids `c-1` and a relation value `"c-10"`, the value came out as
|
||||
// `<new-id-for-c-1>0` — a string that references nothing and that the
|
||||
// importer then stored as though it were the user's data. Bundle ids are
|
||||
// whatever the exporting instance had and an import accepts a
|
||||
// This was `strings.ReplaceAll` over the RAW id, which corrupts a value
|
||||
// that merely CONTAINS one: with a bundle carrying ids `c-1` and a
|
||||
// relation value `"c-10"`, the value came out as `<new-id-for-c-1>0` — a
|
||||
// string that references nothing and existed on neither side. Bundle ids
|
||||
// are whatever the exporting instance had and an import accepts a
|
||||
// caller-supplied file, so this is not confined to well-formed UUIDs.
|
||||
//
|
||||
// It matters more since the carry posture: an unresolvable relation value
|
||||
// is deliberately IMPORTED VERBATIM rather than dropped, and "verbatim" is
|
||||
// the whole promise. A partial rewrite breaks it silently and produces a
|
||||
// value that never existed on either side.
|
||||
// It matters since the carry posture: an unresolvable relation value is
|
||||
// deliberately imported VERBATIM rather than dropped, and verbatim is the
|
||||
// whole promise. A partial rewrite breaks it silently.
|
||||
//
|
||||
// The same text substitution also rewrote ids appearing inside ordinary
|
||||
// text values, which was never the intent — the remap exists to repoint
|
||||
// RELATIONS at their clones. Whole-value matching gets both.
|
||||
// Matching the QUOTED JSON token is what makes it whole-value: `"c-1"`
|
||||
// does not occur inside `"c-10"`, and the form still reaches ids inside
|
||||
// arrays, which a multi-valued relation needs.
|
||||
//
|
||||
// DELIBERATELY NOT an unmarshal/marshal walk. That was this fix's first
|
||||
// version and the full suite caught it: re-encoding rewrites the WHOLE
|
||||
// blob, so a stored `\ufffd` escape came back as the literal replacement
|
||||
// character and broke the NUL-repair import test, which asserts on the
|
||||
// stored bytes. Every byte this function does not deliberately change has
|
||||
// to survive it, and only a textual substitution guarantees that.
|
||||
//
|
||||
// I engineered a fixture AROUND this hazard in the import carry test
|
||||
// rather than asking whether the product had it. It did.
|
||||
var decoded map[string]any
|
||||
if err := json.Unmarshal([]byte(fieldsJSON), &decoded); err != nil {
|
||||
// Not an object we can walk. Left exactly as-is rather than
|
||||
// substring-rewritten: an unparseable blob is not a thing to guess at.
|
||||
return fieldsJSON
|
||||
}
|
||||
for k, v := range decoded {
|
||||
decoded[k] = remapFieldValue(v, itemMap)
|
||||
}
|
||||
out, err := json.Marshal(decoded)
|
||||
if err != nil {
|
||||
return fieldsJSON
|
||||
}
|
||||
return string(out)
|
||||
}
|
||||
|
||||
// remapFieldValue rewrites a value that IS an old item id, and recurses into
|
||||
// arrays so a multi-valued relation is covered. Anything else — a number, a
|
||||
// bool, a nested object, a string that merely contains an id — is returned
|
||||
// unchanged.
|
||||
func remapFieldValue(v any, itemMap map[string]string) any {
|
||||
switch t := v.(type) {
|
||||
case string:
|
||||
if newID, ok := itemMap[t]; ok && t != "" && newID != "" {
|
||||
return newID
|
||||
result := fieldsJSON
|
||||
for oldID, newID := range itemMap {
|
||||
if oldID == "" || newID == "" {
|
||||
continue
|
||||
}
|
||||
return t
|
||||
case []any:
|
||||
out := make([]any, len(t))
|
||||
for i, e := range t {
|
||||
out[i] = remapFieldValue(e, itemMap)
|
||||
}
|
||||
return out
|
||||
default:
|
||||
return v
|
||||
result = strings.ReplaceAll(result, `"`+oldID+`"`, `"`+newID+`"`)
|
||||
}
|
||||
return result
|
||||
}
|
||||
|
||||
@@ -947,8 +947,18 @@ user hunting for an item that provably does not exist.
|
||||
// so it rendered through the fallback as the raw enum string. Rare
|
||||
// while only `github_pr` produced it; routine since TASK-2878, which
|
||||
// emits it for every carried relation value on a cross-workspace copy.
|
||||
//
|
||||
// NEUTRAL WORDING, for the same reason `not_found` below has it,
|
||||
// and it took a second reviewer to see that the same rule applied
|
||||
// here (codex round 18). This reason is emitted for EVERY carried
|
||||
// cross-workspace relation WITHOUT resolving the target, so the
|
||||
// value may name a live item, a deleted one, one the caller cannot
|
||||
// see, or nothing at all — and `github_pr` reaches it too, where
|
||||
// the referent is not in any workspace. "It points at something in
|
||||
// the source workspace" asserted both existence and location, and
|
||||
// the response says neither.
|
||||
case 'referent_not_portable':
|
||||
return 'it points at something in the source workspace';
|
||||
return 'this reference cannot be carried to the destination';
|
||||
// The three same-workspace referent failures (TASK-2878). Worth
|
||||
// separate sentences: "no such item" and "wrong collection" send the
|
||||
// reader to different fixes, and a missing target is a schema problem
|
||||
|
||||
@@ -696,6 +696,36 @@ export interface Item {
|
||||
convention?: ItemConventionMetadata;
|
||||
implementation_notes?: ItemImplementationNote[];
|
||||
decision_log?: ItemDecisionLogEntry[];
|
||||
/**
|
||||
* Advisory notes about a write that SUCCEEDED. Present on create and
|
||||
* update responses only — never on a read, and never stored — so
|
||||
* `undefined` is the normal case and says nothing went unreported.
|
||||
*
|
||||
* The Go side has carried this since BUG-2850 and TASK-2878 added two
|
||||
* more members; nothing here mirrored it, so typed code could not reach
|
||||
* a warning the server was already sending (codex round 18).
|
||||
*/
|
||||
warnings?: ItemWriteWarnings;
|
||||
}
|
||||
|
||||
/**
|
||||
* Mirrors `models.ItemWriteWarnings`. Every member is optional because the Go
|
||||
* struct marks each `omitempty`: an absent key means "nothing to report for
|
||||
* this kind", not "unknown".
|
||||
*/
|
||||
export interface ItemWriteWarnings {
|
||||
/** Field keys stored in the blob that the collection's schema does not declare (BUG-2850). */
|
||||
undeclared_fields?: string[];
|
||||
/** Keys the write DISCARDED — a relation whose referent did not resolve, a value the destination schema has no home for (TASK-2878). */
|
||||
dropped_fields?: string[];
|
||||
/**
|
||||
* Keys KEPT with a value that does not resolve to a live item. Import only:
|
||||
* an import must carry junk relation values rather than refuse them, or no
|
||||
* artifact written before referent validation could be imported. Distinct
|
||||
* from `dropped_fields` — the value survives — and from `undeclared_fields`,
|
||||
* which is about the key rather than what it points at.
|
||||
*/
|
||||
unresolved_relations?: string[];
|
||||
}
|
||||
|
||||
// ─── Items index (skinny projection) ─────────────────────────────────────────
|
||||
|
||||
Reference in New Issue
Block a user