Strip trailing _query_source column before header validation - #43
Open
Morabbin wants to merge 1 commit into
Open
Strip trailing _query_source column before header validation#43Morabbin wants to merge 1 commit into
Morabbin wants to merge 1 commit into
Conversation
session_store_sql supplements local-store results with a trailing _query_source column that materialize-session-query.py's exact header check rejected with "unexpected discovery result column _query_source". Drop the column in table_rows only when it is genuinely the last column in the header, using a single right-hand split so embedded " | " sequences in earlier values (e.g. arguments_json) are preserved. Unknown extra columns and a non-trailing _query_source are still rejected by the unchanged header equality check. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates session query materialization to accept local-store results containing a trailing _query_source column.
Changes:
- Removes trailing
_query_sourceheaders and corresponding row cells. - Adds coverage for valid, malformed, embedded-pipe, and unexpected-column cases.
Show a summary per file
| File | Description |
|---|---|
scripts/materialize-session-query.py |
Normalizes supplemented query results before schema validation. |
tests/test_materialize_session_query.py |
Tests normalization and rejection edge cases. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
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
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.
Summary
session_store_sqlsupplements results from the local session store with a trailing_query_sourcecolumn.materialize-session-query.pyvalidates result headers against an exact expected column list, so any result carrying that column was rejected with:This blocked Repository Skill Forge discovery whenever the underlying
session_store_sqlresult came from (or was supplemented by) the local store.Context: github/agents#1815.
Scope note: this fixes a local/store result-compatibility bug in the materializer's header validation. It does not address the production seven-day scale issue tracked separately; those are unrelated problems that happened to surface through the same script.
Change
In
table_rows(), drop a trailing_query_sourcecolumn from the header before the per-kind header equality check runs, and strip the corresponding cell from each row with a single right-hand split (row.rsplit(" | ", 1)[0]):This only fires when
_query_sourceis genuinely the last header column, so:_query_sourcecolumn that isn't trailing is still rejected (it doesn't match the expected header once the intended trailing check finds a different last column)." | "sequences in earlier values (e.g.arguments_json) are preserved.Merge conflict note
origin/morabbin-forge-deterministic-workerrefactorsresult_content()intoprobe_result()/materialize_content()in this same file, but it does not touchtable_rows()orparse_rows(). This PR is based onmainand targets onlytable_rows(), so a conflict is unlikely; if one does occur it should be limited to nearby context lines, not overlapping logic.Tests
Added to
tests/test_materialize_session_query.py, exercisingtable_rows()andparse_rows()directly:_query_sourcecolumn (baseline, unchanged behavior)._query_sourcecolumn stripped and parsed correctly._query_source) still rejected._query_sourcepresent but not trailing still rejected." | "inarguments_jsonpreserved through stripping (tool-calls kind)._query_sourceheader._query_sourcecell) raises downstream inparse_rowsinstead of silently misparsing.Validation
No repo-wide lint/format tooling is configured for this repository, so none was run.