Skip to content

Strip trailing _query_source column before header validation - #43

Open
Morabbin wants to merge 1 commit into
mainfrom
morabbin-fix-query-source-column
Open

Strip trailing _query_source column before header validation#43
Morabbin wants to merge 1 commit into
mainfrom
morabbin-fix-query-source-column

Conversation

@Morabbin

Copy link
Copy Markdown

Summary

session_store_sql supplements results from the local session store with a trailing _query_source column. materialize-session-query.py validates result headers against an exact expected column list, so any result carrying that column was rejected with:

unexpected discovery result column _query_source

This blocked Repository Skill Forge discovery whenever the underlying session_store_sql result 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_source column 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]):

if header and header[-1] == "_query_source":
    header = header[:-1]
    rows = [row.rsplit(" | ", 1)[0] for row in rows]

This only fires when _query_source is genuinely the last header column, so:

  • Results without the column are unaffected.
  • Unknown extra columns are still rejected by the unchanged header equality check.
  • A _query_source column that isn't trailing is still rejected (it doesn't match the expected header once the intended trailing check finds a different last column).
  • The right-hand split removes only the final Markdown cell, so embedded " | " sequences in earlier values (e.g. arguments_json) are preserved.

Merge conflict note

origin/morabbin-forge-deterministic-worker refactors result_content() into probe_result() / materialize_content() in this same file, but it does not touch table_rows() or parse_rows(). This PR is based on main and targets only table_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, exercising table_rows() and parse_rows() directly:

  • No _query_source column (baseline, unchanged behavior).
  • Trailing _query_source column stripped and parsed correctly.
  • Unknown trailing column (not _query_source) still rejected.
  • _query_source present but not trailing still rejected.
  • Embedded " | " in arguments_json preserved through stripping (tool-calls kind).
  • Zero rows with a trailing _query_source header.
  • Malformed row (missing the _query_source cell) raises downstream in parse_rows instead of silently misparsing.

Validation

$ python3 -m unittest tests.test_materialize_session_query -v
Ran 26 tests in 0.100s
OK

$ python3 -m unittest discover -s tests -p "test_*.py" -v
Ran 105 tests in 2.012s
OK

$ python3 -m py_compile scripts/materialize-session-query.py tests/test_materialize_session_query.py
$ python3 -m compileall -q plugins/repo-dreamer/skills/repository-skill-forge
(no errors)

No repo-wide lint/format tooling is configured for this repository, so none was run.

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>
@Morabbin Morabbin self-assigned this Aug 20, 2026
@GolaraJ
GolaraJ requested a balanced review from Copilot August 20, 2026 23:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates session query materialization to accept local-store results containing a trailing _query_source column.

Changes:

  • Removes trailing _query_source headers 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants