James/builtin#2911
Open
jycor wants to merge 7 commits into
Open
Conversation
Contributor
|
|
SummaryThis run covered core database metadata behavior, including normal built-in type and catalog resolution flows plus edge cases around name resolution ambiguity and object lookup consistency. Overall health is mostly stable on happy-path metadata queries, but upgrade-related lookup behavior shows a compatibility gap. Not safe to merge yet — there is a PR-attributable high-severity failure indicating a regression in upgrade/compatibility behavior, with silent not-found outcomes in a critical lookup path. Even with other covered behaviors passing, this class of break is a merge blocker because it can disrupt existing workflows without clear errors. Tests run by Ito
Tip Reply with @itoqa to send us feedback on this test run. |
![itoqa[bot]](https://avatars.githubusercontent.com/in/2622614?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pre-upgrade rootobject hash workflows regress after behavioral branch deletion
What failed: The upgraded GetTableHash path exits early with found=false when the table map lookup is empty, so root-object-backed names no longer produce hashes and no explicit migration error is returned.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: Upgraded environments can silently fail to resolve hashes for root-object-backed names, breaking existing sequence and extension workflows. Affected users may see these compatibility-dependent operations stop working until the hash lookup behavior is restored or an explicit migration path is provided.
- Steps to Reproduce:
- Create a root-object-backed name such as a sequence (for example, regression_test_seq) so it exists in root objects but not in the table map.
- Call GetTableHash for that same name on the upgraded code path.
- Observe that GetTableHash returns (hash.Hash{}, false, nil) while HasTable still resolves the same name through rootobject.ResolveName.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: In /tmp/output-agent-workspace/repo/core/rootvalue.go, GetTableHash now returns immediately at line 411 after an empty table-map lookup. The subsequent root-object fallback block at lines 413-429 is unreachable, while HasTable (lines 495-515) still resolves root objects through rootobject.ResolveName. This creates a direct behavior regression where existing root-object names can be visible to existence checks but omitted by hash retrieval.
- Why this is likely a bug: The code now has a direct inconsistency between existence checks and hash retrieval for the same root-object-backed name, producing silent not-found results where earlier behavior resolved the object. The smallest practical fix is to restore a guarded root-object fallback in GetTableHash or return an explicit migration-safe error instead of a silent found=false return.
Relevant code
core/rootvalue.go:397-411
func (root *RootValue) GetTableHash(ctx context.Context, tName doltdb.TableName) (hash.Hash, bool, error) {
// Check the tables first
tableMap, err := root.getTableMap(ctx, tName.Schema)
if err != nil {
return hash.Hash{}, false, err
}
tVal, err := tableMap.Get(ctx, tName.Name)
if err != nil {
return hash.Hash{}, false, err
}
if !tVal.IsEmpty() {
return tVal, true, nil
}
return hash.Hash{}, false, nil
}core/rootvalue.go:412-429
// Then check the root objects
_, rawID, objID, err := rootobject.ResolveName(ctx, root, tName)
if err != nil {
return hash.Hash{}, false, err
}
if objID == objinterface.RootObjectID_None {
return hash.Hash{}, false, nil
}
coll, err := rootobject.LoadCollection(ctx, root, objID)
if err != nil {
return hash.Hash{}, false, err
}
obj, ok, err := coll.GetRootObject(ctx, rawID)
if err != nil || !ok {
return hash.Hash{}, false, err
}
h, err := obj.HashOf(ctx)
return h, err == nil && !h.IsEmpty(), errcore/rootvalue.go:495-515
func (root *RootValue) HasTable(ctx context.Context, tName doltdb.TableName) (bool, error) {
// Check the tables first
tableMap, err := root.st.GetTablesMap(ctx, root.vrw, root.ns, tName.Schema)
if err != nil {
return false, err
}
a, err := tableMap.Get(ctx, tName.Name)
if err != nil {
return false, err
}
if !a.IsEmpty() {
return true, nil
}
// Then check the root objects
_, _, objID, err := rootobject.ResolveName(ctx, root, tName)
if err != nil {
return false, err
}
return objID != objinterface.RootObjectID_None, nil
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**High severity — Pre-upgrade rootobject hash workflows regress after behavioral branch deletion**
**What failed:** The upgraded GetTableHash path exits early with found=false when the table map lookup is empty, so root-object-backed names no longer produce hashes and no explicit migration error is returned.
- **Impact:** Upgraded environments can silently fail to resolve hashes for root-object-backed names, breaking existing sequence and extension workflows. Affected users may see these compatibility-dependent operations stop working until the hash lookup behavior is restored or an explicit migration path is provided.
- **Steps to reproduce:**
1. Create a root-object-backed name such as a sequence (for example, regression_test_seq) so it exists in root objects but not in the table map.
2. Call GetTableHash for that same name on the upgraded code path.
3. Observe that GetTableHash returns (hash.Hash{}, false, nil) while HasTable still resolves the same name through rootobject.ResolveName.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** In /tmp/output-agent-workspace/repo/core/rootvalue.go, GetTableHash now returns immediately at line 411 after an empty table-map lookup. The subsequent root-object fallback block at lines 413-429 is unreachable, while HasTable (lines 495-515) still resolves root objects through rootobject.ResolveName. This creates a direct behavior regression where existing root-object names can be visible to existence checks but omitted by hash retrieval.
- **Why this is likely a bug:** The code now has a direct inconsistency between existence checks and hash retrieval for the same root-object-backed name, producing silent not-found results where earlier behavior resolved the object. The smallest practical fix is to restore a guarded root-object fallback in GetTableHash or return an explicit migration-safe error instead of a silent found=false return.
**Relevant code:**
`core/rootvalue.go:397-411`
~~~go
func (root *RootValue) GetTableHash(ctx context.Context, tName doltdb.TableName) (hash.Hash, bool, error) {
// Check the tables first
tableMap, err := root.getTableMap(ctx, tName.Schema)
if err != nil {
return hash.Hash{}, false, err
}
tVal, err := tableMap.Get(ctx, tName.Name)
if err != nil {
return hash.Hash{}, false, err
}
if !tVal.IsEmpty() {
return tVal, true, nil
}
return hash.Hash{}, false, nil
}
~~~
`core/rootvalue.go:412-429`
~~~go
// Then check the root objects
_, rawID, objID, err := rootobject.ResolveName(ctx, root, tName)
if err != nil {
return hash.Hash{}, false, err
}
if objID == objinterface.RootObjectID_None {
return hash.Hash{}, false, nil
}
coll, err := rootobject.LoadCollection(ctx, root, objID)
if err != nil {
return hash.Hash{}, false, err
}
obj, ok, err := coll.GetRootObject(ctx, rawID)
if err != nil || !ok {
return hash.Hash{}, false, err
}
h, err := obj.HashOf(ctx)
return h, err == nil && !h.IsEmpty(), err
~~~
`core/rootvalue.go:495-515`
~~~go
func (root *RootValue) HasTable(ctx context.Context, tName doltdb.TableName) (bool, error) {
// Check the tables first
tableMap, err := root.st.GetTablesMap(ctx, root.vrw, root.ns, tName.Schema)
if err != nil {
return false, err
}
a, err := tableMap.Get(ctx, tName.Name)
if err != nil {
return false, err
}
if !a.IsEmpty() {
return true, nil
}
// Then check the root objects
_, _, objID, err := rootobject.ResolveName(ctx, root, tName)
if err != nil {
return false, err
}
return objID != objinterface.RootObjectID_None, nil
}
~~~
|
Diff SummaryThis run covered core database behavior across normal startup/restart settings, table compatibility and rename flows, and edge-case name lookup paths for tables and other schema objects. Healthy results in the happy-path persistence and table-edit operations are offset by failures in adversarial unresolved-name scenarios where lookup and metadata behaviors become unstable. Not safe to merge yet — this PR has multiple attributable failures, including a high-severity regression plus new and still-failing issues in the same lookup/error-handling area. The failures indicate user-facing panic behavior instead of safe not-found handling in core query paths, which is a merge-blocking reliability risk despite the passing functional cases. Tests run by Ito
Tip Reply with @itoqa to send us feedback on this test run. |
| if err != nil { | ||
| return hash.Hash{}, false, err | ||
|
|
||
| var resColl objinterface.Collection |
There was a problem hiding this comment.
🔁 Regression: previously passing at 6646a91
HasTable and GetTableHash diverge and panic on root-object-backed names
What failed: GetTableHash can call resColl.GetRootObject(ctx, rawID) when resColl was never set, which triggers a nil pointer dereference instead of returning a controlled not-found result; this is inconsistent with HasTable returning true for the same root-object-backed name.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: Operations that request table hashes for sequence-backed names can crash with a nil-pointer panic, interrupting normal database workflows.
- Steps to Reproduce:
- Create a root-object-backed name that is not present in the table map, such as a sequence (for example root_case.my_seq).
- Confirm existence behavior succeeds through HasTable-equivalent operations (for example nextval/setval on the sequence).
- Run a query path that resolves through GetTableHash for that same name (for example SELECT * FROM root_case.my_seq).
- Observe panic recovery with stack trace pointing to core/rootvalue.go:434 in GetTableHash.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: In core/rootvalue.go, the PR-changed GetTableHash implementation (around lines 412-434) loads collections and iterates ResolveName results, but then unconditionally calls resColl.GetRootObject(ctx, rawID). If no collection match is assigned to resColl, this dereferences nil and panics at line 434. The PR diff for core/rootvalue.go shows these changed lines replacing the previous fallback behavior (
return hash.Hash{}, false, niland ResolveName/object-ID checks), so the changed code path is the direct causal path for the regression. Smallest practical fix: add a guard before GetRootObject (return not found when no valid match exists), and preserve explicit not-found handling parity with HasTable for unresolved names. - Why this is likely a bug: The runtime stack trace lands on GetTableHash line 434, and the source shows an unconditional dereference of resColl after a loop that may leave it nil. A direct nil guard and explicit not-found return is required for coherent behavior with HasTable.
Relevant code
core/rootvalue.go:419-437
var resColl objinterface.Collection
rawID := id.Null
for _, coll := range root.colls {
_, rID, err := coll.ResolveName(ctx, tName)
if err != nil {
return hash.Hash{}, false, err
}
if !rID.IsValid() {
continue
}
rawID = rID
resColl = coll
break
}
obj, ok, err := resColl.GetRootObject(ctx, rawID)
if err != nil || !ok {
return hash.Hash{}, false, err
}core/rootvalue.go:506-525
func (root *RootValue) HasTable(ctx context.Context, tName doltdb.TableName) (bool, error) {
// Check the tables first
tableMap, err := root.st.GetTablesMap(ctx, root.vrw, root.ns, tName.Schema)
...
if !a.IsEmpty() {
return true, nil
}
_, _, objID, err := rootobject.ResolveName(ctx, root, tName)
if err != nil {
return false, err
}
return objID != objinterface.RootObjectID_None, nil
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**High severity — HasTable and GetTableHash diverge and panic on root-object-backed names**
**What failed:** GetTableHash can call resColl.GetRootObject(ctx, rawID) when resColl was never set, which triggers a nil pointer dereference instead of returning a controlled not-found result; this is inconsistent with HasTable returning true for the same root-object-backed name.
- **Impact:** Operations that request table hashes for sequence-backed names can crash with a nil-pointer panic, interrupting normal database workflows.
- **Steps to reproduce:**
1. Create a root-object-backed name that is not present in the table map, such as a sequence (for example root_case.my_seq).
2. Confirm existence behavior succeeds through HasTable-equivalent operations (for example nextval/setval on the sequence).
3. Run a query path that resolves through GetTableHash for that same name (for example SELECT * FROM root_case.my_seq).
4. Observe panic recovery with stack trace pointing to core/rootvalue.go:434 in GetTableHash.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** In core/rootvalue.go, the PR-changed GetTableHash implementation (around lines 412-434) loads collections and iterates ResolveName results, but then unconditionally calls resColl.GetRootObject(ctx, rawID). If no collection match is assigned to resColl, this dereferences nil and panics at line 434. The PR diff for core/rootvalue.go shows these changed lines replacing the previous fallback behavior (`return hash.Hash{}, false, nil` and ResolveName/object-ID checks), so the changed code path is the direct causal path for the regression. Smallest practical fix: add a guard before GetRootObject (return not found when no valid match exists), and preserve explicit not-found handling parity with HasTable for unresolved names.
- **Why this is likely a bug:** The runtime stack trace lands on GetTableHash line 434, and the source shows an unconditional dereference of resColl after a loop that may leave it nil. A direct nil guard and explicit not-found return is required for coherent behavior with HasTable.
**Relevant code:**
`core/rootvalue.go:419-437`
~~~go
var resColl objinterface.Collection
rawID := id.Null
for _, coll := range root.colls {
_, rID, err := coll.ResolveName(ctx, tName)
if err != nil {
return hash.Hash{}, false, err
}
if !rID.IsValid() {
continue
}
rawID = rID
resColl = coll
break
}
obj, ok, err := resColl.GetRootObject(ctx, rawID)
if err != nil || !ok {
return hash.Hash{}, false, err
}
~~~
`core/rootvalue.go:506-525`
~~~go
func (root *RootValue) HasTable(ctx context.Context, tName doltdb.TableName) (bool, error) {
// Check the tables first
tableMap, err := root.st.GetTablesMap(ctx, root.vrw, root.ns, tName.Schema)
...
if !a.IsEmpty() {
return true, nil
}
_, _, objID, err := rootobject.ResolveName(ctx, root, tName)
if err != nil {
return false, err
}
return objID != objinterface.RootObjectID_None, nil
}
~~~
| if err != nil { | ||
| return hash.Hash{}, false, err | ||
|
|
||
| var resColl objinterface.Collection |
There was a problem hiding this comment.
🆕 New Failure: identified in this diff run
Unresolved root-object lookup triggers nil pointer panic
What failed: The unresolved-name branch does not guard against an unassigned collection pointer before calling GetRootObject, so the request panics instead of returning not found.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Minor
- Impact: Some users may see the ROOT-3 flow stall instead of finishing, requiring a retry.
- Steps to Reproduce:
- Start the PR build of doltgres and connect to a test database.
- Issue a query path that calls GetTableHash for a schema-qualified name that is missing from the table map and all root-object collections (for example, CREATE TABLE in a fresh schema).
- Observe the returned panic stack trace referencing core/rootvalue.go:434 instead of a controlled not-found style result.
- Run a follow-up query like SELECT 1 to confirm the process recovered while the original operation failed.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: In GetTableHash, the PR-introduced logic iterates root.colls and sets resColl only when ResolveName returns a valid ID. When no collection matches, resColl remains nil but the function still calls
resColl.GetRootObject(ctx, rawID)at line 434. That nil-interface method call is the direct cause of the panic seen in evidence. The smallest practical fix is to add a guard after the loop (for example, returnhash.Hash{}, false, nilwhen no valid ID was found) before dereferencing resColl. - Why this is likely a bug: Runtime evidence shows repeated panic traces at core/rootvalue.go:434 on unresolved-name queries, and source inspection confirms a nil dereference path when no collection resolves the name. The expected behavior for this branch is a controlled not-found result, not a panic-recovery error path.
Relevant code
core/rootvalue.go:419-435
var resColl objinterface.Collection
rawID := id.Null
for _, coll := range root.colls {
_, rID, err := coll.ResolveName(ctx, tName)
if err != nil {
return hash.Hash{}, false, err
}
if !rID.IsValid() {
continue
}
rawID = rID
resColl = coll
break
}
obj, ok, err := resColl.GetRootObject(ctx, rawID)Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Minor severity — Unresolved root-object lookup triggers nil pointer panic**
**What failed:** The unresolved-name branch does not guard against an unassigned collection pointer before calling GetRootObject, so the request panics instead of returning not found.
- **Impact:** Some users may see the ROOT-3 flow stall instead of finishing, requiring a retry.
- **Steps to reproduce:**
1. Start the PR build of doltgres and connect to a test database.
2. Issue a query path that calls GetTableHash for a schema-qualified name that is missing from the table map and all root-object collections (for example, CREATE TABLE in a fresh schema).
3. Observe the returned panic stack trace referencing core/rootvalue.go:434 instead of a controlled not-found style result.
4. Run a follow-up query like SELECT 1 to confirm the process recovered while the original operation failed.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** In GetTableHash, the PR-introduced logic iterates root.colls and sets resColl only when ResolveName returns a valid ID. When no collection matches, resColl remains nil but the function still calls `resColl.GetRootObject(ctx, rawID)` at line 434. That nil-interface method call is the direct cause of the panic seen in evidence. The smallest practical fix is to add a guard after the loop (for example, return `hash.Hash{}, false, nil` when no valid ID was found) before dereferencing resColl.
- **Why this is likely a bug:** Runtime evidence shows repeated panic traces at core/rootvalue.go:434 on unresolved-name queries, and source inspection confirms a nil dereference path when no collection resolves the name. The expected behavior for this branch is a controlled not-found result, not a panic-recovery error path.
**Relevant code:**
`core/rootvalue.go:419-435`
~~~go
var resColl objinterface.Collection
rawID := id.Null
for _, coll := range root.colls {
_, rID, err := coll.ResolveName(ctx, tName)
if err != nil {
return hash.Hash{}, false, err
}
if !rID.IsValid() {
continue
}
rawID = rID
resColl = coll
break
}
obj, ok, err := resColl.GetRootObject(ctx, rawID)
~~~
| if err != nil { | ||
| return hash.Hash{}, false, err | ||
|
|
||
| var resColl objinterface.Collection |
There was a problem hiding this comment.
🆕 New Failure: identified in this diff run
Repeated unresolved-name probes trigger recovered panics in GetTableHash
What failed: Unresolved-name lookups do not fail safely; each probe triggers a recovered runtime panic (nil pointer dereference) instead of returning a controlled not-found outcome.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Minor
- Impact: Some root-account sessions may appear stuck after the flow completes, requiring a refresh or restart before users can continue.
- Steps to Reproduce:
- Start doltgres and connect with psql to the test database.
- Issue repeated CREATE TABLE statements using unresolved names (for example root_probe_a_xyz_1 through root_probe_a_xyz_8).
- Observe each probe returning a recovered panic referencing core/rootvalue.go:434, then run SELECT 1 to confirm the process is still alive.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: In core/rootvalue.go, the PR changed GetTableHash to iterate root.colls and removed the earlier immediate not-found return when table-map lookup is empty. The new loop only assigns resColl on a successful ResolveName match, but line 434 unconditionally calls resColl.GetRootObject after the loop. When no collection resolves the name, resColl remains nil and the dereference panics, matching the runtime stack traces from ROOT-4.
- Why this is likely a bug: The expected behavior for unresolved names is a controlled not-found response, but the current code path dereferences a nil collection and panics on each probe. A minimal fix is to add a no-match guard before GetRootObject (for example return hash.Hash{}, false, nil when no valid collection was found).
Relevant code
core/rootvalue.go:419-435
var resColl objinterface.Collection
rawID := id.Null
for _, coll := range root.colls {
_, rID, err := coll.ResolveName(ctx, tName)
if err != nil {
return hash.Hash{}, false, err
}
if !rID.IsValid() {
continue
}
rawID = rID
resColl = coll
break
}
obj, ok, err := resColl.GetRootObject(ctx, rawID)Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Minor severity — Repeated unresolved-name probes trigger recovered panics in GetTableHash**
**What failed:** Unresolved-name lookups do not fail safely; each probe triggers a recovered runtime panic (nil pointer dereference) instead of returning a controlled not-found outcome.
- **Impact:** Some root-account sessions may appear stuck after the flow completes, requiring a refresh or restart before users can continue.
- **Steps to reproduce:**
1. Start doltgres and connect with psql to the test database.
2. Issue repeated CREATE TABLE statements using unresolved names (for example root_probe_a_xyz_1 through root_probe_a_xyz_8).
3. Observe each probe returning a recovered panic referencing core/rootvalue.go:434, then run SELECT 1 to confirm the process is still alive.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** In core/rootvalue.go, the PR changed GetTableHash to iterate root.colls and removed the earlier immediate not-found return when table-map lookup is empty. The new loop only assigns resColl on a successful ResolveName match, but line 434 unconditionally calls resColl.GetRootObject after the loop. When no collection resolves the name, resColl remains nil and the dereference panics, matching the runtime stack traces from ROOT-4.
- **Why this is likely a bug:** The expected behavior for unresolved names is a controlled not-found response, but the current code path dereferences a nil collection and panics on each probe. A minimal fix is to add a no-match guard before GetRootObject (for example return hash.Hash{}, false, nil when no valid collection was found).
**Relevant code:**
`core/rootvalue.go:419-435`
~~~go
var resColl objinterface.Collection
rawID := id.Null
for _, coll := range root.colls {
_, rID, err := coll.ResolveName(ctx, tName)
if err != nil {
return hash.Hash{}, false, err
}
if !rID.IsValid() {
continue
}
rawID = rID
resColl = coll
break
}
obj, ok, err := resColl.GetRootObject(ctx, rawID)
~~~
| if err != nil { | ||
| return hash.Hash{}, false, err | ||
|
|
||
| var resColl objinterface.Collection |
There was a problem hiding this comment.
🆕 New Failure: identified in this diff run
Nil dereference in table hash fallback
What failed: Expected unresolved names to fail safely and consistently across existence and hash-retrieval flows, but GetTableHash panics on unresolved paths while existence checks return coherent results.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Users hitting ambiguous cross-collection names can trigger deterministic failures in status/log metadata queries instead of receiving a controlled not-found response. This breaks core repository-inspection workflows for affected names until objects are renamed.
- Steps to Reproduce:
- Create a colliding root-object name (for example a sequence and function both named collide_root6).
- Run internal hash-driven queries such as SELECT * FROM DOLT_STATUS;, SELECT message FROM DOLT_LOG LIMIT 1;, or a pg_proc join lookup for that name.
- Observe recovered panic errors referencing core/rootvalue.go:434 instead of a controlled not-found style response.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: I inspected core/rootvalue.go and the PR diff in pr-context.json. The PR replaced the older path that returned not found or resolved via rootobject helpers with a new loop over root.colls. In the new code, resColl starts nil, is set only when ResolveName returns a valid ID, and line 434 calls resColl.GetRootObject unconditionally. For unresolved names, the loop exits with resColl still nil, causing the observed panic. The smallest practical fix is to add a no-match guard before line 434 (for example if !rawID.IsValid() || resColl == nil { return hash.Hash{}, false, nil }) so unresolved lookups return a controlled result.
- Why this is likely a bug: The failure is reproducible across multiple queries and sessions and maps to a concrete nil-dereference path in production code, not to harness behavior. The current implementation violates the expected safe not-found behavior for unresolved names by panicking on a reachable no-match branch.
Relevant code
core/rootvalue.go:419-435
var resColl objinterface.Collection
rawID := id.Null
for _, coll := range root.colls {
_, rID, err := coll.ResolveName(ctx, tName)
if err != nil {
return hash.Hash{}, false, err
}
if !rID.IsValid() {
continue
}
rawID = rID
resColl = coll
break
}
obj, ok, err := resColl.GetRootObject(ctx, rawID)Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Nil dereference in table hash fallback**
**What failed:** Expected unresolved names to fail safely and consistently across existence and hash-retrieval flows, but GetTableHash panics on unresolved paths while existence checks return coherent results.
- **Impact:** Users hitting ambiguous cross-collection names can trigger deterministic failures in status/log metadata queries instead of receiving a controlled not-found response. This breaks core repository-inspection workflows for affected names until objects are renamed.
- **Steps to reproduce:**
1. Create a colliding root-object name (for example a sequence and function both named collide_root6).
2. Run internal hash-driven queries such as SELECT * FROM DOLT_STATUS;, SELECT message FROM DOLT_LOG LIMIT 1;, or a pg_proc join lookup for that name.
3. Observe recovered panic errors referencing core/rootvalue.go:434 instead of a controlled not-found style response.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** I inspected core/rootvalue.go and the PR diff in pr-context.json. The PR replaced the older path that returned not found or resolved via rootobject helpers with a new loop over root.colls. In the new code, resColl starts nil, is set only when ResolveName returns a valid ID, and line 434 calls resColl.GetRootObject unconditionally. For unresolved names, the loop exits with resColl still nil, causing the observed panic. The smallest practical fix is to add a no-match guard before line 434 (for example if !rawID.IsValid() || resColl == nil { return hash.Hash{}, false, nil }) so unresolved lookups return a controlled result.
- **Why this is likely a bug:** The failure is reproducible across multiple queries and sessions and maps to a concrete nil-dereference path in production code, not to harness behavior. The current implementation violates the expected safe not-found behavior for unresolved names by panicking on a reachable no-match branch.
**Relevant code:**
`core/rootvalue.go:419-435`
~~~go
var resColl objinterface.Collection
rawID := id.Null
for _, coll := range root.colls {
_, rID, err := coll.ResolveName(ctx, tName)
if err != nil {
return hash.Hash{}, false, err
}
if !rID.IsValid() {
continue
}
rawID = rID
resColl = coll
break
}
obj, ok, err := resColl.GetRootObject(ctx, rawID)
~~~
|
Diff SummaryThis run exercised core database name-resolution and object-lookup behavior across normal workflows, compatibility scenarios, repeated and concurrent access, and missing-name edge cases, with healthy behavior in the validated paths. Overall, the change area looks stable for both standard operations and stress-style lookup patterns, including graceful handling of not-found conditions. Safe to merge — no failures were attributed to this PR, with prior problem areas in this scope now passing and no PR-linked regressions or newly introduced breakages detected. There is one minor additional finding that appears pre-existing and not caused by this change, so it is a flag for later follow-up rather than a merge blocker. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. ⚪ Ambiguous root-object name resolves to first-match hash
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
Contributor
Footnotes
|
Commit: SummaryThis run exercised core database behavior across everyday and edge-path workflows, including relation lookup semantics, schema isolation and iteration, startup variable persistence and recovery, and type/catalog visibility. Overall health is strong for the PR scope, with expected flows and resilience checks behaving as intended under both normal and adversarial inputs. Safe to merge — the run shows no regressions, no new PR-attributable failures, and no previously flagged PR failures still failing. The remaining issues are medium-severity but pre-existing product defects outside this change, so they are follow-up items rather than merge blockers for this PR. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Schema-qualified rename loses table resolution
Evidence Package🟡 Schema boundary key gets dropped
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
Commit: SummaryThis run exercised database behavior for normal object metadata and built-in data type resolution, along with edge-case rename flows across schemas and transaction boundaries. The core lookup and type behaviors were healthy, and the remaining rename consistency problems appear to be existing product issues rather than effects of this change. Safe to merge — no regressions, new failures, or previously flagged PR-linked failures were attributed to this PR in this run. The remaining failures are high-severity but pre-existing rename/transaction behavior issues that should be tracked separately, not treated as a merge blocker for this change. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟠 Schema-qualified rename fails when source table is outside search_path
Evidence Package🟠 Rename persists after rollback in transaction
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.