Repository navigation
feat(pr-check): guard task PRs against Squad internal artifacts - #257
Merged
Merged
Conversation
added 7 commits
October 9, 2026 15:49
Contributor
Author
Coding agent usage on this pull request
Token and model breakdown
Source: Pi session JSONL usage records and Drill agent-invocation records, covering the task lifetime through report generation. Provider-recorded costs and list-price estimates are labeled separately; subscription usage is not represented as spend. |
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.
Intent
Make Squad's internal execution checklists impossible to accidentally ship in a project commit, preventing recurrence of the T04 and T10 incidents where checklists were removed from delivered PRs after an ambiguous data//artifacts/ path was resolved against the project repository. The plan-execute@1 brief must state that the checklist lives only in the Squad base's data//artifacts/ and that no project commit may contain a Squad internal artifact. Generated briefs must carry that explicit path and prohibition. When sq-pr-check records a task PR, inspect changed file paths and prominently name any file under that task's data// path; inability to fetch GitHub files remains non-fatal, while GitLab file-list fetch or parsing failures must be reported loudly rather than skipped. For GitLab, use the supported, version-verified glab default-JSON paginated diff response parsed with python3 JSONDecoder.raw_decode, never unsupported --jq or --output flags. Add behavior tests for generated guidance, synthetic task-specific artifact paths, GitLab pagination/fetch failure, GitHub wiring, and a glab test stub that rejects unknown flags. Preserve existing brief, playbook, and PR-check behavior; leave project CONTRIBUTING/CI enforcement and retroactive cleanup of merged commits out of scope, and do not change plan-execute@1's required plan fields or any other playbook. Keep scripts shellcheck-clean and follow Squad's one-sentence-per-line and no-agent-co-author conventions.
What Changed
bin/sq-pr-artifact-guard.sh, which reports changed paths underdata/<task-id>/, and wiredsq-pr-check.shto run it against the PR file list so task-owned Squad artifacts are named before arming; GitHub fetch failures stay non-fatal while GitLab fetch or parse failures are reported loudly.glab api .../merge_requests/<n>/diffswith--paginateand an explicit--hostname, parsed withpython3JSONDecoder.raw_decode, avoiding unsupported--jq/--outputflags.plan-execute@1playbook to state the checklist lives only in the Squad base'sdata/<id>/artifacts/and that no project commit may contain a Squad internal artifact, and added behavior tests plus docs for generated guidance, task-specific paths, GitLab pagination/fetch/parse handling, GitHub wiring, and a glab stub that rejects unknown flags.Risk Assessment
✅ Low: The change is a bounded, advisory artifact-path guard plus guidance text and behavior tests; the only fix-round code change (
--hostname) is a version-supported glab flag and is directly asserted by its test, so no material source risk remains.Testing
Ran the three directly affected test files (new sq-pr-artifact-guard test, sq-brief, and the full sq-pr-check-security suite) and all passed; the official runner executes the new test in family=pr-forge and the coverage guard accepts it. I then exercised the real end-user surfaces: generated the plan-execute@1 brief and confirmed it states the checklist lives only in the Squad base's data/<id>/artifacts/ and that no project commit may carry a Squad internal artifact; drove the real bin/sq-pr-check.sh against a flag-rejecting stub glab/gh to show a paginated default-JSON GitLab diff response names task-owned paths (including a later page) and not other tasks, a GitLab fetch or parse failure is warned loudly while arming still succeeds, a clean list is silent, and the GitHub PR file list names the task-owned artifact while a GitHub fetch failure stays non-fatal. The real glab binary is not installed in this sandbox, so the GitLab CLI contract was validated against the documented, version-pinned flag set (glab 1.53.0:--paginate,--hostname, default json; no--jq) and the required flag-rejecting stub rather than the live CLI. Worktree left clean; unrelated 'cost report could not be published' lines in the transcripts come from the sandbox lacking the sq-gh shim and are not part of this change.Evidence: Generated plan-execute@1 brief with the Squad-base checklist location and commit prohibition
3. Followtlc-implementfor the implementation method and write the checklist only under the Squad base'sdata/<id>/artifacts/, never in the project repository. A project commit must never carry a Squad internal artifact. ... - A checklist under the Squad base'sdata/<id>/artifacts/recording the plan fields as realized... - No project commit contains this Squad internal artifact.Evidence: End-to-end sq-pr-check GitLab guard: paginated naming, fetch failure, clean silence
### SCENARIO 1: paginated default JSON with a task-owned artifact warning: PR contains Squad internal artifact path(s) for task task-a; remove them before merge: data/task-a/artifacts/checklist.md data/task-a/artifacts/page-2.md armed: state/task-a.check.sh exit=0 --- glab invocations --- GLAB CALL: api projects/group%2Fsubgroup%2Fproject/merge_requests/7/diffs --paginate --hostname gitlab.example ### SCENARIO 2: GitLab fetch failure must be reported loudly (non-fatal) warning: GitLab merge request https://gitlab.example/group/subgroup/project/-/merge_requests/7 was not inspected for Squad internal artifacts because its changed-file list could not be fetched. armed: state/task-a.check.sh exit=0 ### SCENARIO 3: clean file list must be silent armed: state/task-a.check.sh exit=0Evidence: End-to-end sq-pr-check GitLab parse failure plus GitHub naming and non-fatal fetch failure
### SCENARIO 4: malformed GitLab JSON must be reported loudly (non-fatal) json.decoder.JSONDecodeError: Expecting value: line 1 column 39 (char 38) warning: GitLab merge request https://gitlab.example/group/project/-/merge_requests/7 was not inspected for Squad internal artifacts because its changed-file response could not be parsed. armed: state/task-a.check.sh exit=0 ### SCENARIO 5: GitHub PR file list names the task-owned artifact warning: PR contains Squad internal artifact path(s) for task task-a; remove them before merge: data/task-a/artifacts/checklist.md armed: state/task-a.check.sh exit=0 --- gh invocations --- GH CALL: pr view https://github.com/o/r/pull/7 --json headRefOid -q .headRefOid GH CALL: api repos/o/r/pulls/7/files --paginate --jq .[].filename ### SCENARIO 6: GitHub fetch failure stays non-fatal armed: state/task-a.check.sh exit=0Evidence: Reproducible demo scripts for the two end-to-end transcripts
Evidence: Reproducible demo script for parse-failure and GitHub scenarios
Pipeline
Updates from git push drill
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 3 issues found → auto-fixed (2) ✅
tests/sq-pr-check-security.test.sh:2915- The GitLab pagination fixture's second JSON page contains only an other-task path (data/other-task/report.md), which the guard filters out. Every assertion intest_gitlab_artifact_guardwould still pass if the parser read only page 1, so the test does not actually prove thatglab api --paginateoutput beyond the first page is parsed. Put a task-owned artifact (e.g.data/task-a/artifacts/page-2.md) on the second page and assert it is named, so thepass "...parses paginated default JSON..."claim is substantiated.bin/sq-pr-check.sh:108- The loud parse-failure branch for GitLab was added here, but no test exercises it: the GitLab fixture feeds valid JSON (two pages), an empty file, and a fetch failure. The intent requires that parsing failures be reported loudly rather than skipped, so add a malformed-JSON fixture and assert thecould not be parsedwarning (while arming still succeeds).bin/sq-pr-check.sh:114- The new artifact inspection is advisory: it prints a stderr warning and then still arms the merge poll (|| true), so adata/<task-id>/...file committed to the PR remains shippable if the warning is missed. That keeps the T04/T10 failure technically reachable, but the intent explicitly scopes out CONTRIBUTING/CI or commit-time enforcement and only requires the path to be named "prominently", so this is authorized containment and needs no action.🔧 Fix: Substantiate GitLab pagination and malformed-JSON guard behavior tests
2 issues (1 warning, 1 info) still open:
bin/sq-pr-check.sh:88- The GitLab artifact inspection runsglab api "projects/$ENCODED_PROJECT/merge_requests/$NUMBER/diffs" --paginatewith no--hostname "$HOST". In the pinned glab 1.53.0,glab apitargets gitlab.com or the authenticated host of the current git directory unless--hostname(or GITLAB_HOST) overrides it; it does not derive the instance from the endpoint. Squad explicitly supports self-hosted GitLab and the poll resolves the instance from the recorded project URL via-R, so when glab's default host differs from the MR host this guard queries the wrong server. The common outcome is a 404 that only prints the loudwas not inspectedwarning while arming still succeeds, but if the default host happens to serve a project at the same namespace/path and MR number, the wrong MR is inspected silently and the intended MR's artifacts are never named. Pass--hostname "$HOST"; the test stub currently rejects--hostnameand must be updated to allow it and assert it is logged..agents/skills/execution-playbooks/references/plan-execute-v1.md:13- This changed reference file has no entry in the changed-file map ofbin/sq-test-run.sh. Runningbin/sq-test-run.sh --list --changed --base 1757d143073dbdbf49fd704b9f239cc21bf7ed9cexits 2 withno changed-test mapping for source path: .agents/skills/execution-playbooks/references/plan-execute-v1.md, becausefamilies_for_changed_pathonly special-cases.agents/skills/*/SKILL.mdand the literal-path test-reference scan finds no test containing this full path. The gap predates this change, but this required edit keeps tripping it. Consider adding a.agents/skills/execution-playbooks/references/* -> pure-contract-unitcase (which owns tests/sq-brief.test.sh) so the branch can be selected by the runner.🔧 Fix: Pin GitLab artifact diff fetch to reviewed host
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/sq-pr-artifact-guard.test.sh(guard names task-owned paths, ignores other tasks, silent when clean)bash tests/sq-brief.test.sh(generated plan-execute brief carries the Squad-base path and commit prohibition)bash tests/sq-pr-check-security.test.sh(full PR-check security suite, including new GitLab and GitHub artifact-guard cases)bash tests/sq-pr-check-security.test.sh | grep -iE 'artifact guard'(confirmed both new guard cases pass)bin/sq-test-run.sh tests/sq-pr-artifact-guard.test.sh(runner executes the new test in family=pr-forge)bin/sq-test-run.sh --list --family pr-forge | grep artifact-guardbin/sq-test-run.sh --check-coverage(coverage guard accepts the new test)SQUAD_BASE=<tmp> bin/sq-brief.sh lifecycle-plan-execute repo --mode drill --playbook plan-execute@1(generated real brief inspected)Manual E2Ebin/sq-pr-check.sh task-a <gitlab-url>with a stub glab that rejects unknown flags: paginated default-JSON success, fetch failure, and clean-list silenceManual E2Ebin/sq-pr-check.sh task-a <gitlab-url>with malformed JSON andbin/sq-pr-check.sh task-a <github-url>with a stub gh: parse-failure warning, GitHub path naming, and non-fatal GitHub fetch failuredocs/gitlab-merge-sentry.md:199- The new GitLab branch of bin/sq-pr-check.sh callsglab api ... --paginate --hostnameand parses default JSON with python3, but this maintainer-verification record still states glab JSON 'would need a JSON processor Squad does not require' and carries no version-verified evidence for the new invocation. The stub test pins the flag shape, yet glab is not installed in this environment to confirm the flags against the pinned 1.53.0 CLI. Left as a judgment call because the record's pr_head conclusion is unchanged and it is a dated empirical artifact.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.