Conversation
…ifact
Write the temporary ID map (aw_* → {repo, number}) to
/tmp/gh-aw/temporary-id-map.json during safe output processing.
Include this file alongside the existing safe-output-items.jsonl
in the safe-outputs-items artifact for review and audit.
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/a69d61aa-3e4f-4aa3-8672-06f44bed5bb5
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds persistence of the safe-outputs “temporary ID map” to disk and includes it in the safe-outputs-items artifact to enable post-run review/audit without relying on step outputs.
Changes:
- Add a JS constant for
/tmp/gh-aw/temporary-id-map.jsonand implementwriteTemporaryIdMapFile()to write the full map as pretty-printed JSON. - Update the safe output handler manager to write the temporary ID map file (non-staged mode) in addition to exporting it as an action output.
- Update the Go workflow compiler to upload both
safe-output-items.jsonlandtemporary-id-map.jsonin thesafe-outputs-itemsartifact (and recompile workflow lock files accordingly).
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/constants.cjs |
Defines TEMPORARY_ID_MAP_FILE_PATH under /tmp/gh-aw/ for runtime output. |
actions/setup/js/constants.test.cjs |
Adds coverage ensuring the new constant is exported and under TMP_GH_AW_PATH. |
actions/setup/js/safe_output_manifest.cjs |
Implements writeTemporaryIdMapFile() and exports the new helper/constant. |
actions/setup/js/safe_output_manifest.test.cjs |
Adds tests for writing the temp ID map file (empty/non-empty, mkdirs, overwrite, error path). |
actions/setup/js/safe_output_handler_manager.cjs |
Writes the temp ID map JSON file after processing (skipped in staged mode). |
pkg/constants/job_constants.go |
Introduces TemporaryIdMapFilename used for workflow artifact upload paths. |
pkg/constants/constants_test.go |
Adds assertion for TemporaryIdMapFilename. |
pkg/workflow/compiler_safe_outputs_job.go |
Updates artifact upload step to include both manifest and temp ID map via multiline path. |
.github/workflows/*.lock.yml |
Recompiled workflow lock files to upload the additional temporary-id-map.json file. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 187/187 changed files
- Comments generated: 4
| // TemporaryIdMapFilename is the filename of the temporary ID map JSON file written to /tmp/gh-aw/. | ||
| // This file contains a JSON object mapping temporary IDs (e.g., aw_abc123) to their resolved | ||
| // GitHub resource references ({repo, number}) for review and audit purposes. | ||
| // It is uploaded alongside the safe-output-items.jsonl manifest in the safe-outputs-items artifact. | ||
| const TemporaryIdMapFilename = "temporary-id-map.json" |
There was a problem hiding this comment.
The comment for TemporaryIdMapFilename says the JSON maps temporary IDs to {repo, number} pairs, but the JS implementation stores other shapes in the temporary ID map too (e.g. { projectUrl } in actions/setup/js/create_project.cjs:438-443 and { draftItemId } in actions/setup/js/update_project.cjs:836-841). Please update this docstring to reflect that values can be different resolved reference types (or explicitly constrain what gets written to this file).
| * The file contains a pretty-printed JSON object mapping temporary IDs to their resolved | ||
| * GitHub resource references for review and audit purposes. | ||
| * | ||
| * @param {Object} temporaryIdMap - The temporary ID map object (keys are temp IDs, values are {repo, number}) | ||
| * @param {string} [filePath] - Path to the output file (defaults to TEMPORARY_ID_MAP_FILE_PATH) | ||
| */ | ||
| function writeTemporaryIdMapFile(temporaryIdMap, filePath = TEMPORARY_ID_MAP_FILE_PATH) { |
There was a problem hiding this comment.
writeTemporaryIdMapFile docstring/param typing says map values are {repo, number}, but the temporary ID map can contain other resolved reference shapes (e.g. { projectUrl } / { draftItemId }). This makes the artifact format misleading for auditors/consumers. Please relax/clarify the JSDoc to match actual contents (or filter the map before writing if only repo/number pairs are intended).
| // Write temporary ID map to file for inclusion in the safe-outputs-items artifact. | ||
| // This allows reviewers and auditors to inspect the full map of temporary IDs | ||
| // to resolved GitHub resources (issue numbers, repos) without parsing step outputs. | ||
| if (!isStaged) { | ||
| writeTemporaryIdMapFile(processingResult.temporaryIdMap); | ||
| core.info(`Wrote temporary ID map to file for artifact upload`); |
There was a problem hiding this comment.
The inline comment describes the temporary ID map as mapping to resolved issue {repo, number} references, but the map can also contain non-issue mappings (e.g. project URLs, draft item IDs) depending on which handlers ran. Consider rewording this comment (and the core.info message if needed) to avoid implying the artifact only contains issue mappings.
| it("should create parent directories if they do not exist", () => { | ||
| const deepPath = `/tmp/test-temp-id-deep-${Math.random().toString(36).substring(7)}/a/b/c/map.json`; | ||
| writeTemporaryIdMapFile({ aw_test: { repo: "o/r", number: 1 } }, deepPath); | ||
|
|
||
| expect(fs.existsSync(deepPath)).toBe(true); | ||
| const parsed = JSON.parse(fs.readFileSync(deepPath, "utf8")); | ||
| expect(parsed.aw_test).toEqual({ repo: "o/r", number: 1 }); | ||
|
|
||
| // Cleanup | ||
| fs.rmSync(path.dirname(deepPath).split("/a/")[0] + "/a", { recursive: true, force: true }); | ||
| }); |
There was a problem hiding this comment.
This test creates a deep temp directory under /tmp/test-temp-id-deep-*, but the manual cleanup only removes the /a subdirectory, leaving the parent directory behind. Over many test runs this can leak directories in /tmp and make failures harder to debug. Prefer deleting the top-level temp directory you created (e.g., the /tmp/test-temp-id-deep-* prefix) instead of reconstructing a partial path with split("/a/").
🧪 Test Quality Sentinel ReportTest Quality Score: 65/100
Test Classification DetailsView all 8 test cases
Flagged Tests — Suggestions
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 65/100. Test quality is acceptable — 25% of new tests are implementation tests (threshold: 30%). The writeTemporaryIdMapFile behavioural suite is well-structured with real file I/O, covering empty inputs, nested directories, overwrite semantics, and error propagation.
Summary
Store the temporary ID maps in safe outputs to a file and include it in the safe outputs artifact for review and audit.
During safe output processing, the handler manager now writes the complete temporary ID map (
aw_*→{repo, number}) to/tmp/gh-aw/temporary-id-map.json. This file is uploaded alongside the existingsafe-output-items.jsonlmanifest in thesafe-outputs-itemsartifact.Changes
JavaScript (
actions/setup/js/)constants.cjs: AddedTEMPORARY_ID_MAP_FILE_PATHconstant (/tmp/gh-aw/temporary-id-map.json)safe_output_manifest.cjs: AddedwriteTemporaryIdMapFile()function that writes the temporary ID map as pretty-printed JSON to a file, with directory creation and error handlingsafe_output_handler_manager.cjs: After exporting the temporary ID map viacore.setOutput(), also writes it to disk viawriteTemporaryIdMapFile()(skipped in staged mode)constants.test.cjs: Added test for the newTEMPORARY_ID_MAP_FILE_PATHconstantsafe_output_manifest.test.cjs: Added tests forwriteTemporaryIdMapFile()covering empty maps, maps with entries, directory creation, file overwriting, and error handlingGo (
pkg/)pkg/constants/job_constants.go: AddedTemporaryIdMapFilenameconstant (temporary-id-map.json)pkg/constants/constants_test.go: Added test assertion for the new constantpkg/workflow/compiler_safe_outputs_job.go: UpdatedbuildSafeOutputItemsManifestUploadStep()to include bothsafe-output-items.jsonlandtemporary-id-map.jsonin the artifact upload using multilinepathsyntaxCompiled Workflows (
.github/workflows/*.lock.yml)All lock files recompiled to reflect the updated artifact upload path.
File produced at runtime
Testing
TestConstantValues/TemporaryIdMapFilename)TestBuildConsolidatedSafeOutputsJob,TestJobOutputs,TestStepOrder*)make fmtandmake lintpassmake recompilesucceeds